* [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
@ 2020-06-16 20:04 Sandra Loosemore
2020-06-16 20:47 ` Andrew Burgess
0 siblings, 1 reply; 6+ messages in thread
From: Sandra Loosemore @ 2020-06-16 20:04 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
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
[-- Attachment #2: tdesc-regs.patch --]
[-- Type: text/x-patch, Size: 2837 bytes --]
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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
2020-06-16 20:04 [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp Sandra Loosemore
@ 2020-06-16 20:47 ` Andrew Burgess
2020-06-16 21:08 ` Sandra Loosemore
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2020-06-16 20:47 UTC (permalink / raw)
To: Sandra Loosemore; +Cc: gdb-patches
* 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"
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
2020-06-16 20:47 ` Andrew Burgess
@ 2020-06-16 21:08 ` Sandra Loosemore
2020-06-18 9:43 ` Andrew Burgess
0 siblings, 1 reply; 6+ messages in thread
From: Sandra Loosemore @ 2020-06-16 21:08 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
2020-06-16 21:08 ` Sandra Loosemore
@ 2020-06-18 9:43 ` Andrew Burgess
2020-06-19 2:18 ` Sandra Loosemore
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2020-06-18 9:43 UTC (permalink / raw)
To: Sandra Loosemore; +Cc: gdb-patches
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
2020-06-18 9:43 ` Andrew Burgess
@ 2020-06-19 2:18 ` Sandra Loosemore
2020-06-19 8:03 ` Andrew Burgess
0 siblings, 1 reply; 6+ messages in thread
From: Sandra Loosemore @ 2020-06-19 2:18 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp
2020-06-19 2:18 ` Sandra Loosemore
@ 2020-06-19 8:03 ` Andrew Burgess
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Burgess @ 2020-06-19 8:03 UTC (permalink / raw)
To: Sandra Loosemore; +Cc: gdb-patches
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-19 8:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-16 20:04 [patch] gdb/testsuite: fixes for gdb.xml/tdesc-regs.exp Sandra Loosemore
2020-06-16 20:47 ` Andrew Burgess
2020-06-16 21:08 ` Sandra Loosemore
2020-06-18 9:43 ` Andrew Burgess
2020-06-19 2:18 ` Sandra Loosemore
2020-06-19 8:03 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).