Re: [PATCH 06/29] tests/tcg: add an explicit gdbstub register tester

[Akihiko Odaki]

On 2023/11/04 4:59, Alex Bennée wrote:
> We already do a couple of "info registers" for specific tests but this
> is a more comprehensive multiarch test. It also has some output
> helpful for debugging the gdbstub by showing which XML features are
> advertised and what the underlying register numbers are.
>
> My initial motivation was to see if there are any duplicate register
> names exposed via the gdbstub while I was reviewing the proposed
> register interface for TCG plugins.
>
> Mismatches between the xml and remote-desc are reported for debugging
> but do not fail the test.
>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Luis Machado <luis.machado@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20231012170426.1335442-1-alex.bennee@linaro.org>
>
> ---
> v2
>    - remove python2 compat bits
>    - add SPDX header, clean up comment lines
>    - fix duplicate check
>    - use field 6 (Rmt Nr) instead of field 1 (Nr) for cross-check
>    - more useful output on finding a duplicates and missing regs
>    - handle non-XML targets cleanly
> ---
>   tests/tcg/multiarch/Makefile.target           |  11 +-
>   tests/tcg/multiarch/gdbstub/registers.py      | 188 ++++++++++++++++++
>   .../multiarch/system/Makefile.softmmu-target  |  13 +-
>   3 files changed, 210 insertions(+), 2 deletions(-)
>   create mode 100644 tests/tcg/multiarch/gdbstub/registers.py
>
> diff --git a/tests/tcg/multiarch/Makefile.target 
> b/tests/tcg/multiarch/Makefile.target
> index f3bfaf1a22..d31ba8d6ae 100644
> --- a/tests/tcg/multiarch/Makefile.target
> +++ b/tests/tcg/multiarch/Makefile.target
> @@ -93,12 +93,21 @@ run-gdbstub-thread-breakpoint: testthread
>                 --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>                 --bin $< --test 
> $(MULTIARCH_SRC)/gdbstub/test-thread-breakpoint.py, \
>         hitting a breakpoint on non-main thread)
> +
> +run-gdbstub-registers: sha512
> +       $(call run-test, $@, $(GDB_SCRIPT) \
> +               --gdb $(GDB) \
> +               --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> +               --bin $< --test $(MULTIARCH_SRC)/gdbstub/registers.py, \
> +       checking register enumeration)
> +
>   else
>   run-gdbstub-%:
>         $(call skip-test, "gdbstub test $*", "need working gdb with $(patsubst 
> -%,,$(TARGET_NAME)) support")
>   endif
>   EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
> -             run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint
> +             run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint \
> +             run-gdbstub-registers
>   
>   # ARM Compatible Semi Hosting Tests
>   #
> diff --git a/tests/tcg/multiarch/gdbstub/registers.py 
> b/tests/tcg/multiarch/gdbstub/registers.py
> new file mode 100644
> index 0000000000..2aa0c30165
> --- /dev/null
> +++ b/tests/tcg/multiarch/gdbstub/registers.py
> @@ -0,0 +1,188 @@
> +# Exercise the register functionality by exhaustively iterating
> +# through all supported registers on the system.
> +#
> +# This is launched via tests/guest-debug/run-test.py but you can also
> +# call it directly if using it for debugging/introspection:
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import gdb
> +import sys
> +import xml.etree.ElementTree as ET
> +
> +initial_vlen = 0

This seems unused.

> +failcount = 0
> +
> +def report(cond, msg):
> +    "Report success/fail of test."
> +    if cond:
> +        print("PASS: %s" % (msg))
> +    else:
> +        print("FAIL: %s" % (msg))
> +        global failcount
> +        failcount += 1

I suggest using unittest to remove this boilerplate.

> +
> +
> +def fetch_xml_regmap():
> +    """
> +    Iterate through the XML descriptions and validate.
> +
> +    We check for any duplicate registers and report them. Return a
> +    reg_map hash containing the names, regnums and initial values of
> +    all registers.
> +    """
> +
> +    # First check the XML descriptions we have sent. Most arches
> +    # support XML but a few of the ancient ones don't in which case we
> +    # need to gracefully fail.
> +
> +    try:
> +        xml = gdb.execute("maint print xml-tdesc", False, True)
> +    except (gdb.error):
> +        print("SKIP: target does not support XML")
> +        return None
> +
> +    total_regs = 0
> +    reg_map = {}
> +    frame = gdb.selected_frame()
> +
> +    tree = ET.fromstring(xml)
> +    for f in tree.findall("feature"):
> +        name = f.attrib["name"]
> +        regs = f.findall("reg")
> +
> +        total = len(regs)
> +        total_regs += total
> +        base = int(regs[0].attrib["regnum"])
> +        top = int(regs[-1].attrib["regnum"])
> +
> +        print(f"feature: {name} has {total} registers from {base} to {top}")
> +
> +        for r in regs:
> +            name = r.attrib["name"]
> +            regnum = int(r.attrib["regnum"])
> +            value = frame.read_register(name).__str__()

Does it really need conversion to string?

> +            entry = { "name": name, "initial": value, "regnum": regnum }
> +
> +            if name in reg_map:
> +                report(False, f"duplicate register {entry} vs {reg_map[name]}")
> +                continue
> +
> +            reg_map[name] = entry
> +
> +    # Validate we match
> +    report(total_regs == len(reg_map.keys()),
> +           f"counted all {total_regs} registers in XML")
> +
> +    return reg_map
> +
> +def crosscheck_remote_xml(reg_map):
> +    """
> +    Cross-check the list of remote-registers with the XML info.
> +    """
> +
> +    remote = gdb.execute("maint print remote-registers", False, True)
> +    r_regs = remote.split("\n")
> +
> +    total_regs = len(reg_map.keys())
> +    total_r_regs = 0
> +
> +    for r in r_regs:
> +        fields = r.split()
> +        # Some of the registers reported here are "pseudo" registers that
> +        # gdb invents based on actual registers so we need to filter them
> +        # out.
> +        if len(fields) == 8:
> +            r_name = fields[0]
> +            r_regnum = int(fields[6])
> +
> +            # check in the XML
> +            try:
> +                x_reg = reg_map[r_name]
> +                x_reg["seen"] = True

Place x_reg["seen"] out of this try block.