[PULL,06/23] tests/tcg: add an explicit gdbstub register tester

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.

We also skip the tests for the following arches for now until we can
investigate and fix any issues:

  - s390x (fails to read v0l->v15l, not seen in remote-registers)
  - ppc64 (fails to read vs0h->vs31h, not seen in remote-registers)

Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: Luis Machado <luis.machado@linaro.org>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: qemu-s390x@nongnu.org
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20231106185112.2755262-7-alex.bennee@linaro.org>

On Wed Nov 8, 2023 at 12:23 AM AEST, 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.
>
> We also skip the tests for the following arches for now until we can
> investigate and fix any issues:
>
>   - s390x (fails to read v0l->v15l, not seen in remote-registers)
>   - ppc64 (fails to read vs0h->vs31h, not seen in remote-registers)

binutils-gdb.git/gdb/rs6000-tdep.c has:

static const char *
rs6000_register_name (struct gdbarch *gdbarch, int regno)
{
  ppc_gdbarch_tdep *tdep = (ppc_gdbarch_tdep *) gdbarch_tdep (gdbarch);

  /* The upper half "registers" have names in the XML description,
     but we present only the low GPRs and the full 64-bit registers
     to the user.  */
  if (tdep->ppc_ev0_upper_regnum >= 0
      && tdep->ppc_ev0_upper_regnum <= regno
      && regno < tdep->ppc_ev0_upper_regnum + ppc_num_gprs)
    return "";

  /* Hide the upper halves of the vs0~vs31 registers.  */
  if (tdep->ppc_vsr0_regnum >= 0
      && tdep->ppc_vsr0_upper_regnum <= regno
      && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_gprs)
    return "";

(s390 looks similar for V0-V15 lower).

I guess it is because the upper half is not a real register but an
extension of an existing FP register to make a vector register. I
just don't know how that should be resolved with QEMU.

Should we put an exception in the test case for these? Or is there
something we should be doing differently with the XML regs?

i386 gdb does similar:

static const char *
i386_register_name (struct gdbarch *gdbarch, int regnum)
{
  /* Hide the upper YMM registers.  */
  if (i386_ymmh_regnum_p (gdbarch, regnum))
    return "";

  /* Hide the upper YMM16-31 registers.  */
  if (i386_ymmh_avx512_regnum_p (gdbarch, regnum))
    return "";

  /* Hide the upper ZMM registers.  */
  if (i386_zmmh_regnum_p (gdbarch, regnum))
    return "";

  return tdesc_register_name (gdbarch, regnum);
}

So, I'm not sure how they don't fail this test. Does QEMU just
not have YMM/ZMM in XML regmap?

Thanks,
Nick


>
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Cc: Luis Machado <luis.machado@linaro.org>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Cc: qemu-s390x@nongnu.org
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20231106185112.2755262-7-alex.bennee@linaro.org>
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b86ea7f75a..26e7633346 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2927,7 +2927,7 @@ F: gdbstub/*
>  F: include/exec/gdbstub.h
>  F: include/gdbstub/*
>  F: gdb-xml/
> -F: tests/tcg/multiarch/gdbstub/
> +F: tests/tcg/multiarch/gdbstub/*
>  F: scripts/feature_to_c.py
>  F: scripts/probe-gdb-support.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..ff6076b09e
> --- /dev/null
> +++ b/tests/tcg/multiarch/gdbstub/registers.py
> @@ -0,0 +1,197 @@
> +# 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
> +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
> +
> +
> +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"])
> +            try:
> +                value = frame.read_register(name)
> +            except ValueError:
> +                report(False, f"failed to read reg: {name}")
> +
> +            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]
> +            except KeyError:
> +                report(False, f"{r_name} not in XML description")
> +                continue
> +
> +            x_reg["seen"] = True
> +            x_regnum = x_reg["regnum"]
> +            if r_regnum != x_regnum:
> +                report(False, f"{r_name} {r_regnum} == {x_regnum} (xml)")
> +            else:
> +                total_r_regs += 1
> +
> +    # Just print a mismatch in totals as gdb will filter out 64 bit
> +    # registers on a 32 bit machine. Also print what is missing to
> +    # help with debug.
> +    if total_regs != total_r_regs:
> +        print(f"xml-tdesc has ({total_regs}) registers")
> +        print(f"remote-registers has ({total_r_regs}) registers")
> +
> +        for x_key in reg_map.keys():
> +            x_reg = reg_map[x_key]
> +            if "seen" not in x_reg:
> +                print(f"{x_reg} wasn't seen in remote-registers")
> +
> +def complete_and_diff(reg_map):
> +    """
> +    Let the program run to (almost) completion and then iterate
> +    through all the registers we know about and report which ones have
> +    changed.
> +    """
> +    # Let the program get to the end and we can check what changed
> +    b = gdb.Breakpoint("_exit")
> +    if b.pending: # workaround Microblaze weirdness
> +        b.delete()
> +        gdb.Breakpoint("_Exit")
> +
> +    gdb.execute("continue")
> +
> +    frame = gdb.selected_frame()
> +    changed = 0
> +
> +    for e in reg_map.values():
> +        name = e["name"]
> +        old_val = e["initial"]
> +
> +        try:
> +            new_val = frame.read_register(name)
> +        except:
> +            report(False, f"failed to read {name} at end of run")
> +            continue
> +
> +        if new_val != old_val:
> +            print(f"{name} changes from {old_val} to {new_val}")
> +            changed += 1
> +
> +    # as long as something changed we can be confident its working
> +    report(changed > 0, f"{changed} registers were changed")
> +
> +
> +def run_test():
> +    "Run through the tests"
> +
> +    reg_map = fetch_xml_regmap()
> +
> +    if reg_map is not None:
> +        crosscheck_remote_xml(reg_map)
> +        complete_and_diff(reg_map)
> +
> +
> +#
> +# This runs as the script it sourced (via -x, via run-test.py)
> +#
> +try:
> +    inferior = gdb.selected_inferior()
> +    arch = inferior.architecture()
> +    print("ATTACHED: %s" % arch.name())
> +except (gdb.error, AttributeError):
> +    print("SKIPPING (not connected)", file=sys.stderr)
> +    exit(0)
> +
> +if gdb.parse_and_eval('$pc') == 0:
> +    print("SKIP: PC not set")
> +    exit(0)
> +
> +try:
> +    run_test()
> +except (gdb.error):
> +    print ("GDB Exception: %s" % (sys.exc_info()[0]))
> +    failcount += 1
> +    pass
> +
> +print("All tests complete: %d failures" % failcount)
> +exit(failcount)
> diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target b/tests/tcg/multiarch/system/Makefile.softmmu-target
> index dee4f58dea..32dc0f9830 100644
> --- a/tests/tcg/multiarch/system/Makefile.softmmu-target
> +++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
> @@ -48,9 +48,20 @@ run-gdbstub-untimely-packet: hello
>  	$(call quiet-command, \
>  		(! grep -Fq 'Packet instead of Ack, ignoring it' untimely-packet.gdb.err), \
>  		"GREP", file untimely-packet.gdb.err)
> +
> +run-gdbstub-registers: memory
> +	$(call run-test, $@, $(GDB_SCRIPT) \
> +		--gdb $(GDB) \
> +		--qemu $(QEMU) \
> +		--output $<.registers.gdb.out \
> +		--qargs \
> +		"-monitor none -display none -chardev file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
> +		--bin $< --test $(MULTIARCH_SRC)/gdbstub/registers.py, \
> +	softmmu gdbstub support)
>  else
>  run-gdbstub-%:
>  	$(call skip-test, "gdbstub test $*", "need working gdb with $(patsubst -%,,$(TARGET_NAME)) support")
>  endif
>  
> -MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-interrupt run-gdbstub-untimely-packet
> +MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-interrupt \
> +	run-gdbstub-untimely-packet run-gdbstub-registers
> diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
> index 5721c159f2..1d08076756 100644
> --- a/tests/tcg/ppc64/Makefile.target
> +++ b/tests/tcg/ppc64/Makefile.target
> @@ -38,4 +38,11 @@ PPC64_TESTS += signal_save_restore_xer
>  PPC64_TESTS += xxspltw
>  PPC64_TESTS += test-aes
>  
> +ifneq ($(GDB),)
> +# Skip for now until vsx registers sorted out
> +run-gdbstub-registers:
> +	$(call skip-test, $<, "BROKEN reading VSX registers")
> +endif
> +
> +
>  TESTS += $(PPC64_TESTS)
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index 826f0a18e4..49af091c38 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -100,6 +100,10 @@ run-gdbstub-svc: hello-s390x-asm
>  		--bin $< --test $(S390X_SRC)/gdbstub/test-svc.py, \
>  	single-stepping svc)
>  
> +# Skip for now until vx registers sorted out
> +run-gdbstub-registers:
> +	$(call skip-test, $<, "BROKEN reading VX registers")
> +
>  EXTRA_RUNS += run-gdbstub-signals-s390x run-gdbstub-svc
>  endif
>

"Nicholas Piggin" <npiggin@gmail.com> writes:

> On Wed Nov 8, 2023 at 12:23 AM AEST, 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.
>>
>> We also skip the tests for the following arches for now until we can
>> investigate and fix any issues:
>>
>>   - s390x (fails to read v0l->v15l, not seen in remote-registers)
>>   - ppc64 (fails to read vs0h->vs31h, not seen in remote-registers)
>
> binutils-gdb.git/gdb/rs6000-tdep.c has:
>
> static const char *
> rs6000_register_name (struct gdbarch *gdbarch, int regno)
> {
>   ppc_gdbarch_tdep *tdep = (ppc_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>
>   /* The upper half "registers" have names in the XML description,
>      but we present only the low GPRs and the full 64-bit registers
>      to the user.  */
>   if (tdep->ppc_ev0_upper_regnum >= 0
>       && tdep->ppc_ev0_upper_regnum <= regno
>       && regno < tdep->ppc_ev0_upper_regnum + ppc_num_gprs)
>     return "";
>
>   /* Hide the upper halves of the vs0~vs31 registers.  */
>   if (tdep->ppc_vsr0_regnum >= 0
>       && tdep->ppc_vsr0_upper_regnum <= regno
>       && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_gprs)
>     return "";
>
> (s390 looks similar for V0-V15 lower).
>
> I guess it is because the upper half is not a real register but an
> extension of an existing FP register to make a vector register. I
> just don't know how that should be resolved with QEMU.
>
> Should we put an exception in the test case for these? Or is there
> something we should be doing differently with the XML regs?

Yeah I suspect this is just inconsistency between targets on gdb. My
naive assumption was XML should match the displayed registers but it
seems there is additional filtering going on.

It seems in this case the registers are still there and have regnums (so
I assume the stub could be asked for them) but the names have been
squashed. I guess we could detect that and accept it?

>
> i386 gdb does similar:
>
> static const char *
> i386_register_name (struct gdbarch *gdbarch, int regnum)
> {
>   /* Hide the upper YMM registers.  */
>   if (i386_ymmh_regnum_p (gdbarch, regnum))
>     return "";
>
>   /* Hide the upper YMM16-31 registers.  */
>   if (i386_ymmh_avx512_regnum_p (gdbarch, regnum))
>     return "";
>
>   /* Hide the upper ZMM registers.  */
>   if (i386_zmmh_regnum_p (gdbarch, regnum))
>     return "";
>
>   return tdesc_register_name (gdbarch, regnum);
> }
>
> So, I'm not sure how they don't fail this test. Does QEMU just
> not have YMM/ZMM in XML regmap?

No I think we only send the core one with XMM regs and there are no
additional registers sent via gdb_register_coprocessor.

>
> Thanks,
> Nick
>
>
>>
>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Cc: Luis Machado <luis.machado@linaro.org>
>> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
>> Cc: qemu-s390x@nongnu.org
>> Cc: Nicholas Piggin <npiggin@gmail.com>
>> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
>> Cc: qemu-ppc@nongnu.org
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20231106185112.2755262-7-alex.bennee@linaro.org>
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b86ea7f75a..26e7633346 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2927,7 +2927,7 @@ F: gdbstub/*
>>  F: include/exec/gdbstub.h
>>  F: include/gdbstub/*
>>  F: gdb-xml/
>> -F: tests/tcg/multiarch/gdbstub/
>> +F: tests/tcg/multiarch/gdbstub/*
>>  F: scripts/feature_to_c.py
>>  F: scripts/probe-gdb-support.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..ff6076b09e
>> --- /dev/null
>> +++ b/tests/tcg/multiarch/gdbstub/registers.py
>> @@ -0,0 +1,197 @@
>> +# 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
>> +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
>> +
>> +
>> +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"])
>> +            try:
>> +                value = frame.read_register(name)
>> +            except ValueError:
>> +                report(False, f"failed to read reg: {name}")
>> +
>> +            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]
>> +            except KeyError:
>> +                report(False, f"{r_name} not in XML description")
>> +                continue
>> +
>> +            x_reg["seen"] = True
>> +            x_regnum = x_reg["regnum"]
>> +            if r_regnum != x_regnum:
>> +                report(False, f"{r_name} {r_regnum} == {x_regnum} (xml)")
>> +            else:
>> +                total_r_regs += 1
>> +
>> +    # Just print a mismatch in totals as gdb will filter out 64 bit
>> +    # registers on a 32 bit machine. Also print what is missing to
>> +    # help with debug.
>> +    if total_regs != total_r_regs:
>> +        print(f"xml-tdesc has ({total_regs}) registers")
>> +        print(f"remote-registers has ({total_r_regs}) registers")
>> +
>> +        for x_key in reg_map.keys():
>> +            x_reg = reg_map[x_key]
>> +            if "seen" not in x_reg:
>> +                print(f"{x_reg} wasn't seen in remote-registers")
>> +
>> +def complete_and_diff(reg_map):
>> +    """
>> +    Let the program run to (almost) completion and then iterate
>> +    through all the registers we know about and report which ones have
>> +    changed.
>> +    """
>> +    # Let the program get to the end and we can check what changed
>> +    b = gdb.Breakpoint("_exit")
>> +    if b.pending: # workaround Microblaze weirdness
>> +        b.delete()
>> +        gdb.Breakpoint("_Exit")
>> +
>> +    gdb.execute("continue")
>> +
>> +    frame = gdb.selected_frame()
>> +    changed = 0
>> +
>> +    for e in reg_map.values():
>> +        name = e["name"]
>> +        old_val = e["initial"]
>> +
>> +        try:
>> +            new_val = frame.read_register(name)
>> +        except:
>> +            report(False, f"failed to read {name} at end of run")
>> +            continue
>> +
>> +        if new_val != old_val:
>> +            print(f"{name} changes from {old_val} to {new_val}")
>> +            changed += 1
>> +
>> +    # as long as something changed we can be confident its working
>> +    report(changed > 0, f"{changed} registers were changed")
>> +
>> +
>> +def run_test():
>> +    "Run through the tests"
>> +
>> +    reg_map = fetch_xml_regmap()
>> +
>> +    if reg_map is not None:
>> +        crosscheck_remote_xml(reg_map)
>> +        complete_and_diff(reg_map)
>> +
>> +
>> +#
>> +# This runs as the script it sourced (via -x, via run-test.py)
>> +#
>> +try:
>> +    inferior = gdb.selected_inferior()
>> +    arch = inferior.architecture()
>> +    print("ATTACHED: %s" % arch.name())
>> +except (gdb.error, AttributeError):
>> +    print("SKIPPING (not connected)", file=sys.stderr)
>> +    exit(0)
>> +
>> +if gdb.parse_and_eval('$pc') == 0:
>> +    print("SKIP: PC not set")
>> +    exit(0)
>> +
>> +try:
>> +    run_test()
>> +except (gdb.error):
>> +    print ("GDB Exception: %s" % (sys.exc_info()[0]))
>> +    failcount += 1
>> +    pass
>> +
>> +print("All tests complete: %d failures" % failcount)
>> +exit(failcount)
>> diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target b/tests/tcg/multiarch/system/Makefile.softmmu-target
>> index dee4f58dea..32dc0f9830 100644
>> --- a/tests/tcg/multiarch/system/Makefile.softmmu-target
>> +++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
>> @@ -48,9 +48,20 @@ run-gdbstub-untimely-packet: hello
>>  	$(call quiet-command, \
>>  		(! grep -Fq 'Packet instead of Ack, ignoring it' untimely-packet.gdb.err), \
>>  		"GREP", file untimely-packet.gdb.err)
>> +
>> +run-gdbstub-registers: memory
>> +	$(call run-test, $@, $(GDB_SCRIPT) \
>> +		--gdb $(GDB) \
>> +		--qemu $(QEMU) \
>> +		--output $<.registers.gdb.out \
>> +		--qargs \
>> +		"-monitor none -display none -chardev file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
>> +		--bin $< --test $(MULTIARCH_SRC)/gdbstub/registers.py, \
>> +	softmmu gdbstub support)
>>  else
>>  run-gdbstub-%:
>>  	$(call skip-test, "gdbstub test $*", "need working gdb with $(patsubst -%,,$(TARGET_NAME)) support")
>>  endif
>>  
>> -MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-interrupt run-gdbstub-untimely-packet
>> +MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-interrupt \
>> +	run-gdbstub-untimely-packet run-gdbstub-registers
>> diff --git a/tests/tcg/ppc64/Makefile.target b/tests/tcg/ppc64/Makefile.target
>> index 5721c159f2..1d08076756 100644
>> --- a/tests/tcg/ppc64/Makefile.target
>> +++ b/tests/tcg/ppc64/Makefile.target
>> @@ -38,4 +38,11 @@ PPC64_TESTS += signal_save_restore_xer
>>  PPC64_TESTS += xxspltw
>>  PPC64_TESTS += test-aes
>>  
>> +ifneq ($(GDB),)
>> +# Skip for now until vsx registers sorted out
>> +run-gdbstub-registers:
>> +	$(call skip-test, $<, "BROKEN reading VSX registers")
>> +endif
>> +
>> +
>>  TESTS += $(PPC64_TESTS)
>> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
>> index 826f0a18e4..49af091c38 100644
>> --- a/tests/tcg/s390x/Makefile.target
>> +++ b/tests/tcg/s390x/Makefile.target
>> @@ -100,6 +100,10 @@ run-gdbstub-svc: hello-s390x-asm
>>  		--bin $< --test $(S390X_SRC)/gdbstub/test-svc.py, \
>>  	single-stepping svc)
>>  
>> +# Skip for now until vx registers sorted out
>> +run-gdbstub-registers:
>> +	$(call skip-test, $<, "BROKEN reading VX registers")
>> +
>>  EXTRA_RUNS += run-gdbstub-signals-s390x run-gdbstub-svc
>>  endif
>>

On 11/15/23 20:56, Alex Bennée via Gdb wrote:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>
>> On Wed Nov 8, 2023 at 12:23 AM AEST, 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.
>>>
>>> We also skip the tests for the following arches for now until we can
>>> investigate and fix any issues:
>>>
>>>   - s390x (fails to read v0l->v15l, not seen in remote-registers)
>>>   - ppc64 (fails to read vs0h->vs31h, not seen in remote-registers)
>>
>> binutils-gdb.git/gdb/rs6000-tdep.c has:
>>
>> static const char *
>> rs6000_register_name (struct gdbarch *gdbarch, int regno)
>> {
>>   ppc_gdbarch_tdep *tdep = (ppc_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>>
>>   /* The upper half "registers" have names in the XML description,
>>      but we present only the low GPRs and the full 64-bit registers
>>      to the user.  */
>>   if (tdep->ppc_ev0_upper_regnum >= 0
>>       && tdep->ppc_ev0_upper_regnum <= regno
>>       && regno < tdep->ppc_ev0_upper_regnum + ppc_num_gprs)
>>     return "";
>>
>>   /* Hide the upper halves of the vs0~vs31 registers.  */
>>   if (tdep->ppc_vsr0_regnum >= 0
>>       && tdep->ppc_vsr0_upper_regnum <= regno
>>       && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_gprs)
>>     return "";
>>
>> (s390 looks similar for V0-V15 lower).
>>
>> I guess it is because the upper half is not a real register but an
>> extension of an existing FP register to make a vector register. I
>> just don't know how that should be resolved with QEMU.
>>
>> Should we put an exception in the test case for these? Or is there
>> something we should be doing differently with the XML regs?
>
> Yeah I suspect this is just inconsistency between targets on gdb. My
> naive assumption was XML should match the displayed registers but it
> seems there is additional filtering going on.
>
> It seems in this case the registers are still there and have regnums (so
> I assume the stub could be asked for them) but the names have been
> squashed. I guess we could detect that and accept it?
>
>>
>> i386 gdb does similar:
>>
>> static const char *
>> i386_register_name (struct gdbarch *gdbarch, int regnum)
>> {
>>   /* Hide the upper YMM registers.  */
>>   if (i386_ymmh_regnum_p (gdbarch, regnum))
>>     return "";
>>
>>   /* Hide the upper YMM16-31 registers.  */
>>   if (i386_ymmh_avx512_regnum_p (gdbarch, regnum))
>>     return "";
>>
>>   /* Hide the upper ZMM registers.  */
>>   if (i386_zmmh_regnum_p (gdbarch, regnum))
>>     return "";
>>
>>   return tdesc_register_name (gdbarch, regnum);
>> }
>>
>> So, I'm not sure how they don't fail this test. Does QEMU just
>> not have YMM/ZMM in XML regmap?
>
> No I think we only send the core one with XMM regs and there are no
> additional registers sent via gdb_register_coprocessor.
>
>>
>> Thanks,
>> Nick

FTR, luis.machado@linaro.org doesn't exist anymore.

As for the XML, it serves as an architecture hint/description of what features and registers
are available.

GDB will process that and will potentially include additional pseudo-registers (so QEMU doesn't
need to do so, unless it is some pseudo-register not accounted by gdb).

The rest of the features/registers gdb doesn't care about, it will just add them to the end of the
list, and will assign whatever number is next. GDB will be able to read/write them, but nothing more
than that.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Luis Machado <luis.machado@arm.com> writes:

> On 11/15/23 20:56, Alex Bennée via Gdb wrote:
>> "Nicholas Piggin" <npiggin@gmail.com> writes:
>>
>>> On Wed Nov 8, 2023 at 12:23 AM AEST, 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.
>>>>
>>>> We also skip the tests for the following arches for now until we can
>>>> investigate and fix any issues:
>>>>
>>>>   - s390x (fails to read v0l->v15l, not seen in remote-registers)
>>>>   - ppc64 (fails to read vs0h->vs31h, not seen in remote-registers)
>>>
>>> binutils-gdb.git/gdb/rs6000-tdep.c has:
>>>
>>> static const char *
>>> rs6000_register_name (struct gdbarch *gdbarch, int regno)
>>> {
>>>   ppc_gdbarch_tdep *tdep = (ppc_gdbarch_tdep *) gdbarch_tdep (gdbarch);
>>>
>>>   /* The upper half "registers" have names in the XML description,
>>>      but we present only the low GPRs and the full 64-bit registers
>>>      to the user.  */
>>>   if (tdep->ppc_ev0_upper_regnum >= 0
>>>       && tdep->ppc_ev0_upper_regnum <= regno
>>>       && regno < tdep->ppc_ev0_upper_regnum + ppc_num_gprs)
>>>     return "";
>>>
>>>   /* Hide the upper halves of the vs0~vs31 registers.  */
>>>   if (tdep->ppc_vsr0_regnum >= 0
>>>       && tdep->ppc_vsr0_upper_regnum <= regno
>>>       && regno < tdep->ppc_vsr0_upper_regnum + ppc_num_gprs)
>>>     return "";
>>>
>>> (s390 looks similar for V0-V15 lower).
>>>
>>> I guess it is because the upper half is not a real register but an
>>> extension of an existing FP register to make a vector register. I
>>> just don't know how that should be resolved with QEMU.
>>>
>>> Should we put an exception in the test case for these? Or is there
>>> something we should be doing differently with the XML regs?
>>
>> Yeah I suspect this is just inconsistency between targets on gdb. My
>> naive assumption was XML should match the displayed registers but it
>> seems there is additional filtering going on.
>>
>> It seems in this case the registers are still there and have regnums (so
>> I assume the stub could be asked for them) but the names have been
>> squashed. I guess we could detect that and accept it?
>>
>>>
>>> i386 gdb does similar:
>>>
>>> static const char *
>>> i386_register_name (struct gdbarch *gdbarch, int regnum)
>>> {
>>>   /* Hide the upper YMM registers.  */
>>>   if (i386_ymmh_regnum_p (gdbarch, regnum))
>>>     return "";
>>>
>>>   /* Hide the upper YMM16-31 registers.  */
>>>   if (i386_ymmh_avx512_regnum_p (gdbarch, regnum))
>>>     return "";
>>>
>>>   /* Hide the upper ZMM registers.  */
>>>   if (i386_zmmh_regnum_p (gdbarch, regnum))
>>>     return "";
>>>
>>>   return tdesc_register_name (gdbarch, regnum);
>>> }
>>>
>>> So, I'm not sure how they don't fail this test. Does QEMU just
>>> not have YMM/ZMM in XML regmap?
>>
>> No I think we only send the core one with XMM regs and there are no
>> additional registers sent via gdb_register_coprocessor.
>>
>>>
>>> Thanks,
>>> Nick
>
> FTR, luis.machado@linaro.org doesn't exist anymore.
>
> As for the XML, it serves as an architecture hint/description of what features and registers
> are available.
>
> GDB will process that and will potentially include additional pseudo-registers (so QEMU doesn't
> need to do so, unless it is some pseudo-register not accounted by gdb).
>
> The rest of the features/registers gdb doesn't care about, it will just add them to the end of the
> list, and will assign whatever number is next. GDB will be able to read/write them, but nothing more
> than that.

So with a bit of fiddling I can do:

modified   tests/tcg/multiarch/gdbstub/registers.py
@@ -44,7 +44,6 @@ def fetch_xml_regmap():
 
     total_regs = 0
     reg_map = {}
-    frame = gdb.selected_frame()
 
     tree = ET.fromstring(xml)
     for f in tree.findall("feature"):
@@ -61,12 +60,8 @@ def fetch_xml_regmap():
         for r in regs:
             name = r.attrib["name"]
             regnum = int(r.attrib["regnum"])
-            try:
-                value = frame.read_register(name)
-            except ValueError:
-                report(False, f"failed to read reg: {name}")
 
-            entry = { "name": name, "initial": value, "regnum": regnum }
+            entry = { "name": name, "regnum": regnum }
 
             if name in reg_map:
                 report(False, f"duplicate register {entry} vs {reg_map[name]}")
@@ -80,6 +75,15 @@ def fetch_xml_regmap():
 
     return reg_map
 
+def get_register_by_regnum(reg_map, regnum):
+    """
+    Helper to find a register from the map via its XML regnum
+    """
+    for regname, entry in reg_map.items():
+        if entry['regnum'] == regnum:
+            return entry
+    return None
+
 def crosscheck_remote_xml(reg_map):
     """
     Cross-check the list of remote-registers with the XML info.
@@ -90,6 +94,7 @@ def crosscheck_remote_xml(reg_map):
 
     total_regs = len(reg_map.keys())
     total_r_regs = 0
+    total_r_elided_regs = 0
 
     for r in r_regs:
         fields = r.split()
@@ -100,6 +105,15 @@ def crosscheck_remote_xml(reg_map):
             r_name = fields[0]
             r_regnum = int(fields[6])
 
+            # Some registers are "hidden" so don't have a name
+            # although they still should have a register number
+            if r_name == "''":
+                total_r_elided_regs += 1
+                x_reg = get_register_by_regnum(reg_map, r_regnum)
+                if x_reg is not None:
+                    x_reg["hidden"] = True
+                continue
+
             # check in the XML
             try:
                 x_reg = reg_map[r_name]
@@ -118,13 +132,38 @@ def crosscheck_remote_xml(reg_map):
     # registers on a 32 bit machine. Also print what is missing to
     # help with debug.
     if total_regs != total_r_regs:
-        print(f"xml-tdesc has ({total_regs}) registers")
-        print(f"remote-registers has ({total_r_regs}) registers")
+        print(f"xml-tdesc has {total_regs} registers")
+        print(f"remote-registers has {total_r_regs} registers")
+        print(f"of which {total_r_elided_regs} are hidden")
 
         for x_key in reg_map.keys():
             x_reg = reg_map[x_key]
-            if "seen" not in x_reg:
-                print(f"{x_reg} wasn't seen in remote-registers")
+            if "hidden" in x_reg:
+                print(f"{x_reg} elided by gdb")
+            elif "seen" not in x_reg:
+                report(False, f"{x_reg} wasn't seen in remote-registers")
+
+def initial_register_read(reg_map):
+    """
+    Do an initial read of all registers that we know gdb cares about
+    (so ignore the elided ones).
+    """
+    frame = gdb.selected_frame()
+
+    for e in reg_map.values():
+        name = e["name"]
+        regnum = e["regnum"]
+
+        try:
+            if "hidden" in e:
+                value = frame.read_register(regnum)
+            else:
+                value = frame.read_register(name)
+
+            e["initial"] = value
+        except ValueError:
+                report(False, f"failed to read reg: {name}")
+
 
 def complete_and_diff(reg_map):
     """
@@ -144,18 +183,19 @@ def complete_and_diff(reg_map):
     changed = 0
 
     for e in reg_map.values():
-        name = e["name"]
-        old_val = e["initial"]
+        if "hidden" not in e:
+            name = e["name"]
+            old_val = e["initial"]
 
-        try:
-            new_val = frame.read_register(name)
-        except:
-            report(False, f"failed to read {name} at end of run")
-            continue
+            try:
+                new_val = frame.read_register(name)
+            except ValueError:
+                report(False, f"failed to read {name} at end of run")
+                continue
 
-        if new_val != old_val:
-            print(f"{name} changes from {old_val} to {new_val}")
-            changed += 1
+            if new_val != old_val:
+                print(f"{name} changes from {old_val} to {new_val}")
+                changed += 1
 
     # as long as something changed we can be confident its working
     report(changed > 0, f"{changed} registers were changed")
@@ -168,6 +208,7 @@ def run_test():
 
     if reg_map is not None:
         crosscheck_remote_xml(reg_map)
+        initial_register_read(reg_map)
         complete_and_diff(reg_map)

I'll wrap that into my next set of patches.