Re: [PATCH 1/4] tests/guest-debug: Make QEMU optional in run-test.py

["Alex Bennée" <alex.bennee@linaro.org>]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> Hi Alex,
>
> On 8/25/25 14:26, Gustavo Romero wrote:
>> Hi Alex!
>> On 8/25/25 14:01, Alex Bennée wrote:
>>> Gustavo Romero <gustavo.romero@linaro.org> writes:
>>>
>>>> This commit makes QEMU optional in run-test.py, allowing it to be used
>>>> as a GDB runner, i.e., to call GDB and pass a test script to it without
>>>> launching QEMU. In this configuration, it is the test script’s duty to
>>>> configure and run the VMs that GDB connects to.
>>>>
>>>> The --binary option continues to be required when --qemu is passed.
>>>> sys.argv now includes the full path to the test script in addition to
>>>> the script’s arguments, which allows unittest introspection to work
>>>> properly in case it is used in the test script.
>>>>
>>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>>> ---
>>>>   tests/guest-debug/run-test.py | 81 +++++++++++++++++++----------------
>>>>   1 file changed, 45 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
>>>> index 75e9c92e03..7fa17aedca 100755
>>>> --- a/tests/guest-debug/run-test.py
>>>> +++ b/tests/guest-debug/run-test.py
>>>> @@ -22,10 +22,10 @@
>>>>   def get_args():
>>>>       parser = argparse.ArgumentParser(description="A gdbstub test runner")
>>>>       parser.add_argument("--qemu", help="Qemu binary for test",
>>>> -                        required=True)
>>>> +                        required=False)
>>>>       parser.add_argument("--qargs", help="Qemu arguments for test")
>>>>       parser.add_argument("--binary", help="Binary to debug",
>>>> -                        required=True)
>>>> +                        required=False)
>>>
>>>      parser.add_argument("--binary", help="Binary to debug",
>>>                          required='--qemu' in sys.argv)
>>>
>>>>       parser.add_argument("--test", help="GDB test script")
>>>>       parser.add_argument('test_args', nargs='*',
>>>>                           help="Additional args for GDB test script. "
>>>> @@ -53,7 +53,7 @@ def log(output, msg):
>>>>   if __name__ == '__main__':
>>>>       args = get_args()
>>>> -    # Search for a gdb we can use
>>>> +    # Search for a gdb we can use.
>>>>       if not args.gdb:
>>>>           args.gdb = shutil.which("gdb-multiarch")
>>>>       if not args.gdb:
>>>> @@ -73,41 +73,49 @@ def log(output, msg):
>>>>       socket_dir = TemporaryDirectory("qemu-gdbstub")
>>>>       socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
>>>> -    # Launch QEMU with binary
>>>> -    if "system" in args.qemu:
>>>> -        if args.no_suspend:
>>>> -            suspend = ''
>>>> -        else:
>>>> -            suspend = ' -S'
>>>> -        cmd = f'{args.qemu} {args.qargs} {args.binary}' \
>>>> -            f'{suspend} -gdb unix:path={socket_name},server=on'
>>>> -    else:
>>>> -        if args.no_suspend:
>>>> -            suspend = ',suspend=n'
>>>> -        else:
>>>> -            suspend = ''
>>>> -        cmd = f'{args.qemu} {args.qargs} -g {socket_name}{suspend}' \
>>>> -            f' {args.binary}'
>>>> -
>>>> -    log(output, "QEMU CMD: %s" % (cmd))
>>>> -    inferior = subprocess.Popen(shlex.split(cmd))
>>>> +    if args.qemu and not args.binary:
>>>> +        print("QEMU needs a binary to run, but no binary provided")
>>>> +        exit(-1)
>>>
>>> then we can avoid this.
>> Sure, thanks for the suggestion. I'm fixing it in v2.
>> 
>>>> -    # Now launch gdb with our test and collect the result
>>>> -    gdb_cmd = "%s %s" % (args.gdb, args.binary)
>>>> +    if args.qemu:
>>>> +        # Launch QEMU with binary.
>>>> +        if "system" in args.qemu:
>>>> +            if args.no_suspend:
>>>> +                suspend = ''
>>>> +            else:
>>>> +                suspend = ' -S'
>>>> +            cmd = f'{args.qemu} {args.qargs} {args.binary}' \
>>>> +                f'{suspend} -gdb unix:path={socket_name},server=on'
>>>> +        else:
>>>> +            if args.no_suspend:
>>>> +                suspend = ',suspend=n'
>>>> +            else:
>>>> +                suspend = ''
>>>> +            cmd = f'{args.qemu} {args.qargs} -g {socket_name}{suspend}' \
>>>> +                f' {args.binary}'
>>>> +
>>>> +        log(output, "QEMU CMD: %s" % (cmd))
>>>> +        inferior = subprocess.Popen(shlex.split(cmd))
>>>> +
>>>> +    # Now launch gdb with our test and collect the result.
>>>> +    gdb_cmd = args.gdb
>>>> +    if args.binary:
>>>> +        gdb_cmd += " %s" % (args.binary)
>>>>       if args.gdb_args:
>>>>           gdb_cmd += " %s" % (args.gdb_args)
>>>> -    # run quietly and ignore .gdbinit
>>>> +    # Run quietly and ignore .gdbinit.
>>>>       gdb_cmd += " -q -n -batch"
>>>> -    # disable pagination
>>>> +    # Disable pagination.
>>>>       gdb_cmd += " -ex 'set pagination off'"
>>>> -    # disable prompts in case of crash
>>>> +    # Disable prompts in case of crash.
>>>>       gdb_cmd += " -ex 'set confirm off'"
>>>
>>> The re-formatting makes the diffs very noisy. If you want to clean up
>>> the captilization of stuff do that in another commit.
>> OK.
>> 
>>>> -    # connect to remote
>>>> -    gdb_cmd += " -ex 'target remote %s'" % (socket_name)
>>>> -    # finally the test script itself
>>>> +    # Connect automatically to remote only if QEMU is launched.
>>>> +    if args.qemu:
>>>> +        gdb_cmd += " -ex 'target remote %s'" % (socket_name)
>>>> +    # Finally the test script itself.
>>>>       if args.test:
>>>> -        if args.test_args:
>>>> -            gdb_cmd += f" -ex \"py sys.argv={args.test_args}\""
>>>> +        argv = [args.test] + args.test_args
>>>> +        gdb_cmd += f" -ex \"py sys.argv={argv}\""
>>>>           gdb_cmd += " -x %s" % (args.test)
>>>
>>> I can see this echoes from:
>>>
>>>     env QEMU_TEST_FLAKY_TESTS=1 ./pyvenv/bin/meson test --suite thorough func-aarch64-aarch64_reverse_debug --verbose
>>>
>>> Shows:
>>>
>>>    GDB CMD: /usr/bin/gdb-multiarch -q -n -batch -ex 'set pagination
>>> off' -ex 'set confirm off' -ex "py
>>> sys.argv=['/home/alex/lsrc/qemu.git/tests/functional/test_aarch64_reverse_debug.py']"
>>> -x
>>> /home/alex/lsrc/qemu.git/tests/functional/test_aarch64_reverse_debug.py
>>>
>>> But trying to piece that together on my the command line:
>>>
>>>    env
>>> PYTHONPATH=/home/alex/lsrc/qemu.git/python:/home/alex/lsrc/qemu.git/tests/functional
>>> /usr/bin/gdb-multiarch -q -n -batch -ex 'set pagination off' -ex
>>> 'set confirm off' -ex "py
>>> sys.argv=['/home/alex/lsrc/qemu.git/tests/functional/test_aarch64_reverse_debug.py']"
>>> -x
>>> /home/alex/lsrc/qemu.git/tests/functional/test_aarch64_reverse_debug.py
>>> Python Exception <class 'ModuleNotFoundError'>: No module named 'pycotap'
>>> Error occurred in Python: No module named 'pycotap'
>>>
>>> What am I missing?
>> meson.build in tests/functional is properly setting PYTHONPATH in
>> the env
>> when meson runs this command. libpython called from gdb binary will inspect
>> PYTHONPATH later. It's meson that knows where site-packages from the pyvenv
>> is located, so that's why meson is setting PYTHONPATH and that's why it works
>> when run by meson.
>> We should never need to put pieces together to run QEMU tests, I
>> really
>> hate it (see my previous reply to Thomas about why using Avocado GDB for
>> test like this is not a good idea IMO).
>
> ah, and I can think of some way to avoid having to put command pieces
> together to re-run the test after we agree in the other more fundamental
> aspects of this series, e.g., ok to kept it in tests/functional, ok to
> use GDB Python API, etc.

Hmm could meson devenv help?

>
> Cheers,
> Gustavo
>
>> That said, try to add the site-packages from your pyvenv set in your
>> build dir to PYTHONPATH:
>> gromero@gromero0:/mnt/git/qemu_/build$ ls -l
>> ./pyvenv/lib/python3.10/site-packages
>> total 16
>> drwxrwxr-x  3 gromero gromero 4096 Aug 25 12:47 meson-1.8.1.dist-info
>> drwxrwxr-x 17 gromero gromero 4096 Aug 25 12:47 mesonbuild
>> drwxrwxr-x  3 gromero gromero 4096 Aug 25 12:47 pycotap
>> drwxrwxr-x  2 gromero gromero 4096 Aug 25 12:47 pycotap-1.3.1.dist-info
>> This is where the pycotap and other potential modules reside.
>> Cheers,
>> Gustavo
>> 
>>>> @@ -129,10 +137,11 @@ def log(output, msg):
>>>>           log(output, "GDB crashed? (%d, %d) SKIPPING" % (result, result - 128))
>>>>           exit(0)
>>>> -    try:
>>>> -        inferior.wait(2)
>>>> -    except subprocess.TimeoutExpired:
>>>> -        log(output, "GDB never connected? Killed guest")
>>>> -        inferior.kill()
>>>> +    if args.qemu:
>>>> +        try:
>>>> +            inferior.wait(2)
>>>> +        except subprocess.TimeoutExpired:
>>>> +            log(output, "GDB never connected? Killed guest")
>>>> +            inferior.kill()
>>>>       exit(result)
>>>
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro