[PATCH] Do not record a rejected target description

[PATCH] Do not record a rejected target description

[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

Re: [PATCH] Do not record a rejected target description

[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

Re: [PATCH] Do not record a rejected target description

[Andrew Burgess]

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

Re: [PATCH] Do not record a rejected target description

[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

Re: [PATCH] Do not record a rejected target description

[Andrew Burgess]

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

Re: [PATCH] Do not record a rejected target description

[Tom Tromey]

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

Re: [PATCH] Do not record a rejected target description

[Simon Marchi]

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

Re: [PATCH] Do not record a rejected target description

[Tom Tromey]

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

Re: [PATCH] Do not record a rejected target description

[Tom Tromey]

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"

Re: [PATCH] Do not record a rejected target description

[Simon Marchi]

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

Re: [PATCH] Do not record a rejected target description

[Andrew Burgess]

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"

Re: [PATCH] Do not record a rejected target description

[Tom Tromey]

>>>>> "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

Re: [PATCH] Do not record a rejected target description

[Andrew Burgess]

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