Re: [PULL 21/30] target/arm: use official org.gnu.gdb.aarch64.sve layout for registers

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 1/19/21 3:50 PM, Alex Bennée wrote:
>> 
>> Claudio Fontana <cfontana@suse.de> writes:
>> 
>>> Hi Alex,
>>>
>>> after updating to latest master today, I am getting the following error with
>>>
>>> make check-tcg
>>>
>>> qemu-system-aarch64: -gdb 
>>> unix:path=/tmp/tmp9ru5tgk8qemu-gdbstub/gdbstub.socket,server: info: QEMU 
>>> waiting for connection on: 
>>> disconnected:unix:/tmp/tmp9ru5tgk8qemu-gdbstub/gdbstub.socket,server
>>> warning: while parsing target description (at line 47): Vector "svevhf" 
>>> references undefined type "ieee_half"
>>> warning: Could not load XML target description; ignoring
>>> qemu-system-aarch64: QEMU: Terminated via GDBstub
>>>
>>> Seems to indicate it is "ieee_half" -related?
>> 
>> *sigh*
>> 
>> yes - it is. I thought this was solved by the GDB version check in
>> 14/30. What does your gdb report?
>
>
> $ gdb --version
> GNU gdb (GDB; openSUSE Leap 15.2) 8.3.1
> Copyright (C) 2019 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> gdb --configuration
> This GDB was configured as follows:
>    configure --host=x86_64-suse-linux --target=x86_64-suse-linux
>              --with-auto-load-dir=$debugdir:$datadir/auto-load
>              --with-auto-load-safe-path=$debugdir:$datadir/auto-load
>              --with-expat
>              --with-gdb-datadir=/usr/share/gdb
>              --with-jit-reader-dir=/usr/lib64/gdb
>              --without-libunwind-ia64
>              --with-lzma
>              --without-babeltrace
>              --with-intel-pt
>              --disable-libmcheck
>              --with-mpfr
>              --with-python=/usr
>              --without-guile
>              --disable-source-highlight
>              --with-separate-debug-dir=/usr/lib/debug
>              --with-system-gdbinit=/etc/gdbinit
>
>
> does this help?

So it looks like TDESC_TYPE_IEEE_HALF was only implemented in GDB 9.1
and there is no probing possible during the gdbstub connection. I guess
I can either go back to stubbing it out (which would break gdb's SVE
understanding) or up our minimum GDB version check for running tests.
That would mean less people test GDB (or at least until the distros
catch up) but considering it was zero people not too long ago maybe
that's acceptable?

>
> Let me know if more info is needed. Thanks!
>
> Claudio
>
>
>> 
>>>
>>> Thanks,
>>>
>>> Claudio
>>>
>>> On 1/15/21 2:08 PM, Alex Bennée wrote:
>>>> While GDB can work with any XML description given to it there is
>>>> special handling for SVE registers on the GDB side which makes the
>>>> users life a little better. The changes aren't that major and all the
>>>> registers save the $vg reported the same. All that changes is:
>>>>
>>>>   - report org.gnu.gdb.aarch64.sve
>>>>   - use gdb nomenclature for names and types
>>>>   - minor re-ordering of the types to match reference
>>>>   - re-enable ieee_half (as we know gdb supports it now)
>>>>   - $vg is now a 64 bit int
>>>>   - check $vN and $zN aliasing in test
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Reviewed-by: Luis Machado <luis.machado@linaro.org>
>>>> Message-Id: <20210108224256.2321-11-alex.bennee@linaro.org>
>>>>
>>>> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
>>>> index 866595b4f1..a8fff2a3d0 100644
>>>> --- a/target/arm/gdbstub.c
>>>> +++ b/target/arm/gdbstub.c
>>>> @@ -195,22 +195,17 @@ static const struct TypeSize vec_lanes[] = {
>>>>      { "uint128", 128, 'q', 'u' },
>>>>      { "int128", 128, 'q', 's' },
>>>>      /* 64 bit */
>>>> +    { "ieee_double", 64, 'd', 'f' },
>>>>      { "uint64", 64, 'd', 'u' },
>>>>      { "int64", 64, 'd', 's' },
>>>> -    { "ieee_double", 64, 'd', 'f' },
>>>>      /* 32 bit */
>>>> +    { "ieee_single", 32, 's', 'f' },
>>>>      { "uint32", 32, 's', 'u' },
>>>>      { "int32", 32, 's', 's' },
>>>> -    { "ieee_single", 32, 's', 'f' },
>>>>      /* 16 bit */
>>>> +    { "ieee_half", 16, 'h', 'f' },
>>>>      { "uint16", 16, 'h', 'u' },
>>>>      { "int16", 16, 'h', 's' },
>>>> -    /*
>>>> -     * TODO: currently there is no reliable way of telling
>>>> -     * if the remote gdb actually understands ieee_half so
>>>> -     * we don't expose it in the target description for now.
>>>> -     * { "ieee_half", 16, 'h', 'f' },
>>>> -     */
>>>>      /* bytes */
>>>>      { "uint8", 8, 'b', 'u' },
>>>>      { "int8", 8, 'b', 's' },
>>>> @@ -223,17 +218,16 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int 
>>>> base_reg)
>>>>      GString *s = g_string_new(NULL);
>>>>      DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
>>>>      g_autoptr(GString) ts = g_string_new("");
>>>> -    int i, bits, reg_width = (cpu->sve_max_vq * 128);
>>>> +    int i, j, bits, reg_width = (cpu->sve_max_vq * 128);
>>>>      info->num = 0;
>>>>      g_string_printf(s, "<?xml version=\"1.0\"?>");
>>>>      g_string_append_printf(s, "<!DOCTYPE target SYSTEM 
>>>> \"gdb-target.dtd\">");
>>>> -    g_string_append_printf(s, "<feature 
>>>> name=\"org.qemu.gdb.aarch64.sve\">");
>>>> +    g_string_append_printf(s, "<feature 
>>>> name=\"org.gnu.gdb.aarch64.sve\">");
>>>>  
>>>>      /* First define types and totals in a whole VL */
>>>>      for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
>>>>          int count = reg_width / vec_lanes[i].size;
>>>> -        g_string_printf(ts, "vq%d%c%c", count,
>>>> -                        vec_lanes[i].sz, vec_lanes[i].suffix);
>>>> +        g_string_printf(ts, "svev%c%c", vec_lanes[i].sz, 
>>>> vec_lanes[i].suffix);
>>>>          g_string_append_printf(s,
>>>>                                 "<vector id=\"%s\" type=\"%s\" 
>>>> count=\"%d\"/>",
>>>>                                 ts->str, vec_lanes[i].gdb_type, count);
>>>> @@ -243,39 +237,37 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int 
>>>> base_reg)
>>>>       * signed and potentially float versions of each size from 128 to
>>>>       * 8 bits.
>>>>       */
>>>> -    for (bits = 128; bits >= 8; bits /= 2) {
>>>> -        int count = reg_width / bits;
>>>> -        g_string_append_printf(s, "<union id=\"vq%dn\">", count);
>>>> -        for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
>>>> -            if (vec_lanes[i].size == bits) {
>>>> -                g_string_append_printf(s, "<field name=\"%c\" 
>>>> type=\"vq%d%c%c\"/>",
>>>> -                                       vec_lanes[i].suffix,
>>>> -                                       count,
>>>> -                                       vec_lanes[i].sz, 
>>>> vec_lanes[i].suffix);
>>>> +    for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
>>>> +        const char suf[] = { 'q', 'd', 's', 'h', 'b' };
>>>> +        g_string_append_printf(s, "<union id=\"svevn%c\">", suf[i]);
>>>> +        for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
>>>> +            if (vec_lanes[j].size == bits) {
>>>> +                g_string_append_printf(s, "<field name=\"%c\" 
>>>> type=\"svev%c%c\"/>",
>>>> +                                       vec_lanes[j].suffix,
>>>> +                                       vec_lanes[j].sz, 
>>>> vec_lanes[j].suffix);
>>>>              }
>>>>          }
>>>>          g_string_append(s, "</union>");
>>>>      }
>>>>      /* And now the final union of unions */
>>>> -    g_string_append(s, "<union id=\"vq\">");
>>>> -    for (bits = 128; bits >= 8; bits /= 2) {
>>>> -        int count = reg_width / bits;
>>>> -        for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
>>>> -            if (vec_lanes[i].size == bits) {
>>>> -                g_string_append_printf(s, "<field name=\"%c\" 
>>>> type=\"vq%dn\"/>",
>>>> -                                       vec_lanes[i].sz, count);
>>>> -                break;
>>>> -            }
>>>> -        }
>>>> +    g_string_append(s, "<union id=\"svev\">");
>>>> +    for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
>>>> +        const char suf[] = { 'q', 'd', 's', 'h', 'b' };
>>>> +        g_string_append_printf(s, "<field name=\"%c\" type=\"svevn%c\"/>",
>>>> +                               suf[i], suf[i]);
>>>>      }
>>>>      g_string_append(s, "</union>");
>>>>  
>>>> +    /* Finally the sve prefix type */
>>>> +    g_string_append_printf(s,
>>>> +                           "<vector id=\"svep\" type=\"uint8\" 
>>>> count=\"%d\"/>",
>>>> +                           reg_width / 8);
>>>> +
>>>>      /* Then define each register in parts for each vq */
>>>>      for (i = 0; i < 32; i++) {
>>>>          g_string_append_printf(s,
>>>>                                 "<reg name=\"z%d\" bitsize=\"%d\""
>>>> -                               " regnum=\"%d\" group=\"vector\""
>>>> -                               " type=\"vq\"/>",
>>>> +                               " regnum=\"%d\" type=\"svev\"/>",
>>>>                                 i, reg_width, base_reg++);
>>>>          info->num++;
>>>>      }
>>>> @@ -287,31 +279,22 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int 
>>>> base_reg)
>>>>                             " regnum=\"%d\" group=\"float\""
>>>>                             " type=\"int\"/>", base_reg++);
>>>>      info->num += 2;
>>>> -    /*
>>>> -     * Predicate registers aren't so big they are worth splitting up
>>>> -     * but we do need to define a type to hold the array of quad
>>>> -     * references.
>>>> -     */
>>>> -    g_string_append_printf(s,
>>>> -                           "<vector id=\"vqp\" type=\"uint16\" 
>>>> count=\"%d\"/>",
>>>> -                           cpu->sve_max_vq);
>>>> +
>>>>      for (i = 0; i < 16; i++) {
>>>>          g_string_append_printf(s,
>>>>                                 "<reg name=\"p%d\" bitsize=\"%d\""
>>>> -                               " regnum=\"%d\" group=\"vector\""
>>>> -                               " type=\"vqp\"/>",
>>>> +                               " regnum=\"%d\" type=\"svep\"/>",
>>>>                                 i, cpu->sve_max_vq * 16, base_reg++);
>>>>          info->num++;
>>>>      }
>>>>      g_string_append_printf(s,
>>>>                             "<reg name=\"ffr\" bitsize=\"%d\""
>>>>                             " regnum=\"%d\" group=\"vector\""
>>>> -                           " type=\"vqp\"/>",
>>>> +                           " type=\"svep\"/>",
>>>>                             cpu->sve_max_vq * 16, base_reg++);
>>>>      g_string_append_printf(s,
>>>>                             "<reg name=\"vg\" bitsize=\"64\""
>>>> -                           " regnum=\"%d\" group=\"vector\""
>>>> -                           " type=\"uint32\"/>",
>>>> +                           " regnum=\"%d\" type=\"int\"/>",
>>>>                             base_reg++);
>>>>      info->num += 2;
>>>>      g_string_append_printf(s, "</feature>");
>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>> index 5ab3f5ace3..8a492465d6 100644
>>>> --- a/target/arm/helper.c
>>>> +++ b/target/arm/helper.c
>>>> @@ -276,7 +276,7 @@ static int arm_gdb_get_svereg(CPUARMState *env, 
>>>> GByteArray *buf, int reg)
>>>>           * while the ZCR works in Vector Quads (VQ) which is 128bit 
>>>> chunks.
>>>>           */
>>>>          int vq = sve_zcr_len_for_el(env, arm_current_el(env)) + 1;
>>>> -        return gdb_get_reg32(buf, vq * 2);
>>>> +        return gdb_get_reg64(buf, vq * 2);
>>>>      }
>>>>      default:
>>>>          /* gdbstub asked for something out our range */
>>>> diff --git a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py 
>>>> b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
>>>> index 972cf73c31..b9ef169c1a 100644
>>>> --- a/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
>>>> +++ b/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py
>>>> @@ -40,6 +40,17 @@ class TestBreakpoint(gdb.Breakpoint):
>>>>          except gdb.error:
>>>>              report(False, "checking zregs (out of range)")
>>>>  
>>>> +        # Check the aliased V registers are set and GDB has correctly
>>>> +        # created them for us having recognised and handled SVE.
>>>> +        try:
>>>> +            for i in range(0, 16):
>>>> +                val_z = gdb.parse_and_eval("$z0.b.u[%d]" % i)
>>>> +                val_v = gdb.parse_and_eval("$v0.b.u[%d]" % i)
>>>> +                report(int(val_z) == int(val_v),
>>>> +                       "v0.b.u[%d] == z0.b.u[%d]" % (i, i))
>>>> +        except gdb.error:
>>>> +            report(False, "checking vregs (out of range)")
>>>> +
>>>>  
>>>>  def run_test():
>>>>      "Run through the tests one by one"
>>>>
>> 
>> 


-- 
Alex Bennée