* [PATCH] Do not record a rejected target description @ 2023-01-12 19:33 Tom Tromey 2023-02-14 15:16 ` Tom Tromey 2023-02-14 15:48 ` Andrew Burgess 0 siblings, 2 replies; 13+ messages in thread From: Tom Tromey @ 2023-01-12 19:33 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey When connecting to a certain target, gdb issues a warning about the target description: (gdb) target remote localhost:7947 Remote debugging using localhost:7947 warning: Architecture rejected target-supplied description If you then kill the inferior and change the exec-file, this will happen: (gdb) file bar Architecture of file not recognized. After this, debugging doesn't work very well. What happens here is that, despite the warning, target_find_description records the downloaded description in the target_desc_info. Then the "file" command ends up calling set_gdbarch_from_file, which uses that description. It seems to me that, because the architecture rejected the description, it should not be used. That is what this patch implements. --- gdb/target-descriptions.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1a451c79b82..38d0b3f8c7d 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -565,7 +565,10 @@ target_find_description (void) info.target_desc = tdesc_info->tdesc; if (!gdbarch_update_p (info)) - warning (_("Architecture rejected target-supplied description")); + { + warning (_("Architecture rejected target-supplied description")); + tdesc_info->tdesc = nullptr; + } else { struct tdesc_arch_data *data; -- 2.38.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-01-12 19:33 [PATCH] Do not record a rejected target description Tom Tromey @ 2023-02-14 15:16 ` Tom Tromey 2023-02-14 15:48 ` Andrew Burgess 1 sibling, 0 replies; 13+ messages in thread From: Tom Tromey @ 2023-02-14 15:16 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey >>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: Tom> When connecting to a certain target, gdb issues a warning about the Tom> target description: Tom> (gdb) target remote localhost:7947 Tom> Remote debugging using localhost:7947 Tom> warning: Architecture rejected target-supplied description Tom> If you then kill the inferior and change the exec-file, this will Tom> happen: Tom> (gdb) file bar Tom> Architecture of file not recognized. Tom> After this, debugging doesn't work very well. Tom> What happens here is that, despite the warning, Tom> target_find_description records the downloaded description in the Tom> target_desc_info. Then the "file" command ends up calling Tom> set_gdbarch_from_file, which uses that description. Tom> It seems to me that, because the architecture rejected the Tom> description, it should not be used. That is what this patch Tom> implements. We've been using this internally with our misbehaving target and it is fine, but I'd appreciate someone else's review. thanks, Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-01-12 19:33 [PATCH] Do not record a rejected target description Tom Tromey 2023-02-14 15:16 ` Tom Tromey @ 2023-02-14 15:48 ` Andrew Burgess 2023-02-14 17:27 ` Tom Tromey 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-02-14 15:48 UTC (permalink / raw) To: Tom Tromey via Gdb-patches, gdb-patches; +Cc: Tom Tromey Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: > When connecting to a certain target, gdb issues a warning about the > target description: > > (gdb) target remote localhost:7947 > Remote debugging using localhost:7947 > warning: Architecture rejected target-supplied description > > If you then kill the inferior and change the exec-file, this will > happen: > > (gdb) file bar > Architecture of file not recognized. > > After this, debugging doesn't work very well. > > What happens here is that, despite the warning, > target_find_description records the downloaded description in the > target_desc_info. Then the "file" command ends up calling > set_gdbarch_from_file, which uses that description. > > It seems to me that, because the architecture rejected the > description, it should not be used. That is what this patch > implements. > --- > gdb/target-descriptions.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index 1a451c79b82..38d0b3f8c7d 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -565,7 +565,10 @@ target_find_description (void) > > info.target_desc = tdesc_info->tdesc; > if (!gdbarch_update_p (info)) > - warning (_("Architecture rejected target-supplied description")); > + { > + warning (_("Architecture rejected target-supplied description")); > + tdesc_info->tdesc = nullptr; > + } This seems fine, but it is possible that an exception could take us out of this function too, for example, a misbehaving remote target can cause target_read_description_xml to throw an exception. I guess that any of the issues you were originally seeing would also trigger if we throw an exception. I wonder if a better solution would be to use a RAII object to ensure that tdesc_info->tdesc is always reset to nullptr on scope exit, then at the end of target_find_description, after this line: tdesc_info->fetched = true; we would call a method on the RAII object to indicate that it should NOT clear the tdesc field. Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-14 15:48 ` Andrew Burgess @ 2023-02-14 17:27 ` Tom Tromey 2023-02-15 12:56 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2023-02-14 17:27 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey via Gdb-patches, Tom Tromey >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: >> info.target_desc = tdesc_info->tdesc; >> if (!gdbarch_update_p (info)) >> - warning (_("Architecture rejected target-supplied description")); >> + { >> + warning (_("Architecture rejected target-supplied description")); >> + tdesc_info->tdesc = nullptr; >> + } Andrew> This seems fine, but it is possible that an exception could take us out Andrew> of this function too, for example, a misbehaving remote target can cause Andrew> target_read_description_xml to throw an exception. I guess that any of Andrew> the issues you were originally seeing would also trigger if we throw an Andrew> exception. I don't think this can really happen, because that call is part of the assignment to the field in question: if (tdesc_info->tdesc == nullptr) tdesc_info->tdesc = target_read_description (current_inferior ()->top_target ()); So, if it does throw, the tdesc_info won't be updated. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-14 17:27 ` Tom Tromey @ 2023-02-15 12:56 ` Andrew Burgess 2023-02-15 15:59 ` Tom Tromey 0 siblings, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-02-15 12:56 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey via Gdb-patches, Tom Tromey Tom Tromey <tromey@adacore.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > >>> info.target_desc = tdesc_info->tdesc; >>> if (!gdbarch_update_p (info)) >>> - warning (_("Architecture rejected target-supplied description")); >>> + { >>> + warning (_("Architecture rejected target-supplied description")); >>> + tdesc_info->tdesc = nullptr; >>> + } > > Andrew> This seems fine, but it is possible that an exception could take us out > Andrew> of this function too, for example, a misbehaving remote target can cause > Andrew> target_read_description_xml to throw an exception. I guess that any of > Andrew> the issues you were originally seeing would also trigger if we throw an > Andrew> exception. > > I don't think this can really happen, because that call is part of the > assignment to the field in question: > > if (tdesc_info->tdesc == nullptr) > tdesc_info->tdesc = target_read_description > (current_inferior ()->top_target ()); > > So, if it does throw, the tdesc_info won't be updated. You are, of course, correct. I guess given we know nothing after the assignment can throw the what you have is fine. Hopefully we'll not add an error() call later. Sorry for the noise. Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-15 12:56 ` Andrew Burgess @ 2023-02-15 15:59 ` Tom Tromey 2023-02-15 21:19 ` Simon Marchi 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2023-02-15 15:59 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, Tom Tromey via Gdb-patches Andrew> You are, of course, correct. I guess given we know nothing after the Andrew> assignment can throw the what you have is fine. Hopefully we'll not add Andrew> an error() call later. Andrew> Sorry for the noise. No, thank you for taking a look at this. I appreciate it. I'm going to check it in now. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-15 15:59 ` Tom Tromey @ 2023-02-15 21:19 ` Simon Marchi 2023-02-15 21:38 ` Tom Tromey 2023-02-16 19:20 ` Tom Tromey 0 siblings, 2 replies; 13+ messages in thread From: Simon Marchi @ 2023-02-15 21:19 UTC (permalink / raw) To: Tom Tromey, Andrew Burgess; +Cc: Tom Tromey via Gdb-patches On 2/15/23 10:59, Tom Tromey via Gdb-patches wrote: > Andrew> You are, of course, correct. I guess given we know nothing after the > Andrew> assignment can throw the what you have is fine. Hopefully we'll not add > Andrew> an error() call later. > > Andrew> Sorry for the noise. > > No, thank you for taking a look at this. I appreciate it. I'm going to > check it in now. I see: maint print c-tdesc^M There is no target description to print.^M (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Simon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-15 21:19 ` Simon Marchi @ 2023-02-15 21:38 ` Tom Tromey 2023-02-16 19:20 ` Tom Tromey 1 sibling, 0 replies; 13+ messages in thread From: Tom Tromey @ 2023-02-15 21:38 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, Andrew Burgess, Tom Tromey via Gdb-patches Simon> I see: Simon> maint print c-tdesc^M Simon> There is no target description to print.^M Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Sorry about that. I'll take a look. Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-15 21:19 ` Simon Marchi 2023-02-15 21:38 ` Tom Tromey @ 2023-02-16 19:20 ` Tom Tromey 2023-02-16 19:33 ` Simon Marchi 2023-02-17 10:19 ` Andrew Burgess 1 sibling, 2 replies; 13+ messages in thread From: Tom Tromey @ 2023-02-16 19:20 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, Andrew Burgess, Tom Tromey via Gdb-patches Simon> I see: Simon> maint print c-tdesc^M Simon> There is no target description to print.^M Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield Here's a patch that avoids this problem. Let me know what you think. Tom commit a12897d22ccc1d927960fa411b99723d76620af0 Author: Tom Tromey <tromey@adacore.com> Date: Thu Feb 16 12:17:50 2023 -0700 Fix regression in maint_print_struct.exp An earlier patch of mine caused a regression in maint_print_struct.exp. This patch fixes it by modifying the test so that it does not rely on the erroneous target description being available. diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp index 6f411895501..5b7c1489d3e 100644 --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp @@ -21,12 +21,7 @@ require allow_xml_test gdb_start -# Required registers are not present so it is expected a warning. -# -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " -warning:.*" "setting a new tdesc having only a structure" - -gdb_test "maint print c-tdesc" " +gdb_test "maint print c-tdesc $srcdir/$subdir/maint_print_struct.xml" " .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r .*" "printing tdesc with a structure and a bitfield" ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-16 19:20 ` Tom Tromey @ 2023-02-16 19:33 ` Simon Marchi 2023-02-17 10:19 ` Andrew Burgess 1 sibling, 0 replies; 13+ messages in thread From: Simon Marchi @ 2023-02-16 19:33 UTC (permalink / raw) To: Tom Tromey; +Cc: Andrew Burgess, Tom Tromey via Gdb-patches On 2/16/23 14:20, Tom Tromey wrote: > Simon> I see: > > Simon> maint print c-tdesc^M > Simon> There is no target description to print.^M > Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield > > Here's a patch that avoids this problem. > Let me know what you think. > > Tom > > commit a12897d22ccc1d927960fa411b99723d76620af0 > Author: Tom Tromey <tromey@adacore.com> > Date: Thu Feb 16 12:17:50 2023 -0700 > > Fix regression in maint_print_struct.exp > > An earlier patch of mine caused a regression in > maint_print_struct.exp. This patch fixes it by modifying the test so > that it does not rely on the erroneous target description being > available. I don't have time to dig in the implementation to review, but I confirmed it fixes the problem on my side, so: Tested-By: Simon Marchi <simon.marchi@efficios.com> Simon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-16 19:20 ` Tom Tromey 2023-02-16 19:33 ` Simon Marchi @ 2023-02-17 10:19 ` Andrew Burgess 2023-02-17 14:09 ` Tom Tromey 1 sibling, 1 reply; 13+ messages in thread From: Andrew Burgess @ 2023-02-17 10:19 UTC (permalink / raw) To: Tom Tromey, Simon Marchi; +Cc: Tom Tromey, Tom Tromey via Gdb-patches Tom Tromey <tromey@adacore.com> writes: > Simon> I see: > > Simon> maint print c-tdesc^M > Simon> There is no target description to print.^M > Simon> (gdb) FAIL: gdb.xml/maint_print_struct.exp: printing tdesc with a structure and a bitfield > > Here's a patch that avoids this problem. > Let me know what you think. > > Tom > > commit a12897d22ccc1d927960fa411b99723d76620af0 > Author: Tom Tromey <tromey@adacore.com> > Date: Thu Feb 16 12:17:50 2023 -0700 > > Fix regression in maint_print_struct.exp > > An earlier patch of mine caused a regression in > maint_print_struct.exp. This patch fixes it by modifying the test so > that it does not rely on the erroneous target description being > available. > > diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp > index 6f411895501..5b7c1489d3e 100644 > --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp > +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp > @@ -21,12 +21,7 @@ require allow_xml_test > > gdb_start > > -# Required registers are not present so it is expected a warning. > -# > -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " > -warning:.*" "setting a new tdesc having only a structure" Haven't you just deleted a test for a warning here? > - > -gdb_test "maint print c-tdesc" " This test (without the filename) actually tests for the change in your original patch - if we just update the expected output... > +gdb_test "maint print c-tdesc $srcdir/$subdir/maint_print_struct.xml" " This should probably be as well as retaining the above tests. I'd propose the patch below. thanks, Andrew > .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r > .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r > .*" "printing tdesc with a structure and a bitfield" --- commit 416465d0331d3347bebf4c72c544d7d3ecb926e7 Author: Andrew Burgess <aburgess@redhat.com> Date: Fri Feb 17 10:15:27 2023 +0000 gdb: fix regression in gdb.xml/maint_print_struct.exp A regression in gdb.xml/maint_print_struct.exp was introduced with commit: commit 81b86eced24f905545b58aa6c27478104c364976 Date: Fri Jan 6 09:30:40 2023 -0700 Do not record a rejected target description The test relied on an invalid target description being stored within the tdesc_info of the current inferior, the above commit stopped this behaviour. Update the test to check that the invalid architecture is NOT stored, and then check printing the target description directly from the file. diff --git a/gdb/testsuite/gdb.xml/maint_print_struct.exp b/gdb/testsuite/gdb.xml/maint_print_struct.exp index 6f411895501..fbb16aeb8f5 100644 --- a/gdb/testsuite/gdb.xml/maint_print_struct.exp +++ b/gdb/testsuite/gdb.xml/maint_print_struct.exp @@ -21,12 +21,17 @@ require allow_xml_test gdb_start +set xml_file "$srcdir/$subdir/maint_print_struct.xml" + # Required registers are not present so it is expected a warning. # -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " +gdb_test "set tdesc filename $xml_file" " warning:.*" "setting a new tdesc having only a structure" -gdb_test "maint print c-tdesc" " +gdb_test "maint print c-tdesc" \ + "There is no target description to print\\." + +gdb_test "maint print c-tdesc $xml_file" " .*tdesc_create_reg \\(feature, \"bad_reg1\", \[0-9\]+, 1, NULL, 128, \"two_fielded\"\\);\r .*tdesc_create_reg \\(feature, \"bad_reg2\", \[0-9\]+, 1, NULL, 64, \"bitfield\"\\);\r .*" "printing tdesc with a structure and a bitfield" ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-17 10:19 ` Andrew Burgess @ 2023-02-17 14:09 ` Tom Tromey 2023-02-17 22:29 ` Andrew Burgess 0 siblings, 1 reply; 13+ messages in thread From: Tom Tromey @ 2023-02-17 14:09 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, Simon Marchi, Tom Tromey via Gdb-patches >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: >> -# Required registers are not present so it is expected a warning. >> -# >> -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " >> -warning:.*" "setting a new tdesc having only a structure" Andrew> Haven't you just deleted a test for a warning here? I thought it was just incidental. I looked at the history of the file and the original commit message only discusses handling the "struct" case. Andrew> I'd propose the patch below. Looks good to me, thanks. Approved-By: Tom Tromey <tromey@adacore.com> Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Do not record a rejected target description 2023-02-17 14:09 ` Tom Tromey @ 2023-02-17 22:29 ` Andrew Burgess 0 siblings, 0 replies; 13+ messages in thread From: Andrew Burgess @ 2023-02-17 22:29 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey, Simon Marchi, Tom Tromey via Gdb-patches Tom Tromey <tromey@adacore.com> writes: >>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes: > >>> -# Required registers are not present so it is expected a warning. >>> -# >>> -gdb_test "set tdesc filename $srcdir/$subdir/maint_print_struct.xml" " >>> -warning:.*" "setting a new tdesc having only a structure" > > Andrew> Haven't you just deleted a test for a warning here? > > I thought it was just incidental. I looked at the history of the file > and the original commit message only discusses handling the "struct" > case. > > Andrew> I'd propose the patch below. > > Looks good to me, thanks. > Approved-By: Tom Tromey <tromey@adacore.com> Pushed. Thanks, Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-02-17 22:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-12 19:33 [PATCH] Do not record a rejected target description Tom Tromey 2023-02-14 15:16 ` Tom Tromey 2023-02-14 15:48 ` Andrew Burgess 2023-02-14 17:27 ` Tom Tromey 2023-02-15 12:56 ` Andrew Burgess 2023-02-15 15:59 ` Tom Tromey 2023-02-15 21:19 ` Simon Marchi 2023-02-15 21:38 ` Tom Tromey 2023-02-16 19:20 ` Tom Tromey 2023-02-16 19:33 ` Simon Marchi 2023-02-17 10:19 ` Andrew Burgess 2023-02-17 14:09 ` Tom Tromey 2023-02-17 22:29 ` 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).