Question on the gdb.xml/tdesc-regs.exp test

Question on the gdb.xml/tdesc-regs.exp test

[Carl Love]

Stafford:

In your patch for the test on Dec 27, 2017, you updated the comment in
the test to say:

+# Reggroups should have at least general and the extra foo group               
+gdb_test "maintenance print reggroups" \                                       
+    " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[ \t\]+\r\n.* foo\[ \
\t\]+user\[ \t\]+"

On Powerpc, the test results are as follows:

maintenance print reggroups
 Group      Type      
 foo        user      
(gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups
set tdesc filename test-regs.xml

I see that the output on X86 includes an line with general:

 maintenance print reggroups
 Group      Type      
 sse        user      
 mmx        user      
 general    user      
...

I am wondering if you can help me understand why a line with general is
expected?  I have been looking at Powerpc and Intel trying to figure
out why it shows up on Intel but not Powerpc?  Anyway, it would be good
to understand what results in the general line being generated so I can
figure out how to get the test to run properly on Powerpc.  Currently,
this is the only failure on Powerpc.   Thanks for your help.

                       Carl Love

Re: Question on the gdb.xml/tdesc-regs.exp test

[Andrew Burgess]

Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:

> Stafford:
>
> In your patch for the test on Dec 27, 2017, you updated the comment in
> the test to say:
>
> +# Reggroups should have at least general and the extra foo group               
> +gdb_test "maintenance print reggroups" \                                       
> +    " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[ \t\]+\r\n.* foo\[ \
> \t\]+user\[ \t\]+"
>
> On Powerpc, the test results are as follows:
>
> maintenance print reggroups
>  Group      Type      
>  foo        user      
> (gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups
> set tdesc filename test-regs.xml
>
> I see that the output on X86 includes an line with general:
>
>  maintenance print reggroups
>  Group      Type      
>  sse        user      
>  mmx        user      
>  general    user      
> ...
>
> I am wondering if you can help me understand why a line with general is
> expected?  I have been looking at Powerpc and Intel trying to figure
> out why it shows up on Intel but not Powerpc?  Anyway, it would be good
> to understand what results in the general line being generated so I can
> figure out how to get the test to run properly on Powerpc.  Currently,
> this is the only failure on Powerpc.   Thanks for your help.

The 'general' set usually includes the core integer registers for an
architecture, so on ppc this would be r0 - r31.  Different architectures
might add some additional registers to this set, maybe $pc if it's
separate, or some flag registers... but the integer regs are the main
ones.

So, if I fire up GDB on ppc and do:

  maint print register-groups

I can see that r0 - r31 are indeed in the general group.

But, if I load the xml description used in the test, I can see that the
r0-r31 registers are still present, but they are no longer in the
general set.  This is the problem that needs solving.

I've not (yet) looked into why this might be, but thought the above
might be a good starting point if you wanted to look at this yourself.

Thanks,
Andrew

Re: Question on the gdb.xml/tdesc-regs.exp test

[Andrew Burgess]

Andrew Burgess <aburgess@redhat.com> writes:

> Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> Stafford:
>>
>> In your patch for the test on Dec 27, 2017, you updated the comment in
>> the test to say:
>>
>> +# Reggroups should have at least general and the extra foo group               
>> +gdb_test "maintenance print reggroups" \                                       
>> +    " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[ \t\]+\r\n.* foo\[ \
>> \t\]+user\[ \t\]+"
>>
>> On Powerpc, the test results are as follows:
>>
>> maintenance print reggroups
>>  Group      Type      
>>  foo        user      
>> (gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups
>> set tdesc filename test-regs.xml
>>
>> I see that the output on X86 includes an line with general:
>>
>>  maintenance print reggroups
>>  Group      Type      
>>  sse        user      
>>  mmx        user      
>>  general    user      
>> ...
>>
>> I am wondering if you can help me understand why a line with general is
>> expected?  I have been looking at Powerpc and Intel trying to figure
>> out why it shows up on Intel but not Powerpc?  Anyway, it would be good
>> to understand what results in the general line being generated so I can
>> figure out how to get the test to run properly on Powerpc.  Currently,
>> this is the only failure on Powerpc.   Thanks for your help.
>
> The 'general' set usually includes the core integer registers for an
> architecture, so on ppc this would be r0 - r31.  Different architectures
> might add some additional registers to this set, maybe $pc if it's
> separate, or some flag registers... but the integer regs are the main
> ones.
>
> So, if I fire up GDB on ppc and do:
>
>   maint print register-groups
>
> I can see that r0 - r31 are indeed in the general group.
>
> But, if I load the xml description used in the test, I can see that the
> r0-r31 registers are still present, but they are no longer in the
> general set.  This is the problem that needs solving.
>
> I've not (yet) looked into why this might be, but thought the above
> might be a good starting point if you wanted to look at this yourself.

OK, so what happens is this.

In "normal" native operation, the target descriptions GDB uses don't
include any register groups, so reggroup_add is never called.

As a result, when we call reggroup_next to iterate over the register
groups we end up using the "default" set of register groups, this
default list includes the general purpose set.

Then, in the default_register_reggroup_p function, which decides if a
register is in a group or not (which ppc uses), and non-vector,
non-float register is declared in the general set.

Now, in the test, we do add a register group to the target description,
the group 'foo'.  This causes reggroup_add to be called, which starts a
new register group list within the gdbarch.

Now when GDB calls reggroup_next we find we do have a list of reggroups,
so we use that instead of the default list.  Unfortunately, for ppc,
this means we don't include the general set.

My honest opinion is that the whole "default register groups" mechanism
in GDB is not really up to scratch and needs to be rewritten.  But you
probably don't care about that.

What most architectures do to solve this problem is add all of the
default register groups from within their own *-tdep.c file.  For
example the function i386_add_reggroups, aarch64_add_reggroups, or
riscv_add_reggroups, etc.

If you look at riscv_add_reggroups you'll see the registers are split
into two sets, the 7 default groups, and then 1 RISC-V specific set.  I
think ppc just needs a new rs6000_add_reggroups function that adds the
same 7 default groups.

The patch below does just this.  Other than running the
gdb.xml/tdesc-regs.exp test, I've done no testing on this yet, but do
let me know your thoughts.

Thanks,
Andrew

---

commit 9a7f4d69cb5e2d7ecf19cd38000a7aef66607794
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Mar 30 06:54:15 2022 -0400

    gdb/rs6000: ensure default register groups are always present
    
    If the target description includes any custom register groups then the
    default register groups will not be included for rs6000.  Solve this
    by ensuring that all the default register groups are always present.
    This is identical to how architectures like i386, aarch64, and riscv
    all solve this problem.
    
    After this commit the test gdb.xml/tdesc-regs.exp should pass on ppc,
    while it fails before this commit.

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index c246ce85..ee603bc6 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -6310,6 +6310,21 @@ rs6000_program_breakpoint_here_p (gdbarch *gdbarch, CORE_ADDR address)
   return false;
 }
 
+/* Add all the expected register sets into GDBARCH.  */
+
+static void
+rs6000_add_reggroups (struct gdbarch *gdbarch)
+{
+  /* Add predefined register groups.  */
+  reggroup_add (gdbarch, all_reggroup);
+  reggroup_add (gdbarch, save_reggroup);
+  reggroup_add (gdbarch, restore_reggroup);
+  reggroup_add (gdbarch, system_reggroup);
+  reggroup_add (gdbarch, vector_reggroup);
+  reggroup_add (gdbarch, general_reggroup);
+  reggroup_add (gdbarch, float_reggroup);
+}
+
 /* Initialize the current architecture based on INFO.  If possible, re-use an
    architecture from ARCHES, which is a list of architectures already created
    during this debugging session.
@@ -7235,6 +7250,8 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       frame_base_append_sniffer (gdbarch, rs6000_frame_base_sniffer);
     }
 
+  rs6000_add_reggroups (gdbarch);
+
   set_tdesc_pseudo_register_type (gdbarch, rs6000_pseudo_register_type);
   set_tdesc_pseudo_register_reggroup_p (gdbarch,
 					rs6000_pseudo_register_reggroup_p);

Re: Question on the gdb.xml/tdesc-regs.exp test

[will schmidt]

On Wed, 2022-03-30 at 11:58 +0100, Andrew Burgess via Gdb-patches
wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
> > Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> > 
> > > Stafford:
> > > 
> > > In your patch for the test on Dec 27, 2017, you updated the
> > > comment in
> > > the test to say:
> > > 
> > > +# Reggroups should have at least general and the extra foo
> > > group               
> > > +gdb_test "maintenance print reggroups"
> > > \                                       
> > > +    " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[
> > > \t\]+\r\n.* foo\[ \
> > > \t\]+user\[ \t\]+"
> > > 
> > > On Powerpc, the test results are as follows:
> > > 
> > > maintenance print reggroups
> > >  Group      Type      
> > >  foo        user      
> > > (gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups
> > > set tdesc filename test-regs.xml
> > > 
> > > I see that the output on X86 includes an line with general:
> > > 
> > >  maintenance print reggroups
> > >  Group      Type      
> > >  sse        user      
> > >  mmx        user      
> > >  general    user      
> > > ...
> > > 
> > > I am wondering if you can help me understand why a line with
> > > general is
> > > expected?  I have been looking at Powerpc and Intel trying to
> > > figure
> > > out why it shows up on Intel but not Powerpc?  Anyway, it would
> > > be good
> > > to understand what results in the general line being generated so
> > > I can
> > > figure out how to get the test to run properly on
> > > Powerpc.  Currently,
> > > this is the only failure on Powerpc.   Thanks for your help.
> > 
> > The 'general' set usually includes the core integer registers for
> > an
> > architecture, so on ppc this would be r0 - r31.  Different
> > architectures
> > might add some additional registers to this set, maybe $pc if it's
> > separate, or some flag registers... but the integer regs are the
> > main
> > ones.
> > 
> > So, if I fire up GDB on ppc and do:
> > 
> >   maint print register-groups
> > 
> > I can see that r0 - r31 are indeed in the general group.
> > 
> > But, if I load the xml description used in the test, I can see that
> > the
> > r0-r31 registers are still present, but they are no longer in the
> > general set.  This is the problem that needs solving.
> > 
> > I've not (yet) looked into why this might be, but thought the above
> > might be a good starting point if you wanted to look at this
> > yourself.
> 
> OK, so what happens is this.
> 
> In "normal" native operation, the target descriptions GDB uses don't
> include any register groups, so reggroup_add is never called.
> 
> As a result, when we call reggroup_next to iterate over the register
> groups we end up using the "default" set of register groups, this
> default list includes the general purpose set.
> 
> Then, in the default_register_reggroup_p function, which decides if a
> register is in a group or not (which ppc uses), and non-vector,
> non-float register is declared in the general set.
> 
> Now, in the test, we do add a register group to the target
> description,
> the group 'foo'.  This causes reggroup_add to be called, which starts
> a
> new register group list within the gdbarch.
> 
> Now when GDB calls reggroup_next we find we do have a list of
> reggroups,
> so we use that instead of the default list.  Unfortunately, for ppc,
> this means we don't include the general set.
> 
> My honest opinion is that the whole "default register groups"
> mechanism
> in GDB is not really up to scratch and needs to be rewritten.  But
> you
> probably don't care about that.
> 
> What most architectures do to solve this problem is add all of the
> default register groups from within their own *-tdep.c file.  For
> example the function i386_add_reggroups, aarch64_add_reggroups, or
> riscv_add_reggroups, etc.
> 
> If you look at riscv_add_reggroups you'll see the registers are split
> into two sets, the 7 default groups, and then 1 RISC-V specific
> set.  I
> think ppc just needs a new rs6000_add_reggroups function that adds
> the
> same 7 default groups.
> 
> The patch below does just this.  Other than running the
> gdb.xml/tdesc-regs.exp test, I've done no testing on this yet, but do
> let me know your thoughts.

Hi Carl, Andrew, all, 
   Rogerio should be on copy for these.

We (powerpc/rs6000 have an
internal todo to rework the rs6000 code to fully leverage the XML
target descriptions.  Rogerio has a set of patches / work in progress
for this effort. 
The tdesc-regs.exp test is one of those test failures
that gets addressed with that patch set, though there is not a firm ETA
for the patch set to be upstreamed at this time.

The proposed patch looks reasonable to me.  :-)
Thanks,
-Will

> 
> Thanks,
> Andrew
> 
> ---
> 
> commit 9a7f4d69cb5e2d7ecf19cd38000a7aef66607794
> Author: Andrew Burgess <aburgess@redhat.com>
> Date:   Wed Mar 30 06:54:15 2022 -0400
> 
>     gdb/rs6000: ensure default register groups are always present
> 
>     If the target description includes any custom register groups
> then the
>     default register groups will not be included for rs6000.  Solve
> this
>     by ensuring that all the default register groups are always
> present.
>     This is identical to how architectures like i386, aarch64, and
> riscv
>     all solve this problem.
> 
>     After this commit the test gdb.xml/tdesc-regs.exp should pass on
> ppc,
>     while it fails before this commit.
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index c246ce85..ee603bc6 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -6310,6 +6310,21 @@ rs6000_program_breakpoint_here_p (gdbarch
> *gdbarch, CORE_ADDR address)
>    return false;
>  }
> 
> +/* Add all the expected register sets into GDBARCH.  */
> +
> +static void
> +rs6000_add_reggroups (struct gdbarch *gdbarch)
> +{
> +  /* Add predefined register groups.  */
> +  reggroup_add (gdbarch, all_reggroup);
> +  reggroup_add (gdbarch, save_reggroup);
> +  reggroup_add (gdbarch, restore_reggroup);
> +  reggroup_add (gdbarch, system_reggroup);
> +  reggroup_add (gdbarch, vector_reggroup);
> +  reggroup_add (gdbarch, general_reggroup);
> +  reggroup_add (gdbarch, float_reggroup);
> +}
> +
>  /* Initialize the current architecture based on INFO.  If possible,
> re-use an
>     architecture from ARCHES, which is a list of architectures
> already created
>     during this debugging session.
> @@ -7235,6 +7250,8 @@ rs6000_gdbarch_init (struct gdbarch_info info,
> struct gdbarch_list *arches)
>        frame_base_append_sniffer (gdbarch,
> rs6000_frame_base_sniffer);
>      }
> 
> +  rs6000_add_reggroups (gdbarch);
> +
>    set_tdesc_pseudo_register_type (gdbarch,
> rs6000_pseudo_register_type);
>    set_tdesc_pseudo_register_reggroup_p (gdbarch,
>  					rs6000_pseudo_register_reggroup
> _p);
> 

Re: Question on the gdb.xml/tdesc-regs.exp test

[Andrew Burgess]

will schmidt via Gdb-patches <gdb-patches@sourceware.org> writes:

> On Wed, 2022-03-30 at 11:58 +0100, Andrew Burgess via Gdb-patches
> wrote:
>> Andrew Burgess <aburgess@redhat.com> writes:
>> 
>> > Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
>> > 
>> > > Stafford:
>> > > 
>> > > In your patch for the test on Dec 27, 2017, you updated the
>> > > comment in
>> > > the test to say:
>> > > 
>> > > +# Reggroups should have at least general and the extra foo
>> > > group               
>> > > +gdb_test "maintenance print reggroups"
>> > > \                                       
>> > > +    " Group\[ \t\]+Type\[ \t\]+\r\n.* general\[ \t\]+user\[
>> > > \t\]+\r\n.* foo\[ \
>> > > \t\]+user\[ \t\]+"
>> > > 
>> > > On Powerpc, the test results are as follows:
>> > > 
>> > > maintenance print reggroups
>> > >  Group      Type      
>> > >  foo        user      
>> > > (gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups
>> > > set tdesc filename test-regs.xml
>> > > 
>> > > I see that the output on X86 includes an line with general:
>> > > 
>> > >  maintenance print reggroups
>> > >  Group      Type      
>> > >  sse        user      
>> > >  mmx        user      
>> > >  general    user      
>> > > ...
>> > > 
>> > > I am wondering if you can help me understand why a line with
>> > > general is
>> > > expected?  I have been looking at Powerpc and Intel trying to
>> > > figure
>> > > out why it shows up on Intel but not Powerpc?  Anyway, it would
>> > > be good
>> > > to understand what results in the general line being generated so
>> > > I can
>> > > figure out how to get the test to run properly on
>> > > Powerpc.  Currently,
>> > > this is the only failure on Powerpc.   Thanks for your help.
>> > 
>> > The 'general' set usually includes the core integer registers for
>> > an
>> > architecture, so on ppc this would be r0 - r31.  Different
>> > architectures
>> > might add some additional registers to this set, maybe $pc if it's
>> > separate, or some flag registers... but the integer regs are the
>> > main
>> > ones.
>> > 
>> > So, if I fire up GDB on ppc and do:
>> > 
>> >   maint print register-groups
>> > 
>> > I can see that r0 - r31 are indeed in the general group.
>> > 
>> > But, if I load the xml description used in the test, I can see that
>> > the
>> > r0-r31 registers are still present, but they are no longer in the
>> > general set.  This is the problem that needs solving.
>> > 
>> > I've not (yet) looked into why this might be, but thought the above
>> > might be a good starting point if you wanted to look at this
>> > yourself.
>> 
>> OK, so what happens is this.
>> 
>> In "normal" native operation, the target descriptions GDB uses don't
>> include any register groups, so reggroup_add is never called.
>> 
>> As a result, when we call reggroup_next to iterate over the register
>> groups we end up using the "default" set of register groups, this
>> default list includes the general purpose set.
>> 
>> Then, in the default_register_reggroup_p function, which decides if a
>> register is in a group or not (which ppc uses), and non-vector,
>> non-float register is declared in the general set.
>> 
>> Now, in the test, we do add a register group to the target
>> description,
>> the group 'foo'.  This causes reggroup_add to be called, which starts
>> a
>> new register group list within the gdbarch.
>> 
>> Now when GDB calls reggroup_next we find we do have a list of
>> reggroups,
>> so we use that instead of the default list.  Unfortunately, for ppc,
>> this means we don't include the general set.
>> 
>> My honest opinion is that the whole "default register groups"
>> mechanism
>> in GDB is not really up to scratch and needs to be rewritten.  But
>> you
>> probably don't care about that.
>> 
>> What most architectures do to solve this problem is add all of the
>> default register groups from within their own *-tdep.c file.  For
>> example the function i386_add_reggroups, aarch64_add_reggroups, or
>> riscv_add_reggroups, etc.
>> 
>> If you look at riscv_add_reggroups you'll see the registers are split
>> into two sets, the 7 default groups, and then 1 RISC-V specific
>> set.  I
>> think ppc just needs a new rs6000_add_reggroups function that adds
>> the
>> same 7 default groups.
>> 
>> The patch below does just this.  Other than running the
>> gdb.xml/tdesc-regs.exp test, I've done no testing on this yet, but do
>> let me know your thoughts.
>
> Hi Carl, Andrew, all, 
>    Rogerio should be on copy for these.
>
> We (powerpc/rs6000 have an
> internal todo to rework the rs6000 code to fully leverage the XML
> target descriptions.  Rogerio has a set of patches / work in progress
> for this effort. 
> The tdesc-regs.exp test is one of those test failures
> that gets addressed with that patch set, though there is not a firm ETA
> for the patch set to be upstreamed at this time.
>
> The proposed patch looks reasonable to me.  :-)

I pushed an alternative series that ensures the default groups are
always present:

  https://sourceware.org/pipermail/gdb-patches/2022-April/187504.html

Integration with any ongoing work should be simple enough - you no
longer need to register the default register groups for ppc, these will
be registered automatically now.  Any ppc specific groups should be
added in the normal way.

Thanks,
Andrew

Re: Question on the gdb.xml/tdesc-regs.exp test

[will schmidt]

On Thu, 2022-04-07 at 16:15 +0100, Andrew Burgess wrote:
> will schmidt via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > On Wed, 2022-03-30 at 11:58 +0100, Andrew Burgess via Gdb-patches
> > wrote:
> > > Andrew Burgess <aburgess@redhat.com> writes:
> > > 
> > > > Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> > > > 
> > > > > 

<snipsnip>

> > We (powerpc/rs6000 have an
> > internal todo to rework the rs6000 code to fully leverage the XML
> > target descriptions.  Rogerio has a set of patches / work in
> > progress
> > for this effort. 
> > The tdesc-regs.exp test is one of those test failures
> > that gets addressed with that patch set, though there is not a firm
> > ETA
> > for the patch set to be upstreamed at this time.
> > 
> > The proposed patch looks reasonable to me.  :-)
> 
> I pushed an alternative series that ensures the default groups are
> always present:
> 
>   https://sourceware.org/pipermail/gdb-patches/2022-April/187504.html
> 
> Integration with any ongoing work should be simple enough - you no
> longer need to register the default register groups for ppc, these
> will
> be registered automatically now.  Any ppc specific groups should be
> added in the normal way.

Sounds good, thanks! 


Thanks,
-Will


> 
> Thanks,
> Andrew
>