add: testing framework and initial tests #14

ounuvar · 2024-07-12T12:54:02Z

Added a testing framework and initial tests for the memory decoder. Two classes called Field and Instruction in the testing-utils.hh file form the testing framework. Some of the initial tests currently fail because of bugs in the memory decoder, and the correct decoder outputs for some instructions (like prefetch instructions) are ambiguous.

To run the tests (while at the top of the qflex repository):
$ cd qemu/middleware/testing
$ cmake -S . -B build
$ cmake --build build -j$(nproc)
$ (cd build && ctest -j$(nproc))

…take less time

branylagaffe

Overall good.

Few design flaw. Few coding mistakes.

However, the comments are to improve. They are usually shallow and only reflect the meaning of the function name or the next line. Comments should indicate to the reader the possible state of the variable, why we need this function, the limitations of the function, any kind of asserted input/output.

branylagaffe · 2024-07-15T09:05:30Z

testing/CMakeLists.txt

As somewhat expected, the tests do not build OOTB, because the decoder require some qemu includes (osdep).
Two options from here:
1. Add a FLAG from the CmakeList.txt file which switch the header in the aforementioned files
2. Use QEMU build system to build the test, tricky but good.

Could you elaborate on the second option? How would it work?

Implemented the first option for now.

branylagaffe · 2024-07-15T09:06:45Z

testing/testing-utils.hh

+#ifndef TESTING_UTILS_HH
+#define TESTING_UTILS_HH
+
+#include <iostream>


Some of these include are not needed anymore

You're right. Fixing.

branylagaffe · 2024-07-15T09:09:15Z

testing/testing-utils.hh

+        std::string bitmask{""};
+
+        // Current iteration number (iterates from 0 to numIters - 1)
+        int currIter;


Use coherently the std defined integer (uint8_t, size_t, ....) and c++ defined integer (int, long).
I guess we can use the std defined ones here

branylagaffe · 2024-07-15T09:09:46Z

testing/testing-utils.hh

+    Instruction(Args&&... args);
+
+    // Vector to hold pointers to Field objects
+    std::vector<Field*> fields;


Any reason to use raw pointer instead of reference here ?

Apparently, we can't store references inside containers like vector in C++. I guess I could use a reference_wrapper object here as well (just like the void Instruction::addField(std::reference_wrapper field) function in testing-utils.cc). That might make even more sense actually since we already take the arguments as reference_wrapper objects.

branylagaffe · 2024-07-15T09:12:24Z

testing/testing-utils.hh

+    InstructionIterator end() const;
+
+  private:
+    // Variadic template function to add fields (Field objects or string literals) to the instruction


variadic is not an English word

Suggested change

// Variadic template function to add fields (Field objects or string literals) to the instruction

// specialized function ..

Variadic is the correct technical term I believe: https://en.wikipedia.org/wiki/Variadic

branylagaffe · 2024-07-15T09:27:31Z

testing/testing-utils.cc

+    bitmask += field->bitmask;
+  }
+  // The instruction bitmask must be 32 bits
+  assert(bitmask.size() == 32 && "Bitmask size must be 32.");


Your assert is wrongly defined.

assert((void("void helps to avoid 'unused value' warning"), 2 * 2 == 4)); OR assert((010 + 010 == 16) && "Yet another way to add an assert message"); OR assertm((2 + 2) % 3 == 1, "Success");

Notice the parentheses around the expression

== should have higher precedence than && but will add the parentheses just to make it clearer.

branylagaffe · 2024-07-15T09:30:29Z

testing/testing-utils.cc

+  assert(bitmask.size() == 32 && "Bitmask size must be 32.");
+
+  // Count the number of don't cares in the instruction bitmask
+  int numDontCares = std::count(bitmask.begin(), bitmask.end(), 'x');


Type coherence

Are you again referring to the use of builtin int instead of stdint types? (Or the lack of consisteny in the usage?)

branylagaffe · 2024-07-15T09:33:16Z

testing/testing-utils.cc

+  uint32_t result = 0;
+
+  // Generate the instruction based on the bitmask
+  for (int i = 0, j = 0; i < 32; ++i) {


Avoid hardcoded number (32, 31)

branylagaffe · 2024-07-15T09:36:07Z

testing/testing-utils.cc

+}
+
+// Dereference operator to get the current instruction value during range-based for loops
+uint32_t Instruction::InstructionIterator::operator*() const {


I don't understand why the dereference operator is link to generating an instruction, Shall not the instruction get pre-processed and retrieved by the deref. operator

Wdym by pre-processed? The exact instruction bits (the uint32_t value) for each iteration in for (uint32_t instr : bitmask) is generated during runtime through the dereference operator.

branylagaffe · 2024-07-15T09:38:15Z

testing/testing-utils.cc

+uint32_t Instruction::InstructionIterator::operator*() const {
+  uint32_t result = 0;
+
+  // Generate the instruction based on the bitmask


This loop should be a function in Instruction instead of the Iterator.
The loop is slightly unintuitive (basically everything with bitwise operator is); i would suggest decoupling this loop into more functions

The dereference operator (method) needs to be defined for the Iterator class. I could move the logic to a separate helper function in the Instruction class (and also maybe divide it into multiple functions in the meantime like you said) and call it from the dereference operator in the Iterator class.

Yep the deref. operator is needed for the Iterator, no question. As you said the logic should be moved elsewhere, and maybe pre-processed

Orhan Unuvar added 12 commits July 12, 2024 14:26

add: testing utils

78277b1

add: compare and swap pair test

8ee7146

add: instruction bitmask testing utility v1

f720368

add: instruction bitmask testing utility v2

73825a1

update: testing utils

aad98d5

update: load register literal test v2

2b15287

add: compare and swap pair test

fc23a3c

add: load store register pair tests

6ae8408

fix: remove redundant headers

24c67b0

fix: make test names more readable

9d20f9f

add: prefetch memory instruction test

945503f

add: new tests

0899c46

ounuvar requested a review from branylagaffe July 12, 2024 12:54

ounuvar self-assigned this Jul 12, 2024

ounuvar changed the title ~~Testing framework and initial tests~~ add: testing framework and initial tests Jul 12, 2024

Orhan Unuvar and others added 2 commits July 12, 2024 17:00

fix: replace EXPECT_EQ with ASSERT_EQ for all tests so failing tests …

4a0c06a

…take less time

fix: add missing argument for Cmake External Fetch

e01cd0e

branylagaffe requested changes Jul 15, 2024

View reviewed changes

Orhan Unuvar added 4 commits July 17, 2024 03:06

fix: ootb test compilation

36163ba

fix: refactor testing utils

3051947

fix: change file extensions to .cpp and .h, move "extern C" to trace.h

1859d38

add: branch decoder tests

320d4b6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: testing framework and initial tests #14

add: testing framework and initial tests #14

ounuvar commented Jul 12, 2024 •

edited

Loading

branylagaffe left a comment

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

ounuvar Jul 17, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

ounuvar Jul 15, 2024

branylagaffe Jul 15, 2024

	// Variadic template function to add fields (Field objects or string literals) to the instruction
	// specialized function ..

add: testing framework and initial tests #14

Are you sure you want to change the base?

add: testing framework and initial tests #14

Conversation

ounuvar commented Jul 12, 2024 • edited Loading

branylagaffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ounuvar commented Jul 12, 2024 •

edited

Loading