gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp

Message ID 71c00a14-a956-a087-8b9a-9b453194d4f2@codesourcery.com
State New
Headers show
Series
  • gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
Related show

Commit Message

Sandra Loosemore June 16, 2020, 8:04 p.m.
We've had this patch to fix various failures in gdb.xml/tdesc-regs.exp 
in our local tree for a few years now and would like to get it committed 
upstream.  It fixes these problems:

- It's using the wrong source pathname when trying to copy the .xml file 
to remote host.

- We've seen at least one case where the type of the 32-bit register 
prints as "int32_t" rather than "int|long" etc -- I think this was on an 
ilp64 target.

- This test expects to see a register group named "general" but not all 
targets provide one.

OK to commit?

-Sandra

Comments

Andrew Burgess June 16, 2020, 8:47 p.m. | #1
* Sandra Loosemore <sandra@codesourcery.com> [2020-06-16 14:04:51 -0600]:

> We've had this patch to fix various failures in gdb.xml/tdesc-regs.exp in

> our local tree for a few years now and would like to get it committed

> upstream.  It fixes these problems:

> 

> - It's using the wrong source pathname when trying to copy the .xml file to

> remote host.

> 

> - We've seen at least one case where the type of the 32-bit register prints

> as "int32_t" rather than "int|long" etc -- I think this was on an ilp64

> target.

> 

> - This test expects to see a register group named "general" but not all

> targets provide one.


I'd be interested to know more about which targets don't place any
registers in the 'general' group.  This group is used in
default_print_registers_info to implement 'info registers', so I'd
like to see what this particular target has done instead.

Thanks,
Andrew


> 

> OK to commit?

> 

> -Sandra


> commit d32235b2037694e2586f83b6c3a5bc76fd1241ab

> Author: Sandra Loosemore <sandra@codesourcery.com>

> Date:   Tue Jun 16 12:48:42 2020 -0700

> 

>     gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp

>     

>     2020-06-16  Sandra Loosemore  <sandra@codesourcery.com>

>     	    Hafiz Abid Qadeer  <abidh@codesourcery.com>

>     

>     	gdb/testsuite/

>     	* gdb.xml/tdesc-regs.exp (load_description): Correct pathname of

>     	file sent to remote host.

>     	(top level): Allow int32_t as type of 32-bit register.  Don't

>     	require a register group named "general".

> 

> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog

> index d2ed9db..4b8c7b5 100644

> --- a/gdb/testsuite/ChangeLog

> +++ b/gdb/testsuite/ChangeLog

> @@ -1,3 +1,11 @@

> +2020-06-16  Sandra Loosemore  <sandra@codesourcery.com>

> +	    Hafiz Abid Qadeer  <abidh@codesourcery.com>

> +

> +	* gdb.xml/tdesc-regs.exp (load_description): Correct pathname of

> +	file sent to remote host.

> +	(top level): Allow int32_t as type of 32-bit register.  Don't

> +	require a register group named "general".

> +

>  2020-06-16  Gary Benson <gbenson@redhat.com>

>  

>  	* gdb.python/py-nested-maps.c (create_map): Add missing return

> diff --git a/gdb/testsuite/gdb.xml/tdesc-regs.exp b/gdb/testsuite/gdb.xml/tdesc-regs.exp

> index bb04420..b1e4525 100644

> --- a/gdb/testsuite/gdb.xml/tdesc-regs.exp

> +++ b/gdb/testsuite/gdb.xml/tdesc-regs.exp

> @@ -145,7 +145,7 @@ proc load_description { file errmsg xml_file } {

>      close $ofd

>  

>      if {[is_remote host]} {

> -	set regs_file [remote_download host "$subdir/$xml_file" $xml_file]

> +	set regs_file [remote_download host "$regs_file" $xml_file]

>      }

>  

>      # Anchor the test output, so that error messages are detected.

> @@ -165,7 +165,7 @@ if {![is_remote host]} {

>  }

>  

>  load_description "extra-regs.xml" "" "test-extra-regs.xml"

> -gdb_test "ptype \$extrareg" "type = (int|long|long long)"

> +gdb_test "ptype \$extrareg" "type = (int32_t|int|long|long long)"

>  gdb_test "ptype \$uintreg" "type = uint32_t"

>  gdb_test "ptype \$vecreg" "type = int8_t __attribute__ \\(\\(vector_size\\(4\\)\\)\\)"

>  gdb_test "ptype \$unionreg" \

> @@ -180,9 +180,9 @@ gdb_test "ptype \$flags" \

>      "type = flag flags {\r\n *bool X @0;\r\n *uint32_t Y @2;\r\n}"

>  gdb_test "ptype \$mixed_flags" \

>      "type = flag mixed_flags {\r\n *bool A @0;\r\n *uint32_t B @1-3;\r\n *bool C @4;\r\n *uint32_t D @5;\r\n *uint32_t @6-7;\r\n *enum Z_values {yes = 1, no = 0, maybe = 2, so} Z @8-9;\r\n}"

> -# Reggroups should have at least general and the extra foo group

> +# Reggroups should have at least 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\]+"

> +    " Group\[ \t\]+Type\[ \t\]+\r\n.* foo\[ \t\]+user\[ \t\]+"

>  

>  with_test_prefix "core-only.xml" {

>      load_description "core-only.xml" "" "test-regs.xml"
Sandra Loosemore June 16, 2020, 9:08 p.m. | #2
On 6/16/20 2:47 PM, Andrew Burgess wrote:

> I'd be interested to know more about which targets don't place any

> registers in the 'general' group.  This group is used in

> default_print_registers_info to implement 'info registers', so I'd

> like to see what this particular target has done instead.


nios2, for one.  From the original test log for nios2-linux-gnu:

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

I don't have a mainline build for ARM handy but the failure reproduces 
on GDB 9 branch for arm-none-eabi.

This is Abid's original summary of his investigation in 2018:

   There are 2 reggroups maintained in the code. One is the
   default_groups which is populated in _initialize_reggroup. This group
   has 'general' group. So if you do 'maint print reggroups without
   loading anything in gdb, you will see 'general' group.

   The other group is stored in reggroups_data. This is populated when we
   load a target description xml file. So it only has group defined in
   xml file. But if has a valid group, then 'maint print reggroups' will
   not use default_groups. So that command will not print 'general' group
   if xml files did not have any.

   For x86, we have slightly different behavior. Its tdep code calls
   i386_add_reggroups which add 'general' group to reggroups_data as
   well.

   So this testcase assumes that 'general' group is always present. This
   asumption is true on x86 only not in general. So I have removed it
   from the match pattern.

-Sandra
Andrew Burgess June 18, 2020, 9:43 a.m. | #3
* Sandra Loosemore <sandra@codesourcery.com> [2020-06-16 15:08:59 -0600]:

> On 6/16/20 2:47 PM, Andrew Burgess wrote:

> 

> > I'd be interested to know more about which targets don't place any

> > registers in the 'general' group.  This group is used in

> > default_print_registers_info to implement 'info registers', so I'd

> > like to see what this particular target has done instead.

> 

> nios2, for one.  From the original test log for nios2-linux-gnu:


OK, but...

Please bear with me, I don't have a nios2 tool chain to hand, but...

In nios2-tdep.c, there is no call to set_gdbarch_print_registers_info,
which means (from gdbarch.c) that nios2 will use
default_print_registers_info.

Now if we look in infcmd.c at both default_print_registers_info and
registers_info, then we see that if a user says:

  (gdb) info registers

then they will be asking which registers are in the general_reggroup.

Now, nios can optionally use a remote target-description (from
nios2_gdbarch_init), so, lets for now assume no target description.  I
see no call to set_gdbarch_register_reggroup_p for nios2, which means
we are going to be using default_register_reggroup_p, which if we
inspect (in reggroups.c) we'll see does place some registers into the
general group.

Now, does this matter? The reggroup name is never printed anywhere, we
just see a set of random registers?

But it is annoying that the user is not easily able to say:

  (gdb) info registers general

and get the same output.

If we look at how other targets deal with this most of them manually
add at least some sub-set of the default register groups to their
architectures set of register groups as part of their _gdbarch_init
routine.

My gut instinct here is that this is what nios2 should be doing (and
maybe arm too).

It does seem odd that there's no central "add-the-default-reggroups"
type routine that targets can or should call.

Thanks,
Andrew

> 

> maintenance print reggroups

>  Group      Type

>  foo        user

> (gdb) FAIL: gdb.xml/tdesc-regs.exp: maintenance print reggroups

> 

> I don't have a mainline build for ARM handy but the failure reproduces on

> GDB 9 branch for arm-none-eabi.

> 

> This is Abid's original summary of his investigation in 2018:

> 

>   There are 2 reggroups maintained in the code. One is the

>   default_groups which is populated in _initialize_reggroup. This group

>   has 'general' group. So if you do 'maint print reggroups without

>   loading anything in gdb, you will see 'general' group.

> 

>   The other group is stored in reggroups_data. This is populated when we

>   load a target description xml file. So it only has group defined in

>   xml file. But if has a valid group, then 'maint print reggroups' will

>   not use default_groups. So that command will not print 'general' group

>   if xml files did not have any.

> 

>   For x86, we have slightly different behavior. Its tdep code calls

>   i386_add_reggroups which add 'general' group to reggroups_data as

>   well.

> 

>   So this testcase assumes that 'general' group is always present. This

>   asumption is true on x86 only not in general. So I have removed it

>   from the match pattern.

> 

> -Sandra
Sandra Loosemore June 19, 2020, 2:18 a.m. | #4
On 6/18/20 3:43 AM, Andrew Burgess wrote:
> * Sandra Loosemore <sandra@codesourcery.com> [2020-06-16 15:08:59 -0600]:

> 

>> On 6/16/20 2:47 PM, Andrew Burgess wrote:

>>

>>> I'd be interested to know more about which targets don't place any

>>> registers in the 'general' group.  This group is used in

>>> default_print_registers_info to implement 'info registers', so I'd

>>> like to see what this particular target has done instead.

>>

>> nios2, for one.  From the original test log for nios2-linux-gnu:

> 

> OK, but...

> 

> Please bear with me, I don't have a nios2 tool chain to hand, but...

> 

> In nios2-tdep.c, there is no call to set_gdbarch_print_registers_info,

> which means (from gdbarch.c) that nios2 will use

> default_print_registers_info.

> 

> Now if we look in infcmd.c at both default_print_registers_info and

> registers_info, then we see that if a user says:

> 

>    (gdb) info registers

> 

> then they will be asking which registers are in the general_reggroup.

> 

> Now, nios can optionally use a remote target-description (from

> nios2_gdbarch_init), so, lets for now assume no target description.  I

> see no call to set_gdbarch_register_reggroup_p for nios2, which means

> we are going to be using default_register_reggroup_p, which if we

> inspect (in reggroups.c) we'll see does place some registers into the

> general group.

> 

> Now, does this matter? The reggroup name is never printed anywhere, we

> just see a set of random registers?

> 

> But it is annoying that the user is not easily able to say:

> 

>    (gdb) info registers general

> 

> and get the same output.

> 

> If we look at how other targets deal with this most of them manually

> add at least some sub-set of the default register groups to their

> architectures set of register groups as part of their _gdbarch_init

> routine.

> 

> My gut instinct here is that this is what nios2 should be doing (and

> maybe arm too).

> 

> It does seem odd that there's no central "add-the-default-reggroups"

> type routine that targets can or should call.


OK.  How about I commit the other two pieces of the patch (which seem 
like genuine bugs in the testcase), and treat this one as uncovering a 
bug in the implementation instead of just an inappropriate x86-specific 
assumption wired into the testcase?

-Sandra
Andrew Burgess June 19, 2020, 8:03 a.m. | #5
* Sandra Loosemore <sandra@codesourcery.com> [2020-06-18 20:18:18 -0600]:

> On 6/18/20 3:43 AM, Andrew Burgess wrote:

> > * Sandra Loosemore <sandra@codesourcery.com> [2020-06-16 15:08:59 -0600]:

> > 

> > > On 6/16/20 2:47 PM, Andrew Burgess wrote:

> > > 

> > > > I'd be interested to know more about which targets don't place any

> > > > registers in the 'general' group.  This group is used in

> > > > default_print_registers_info to implement 'info registers', so I'd

> > > > like to see what this particular target has done instead.

> > > 

> > > nios2, for one.  From the original test log for nios2-linux-gnu:

> > 

> > OK, but...

> > 

> > Please bear with me, I don't have a nios2 tool chain to hand, but...

> > 

> > In nios2-tdep.c, there is no call to set_gdbarch_print_registers_info,

> > which means (from gdbarch.c) that nios2 will use

> > default_print_registers_info.

> > 

> > Now if we look in infcmd.c at both default_print_registers_info and

> > registers_info, then we see that if a user says:

> > 

> >    (gdb) info registers

> > 

> > then they will be asking which registers are in the general_reggroup.

> > 

> > Now, nios can optionally use a remote target-description (from

> > nios2_gdbarch_init), so, lets for now assume no target description.  I

> > see no call to set_gdbarch_register_reggroup_p for nios2, which means

> > we are going to be using default_register_reggroup_p, which if we

> > inspect (in reggroups.c) we'll see does place some registers into the

> > general group.

> > 

> > Now, does this matter? The reggroup name is never printed anywhere, we

> > just see a set of random registers?

> > 

> > But it is annoying that the user is not easily able to say:

> > 

> >    (gdb) info registers general

> > 

> > and get the same output.

> > 

> > If we look at how other targets deal with this most of them manually

> > add at least some sub-set of the default register groups to their

> > architectures set of register groups as part of their _gdbarch_init

> > routine.

> > 

> > My gut instinct here is that this is what nios2 should be doing (and

> > maybe arm too).

> > 

> > It does seem odd that there's no central "add-the-default-reggroups"

> > type routine that targets can or should call.

> 

> OK.  How about I commit the other two pieces of the patch (which seem like

> genuine bugs in the testcase), and treat this one as uncovering a bug in the

> implementation instead of just an inappropriate x86-specific assumption

> wired into the testcase?


That sounds good to me.

Thanks,
Andrew

Patch

commit d32235b2037694e2586f83b6c3a5bc76fd1241ab
Author: Sandra Loosemore <sandra@codesourcery.com>
Date:   Tue Jun 16 12:48:42 2020 -0700

    gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
    
    2020-06-16  Sandra Loosemore  <sandra@codesourcery.com>
    	    Hafiz Abid Qadeer  <abidh@codesourcery.com>
    
    	gdb/testsuite/
    	* gdb.xml/tdesc-regs.exp (load_description): Correct pathname of
    	file sent to remote host.
    	(top level): Allow int32_t as type of 32-bit register.  Don't
    	require a register group named "general".

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d2ed9db..4b8c7b5 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,11 @@ 
+2020-06-16  Sandra Loosemore  <sandra@codesourcery.com>
+	    Hafiz Abid Qadeer  <abidh@codesourcery.com>
+
+	* gdb.xml/tdesc-regs.exp (load_description): Correct pathname of
+	file sent to remote host.
+	(top level): Allow int32_t as type of 32-bit register.  Don't
+	require a register group named "general".
+
 2020-06-16  Gary Benson <gbenson@redhat.com>
 
 	* gdb.python/py-nested-maps.c (create_map): Add missing return
diff --git a/gdb/testsuite/gdb.xml/tdesc-regs.exp b/gdb/testsuite/gdb.xml/tdesc-regs.exp
index bb04420..b1e4525 100644
--- a/gdb/testsuite/gdb.xml/tdesc-regs.exp
+++ b/gdb/testsuite/gdb.xml/tdesc-regs.exp
@@ -145,7 +145,7 @@  proc load_description { file errmsg xml_file } {
     close $ofd
 
     if {[is_remote host]} {
-	set regs_file [remote_download host "$subdir/$xml_file" $xml_file]
+	set regs_file [remote_download host "$regs_file" $xml_file]
     }
 
     # Anchor the test output, so that error messages are detected.
@@ -165,7 +165,7 @@  if {![is_remote host]} {
 }
 
 load_description "extra-regs.xml" "" "test-extra-regs.xml"
-gdb_test "ptype \$extrareg" "type = (int|long|long long)"
+gdb_test "ptype \$extrareg" "type = (int32_t|int|long|long long)"
 gdb_test "ptype \$uintreg" "type = uint32_t"
 gdb_test "ptype \$vecreg" "type = int8_t __attribute__ \\(\\(vector_size\\(4\\)\\)\\)"
 gdb_test "ptype \$unionreg" \
@@ -180,9 +180,9 @@  gdb_test "ptype \$flags" \
     "type = flag flags {\r\n *bool X @0;\r\n *uint32_t Y @2;\r\n}"
 gdb_test "ptype \$mixed_flags" \
     "type = flag mixed_flags {\r\n *bool A @0;\r\n *uint32_t B @1-3;\r\n *bool C @4;\r\n *uint32_t D @5;\r\n *uint32_t @6-7;\r\n *enum Z_values {yes = 1, no = 0, maybe = 2, so} Z @8-9;\r\n}"
-# Reggroups should have at least general and the extra foo group
+# Reggroups should have at least 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\]+"
+    " Group\[ \t\]+Type\[ \t\]+\r\n.* foo\[ \t\]+user\[ \t\]+"
 
 with_test_prefix "core-only.xml" {
     load_description "core-only.xml" "" "test-regs.xml"