On 26/09/2025 18.00, Gustavo Romero wrote:
Hi Thomas,
On 9/26/25 05:44, Thomas Huth wrote:
On 26/09/2025 07.15, Gustavo Romero wrote:
This commit removes Avocado as a dependency for running the
reverse_debugging test.
The main benefit, beyond eliminating an extra dependency, is that there
is no longer any need to handle GDB packets manually. This removes the
need for ad-hoc functions dealing with endianness and arch-specific
register numbers, making the test easier to read. The timeout variable
is also removed, since Meson now manages timeouts automatically.
reverse_debugging now uses the pygdbmi module to interact with GDB, if
it's available in the test environment, otherwise the test is skipped.
GDB is detect via the QEMU_TEST_GDB env. variable.
This commit also significantly improves the output for the test and
now prints all the GDB commands used in sequence. It also adds
some clarifications to existing comments, for example, clarifying that
once the replay-break is reached, a SIGINT is captured in GDB.
reverse_debugging is kept "skipped" for aarch64, ppc64, and x86_64, so
won't run unless QEMU_TEST_FLAKY_TESTS=1 is set in the test environment,
before running 'make check-functional' or 'meson test [...]'.
Signed-off-by: Gustavo Romero <[email protected]>
---
...
@@ -53,49 +55,11 @@ def run_vm(self, record, shift, args, replay_path,
image_path, port):
vm.launch()
return vm
- @staticmethod
- def get_reg_le(g, reg):
- res = g.cmd(b'p%x' % reg)
- num = 0
- for i in range(len(res))[-2::-2]:
- num = 0x100 * num + int(res[i:i + 2], 16)
- return num
-
- @staticmethod
- def get_reg_be(g, reg):
- res = g.cmd(b'p%x' % reg)
- return int(res, 16)
-
- def get_reg(self, g, reg):
- # value may be encoded in BE or LE order
- if self.endian_is_le:
- return self.get_reg_le(g, reg)
- else:
- return self.get_reg_be(g, reg)
-
- def get_pc(self, g):
- return self.get_reg(g, self.REG_PC)
-
- def check_pc(self, g, addr):
- pc = self.get_pc(g)
- if pc != addr:
- self.fail('Invalid PC (read %x instead of %x)' % (pc, addr))
I think it would make sense to keep wrapper functions get_pc() and
check_pc(), since that functionality is still used in a bunch of places
(e.g. "gdb.cli("print $pc").get_addr()" is used a couple of times).
Yeah, I considered that, but I really think that using the
wrapper functions doesn't add to this test (as in the original test).
First because I like reading the test code and easily map it to
its output and, second, the original test that used check_pc() was
actually failing with this "Invalid PC ... " message in all the cases,
which is not informative. In my version, because I check PC in place,
I also fail with a specific message for each case, like "Forward stepping
failed¨,
"Reverse stepping failed", "reverse-continue", etc.
If you don't mind I'd like to keep the test this way.
Ok, fair point. But you could at least consider keep the wrapper for
get_pc(), I think.
+ @skipIfMissingEnv("QEMU_TEST_GDB")
Somehow this SKIP is always triggered on my system, even if I correctly
pointed "configure" to the right GDB with the "--gdb" option ... not sure
why this happens, but I'll try to find out...
It was the additional logic in patch 3 (see my other mail) that caused the
gdb detection to fail and thus the environment variable was not set.
Thomas
