Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> With the current GDB master, the amdgcn architectures fail to
>> initialize:
>>
>>     $ ./gdb -nx -q --data-directory=data-directory -ex "set arch amdgcn:gfx1010"
>>     /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:517: internal-error: verify_gdbarch: the following are invalid ...
>>             return_value_as_value
>>
>> This must have been because of a race condition between merging the
>> AMDGPU support and commit 4e1d2f5814b ("Add new overload of
>> gdbarch_return_value") (i.e. I probably didn't re-test when rebasing
>> over the latter).
>>
>> The new gdbarch_return_value_as_value hook as a check that verifies that
>
> typo: "...hook HAS a check..."
>
>> exactly one of return_value_as_value (the new one) and return_value (the
>> deprecated one) is set.  The intent is to catch cases where an
>> architecture would set both, which we don't want.  However, it fails to
>> consider architecture defining neither, like the amdgcn architecture in
>> today's master branch.
>>
>> The downstream port already has gdbarch_return_value implemented, and we
>> will send that upstream shortly (and we should probably migrate to
>> gdbarch_return_value_as_value before doing that), so the problem will
>> disappear then.  However, I think it would be nice to fix the situation
>> to allow ports not to define return_value nor return_value_as_value.  I
>> think this can be useful when writing new ports, to avoid having to
>> define a dummy return_value_as_value just for the gdbarch validation to
>> pass.
>
> Looking at all the places you've added error checks too, I think I
> disagree with you.
>
> It feels like this functionality (the gdbarch_return_value callbacks) is
> pretty critical to GDB, so any serious port is going to have to
> implement this sooner or later.
>
> I think it's perfectly reasonable to expect new port authors to add a
> stubbed out function while getting their new port off the ground.  And
> personally, I'd rather require new port authors to do that than have GDB
> carry around a bunch of error checks that only exist for some code that
> is mostly out of tree.
>
> [ NOTE: I know the ROCm code is in tree without the gdbarch_return_value
>   check, but I think that is because there was no error check for this
>   field when the ROCm code was first posted.  If there had been, you'd
>   have included a stub function.  ]
>
>>
>> The current behavior is to install a default return_value_as_value
>> callback (default_gdbarch_return_value) which offloads to
>> gdbarch_return_value.  Architectures that have been updated to use the
>> new callback override it to set their own return_value_as_value
>> callback.  The "invalid" predicate for return_value_as_value will then
>> flag the cases where an architecture sets both or sets neither.
>>
>> With this patch, the goal is to have a gdbarch_return_value_as_value_p
>> that returns true if either return_value_as_value or return_value is
>> set.  Make both callbacks start as nullptr, and make
>> return_value_as_value have a postdefault that installs
>> default_gdbarch_return_value if it hasn't been set but return_value has
>> been.  To summarize all cases:
>>
>>  - If the arch sets only return_value_as_value, it stays as is and
>>    gdbarch_return_value_as_value_p returns true
>>  - If the arch sets only return_value, we install
>>    default_gdbarch_return_value in return_value_as_value (which offloads
>>    to return_value) and gdbarch_return_value_as_value_p returns true
>>  - If the arch sets neither, both fields stay nullptr and
>>    gdbarch_return_value_as_value_p returns false
>>  - If the arch sets both, we unfortunately don't flag it as an error, as
>>    we do today.  The current implementation of gdbarch.py doesn't let us
>>    have an "invalid" check at the same time as a predicate function, for
>>    some reason.  But gdbarch_return_value_as_value_p will return true
>>    in that case.
>
> As I understand it the gdbarch.py algorithm was just copied over from
> the old shell scripts.  The old shell script algorithm was, I suspect
> rather hacked together.
>
> The comments around this code hint that and "invalid" check doesn't make
> sense when we have a predicate because the predicate would just return
> false for an invalid value, so why would you want an earlier validity
> check.
>
> I think I disagree with this reasoning, and think it makes perfect sense
> to offer both a validity check and a predicate for the same component.
> I'd be happy to see us move in this direction.
>
>>
>> With that in place, add some checks before all uses of
>> gdbarch_return_value_as_value.  On failure, call
>> error_arch_no_return_value, which throws with a standard error
>> message.
>
> This is another area where I think the gdbarch.py generation code could
> be improved.  It seems a shame that we need to remember to place a
> predicate check before every call to gdbarch_return_value_as_value,
> surely it would be better if we could inject this error check directly
> into the generated gdbarch_return_value_as_value code?  We already have
> 'param_checks' and 'result_checks' which are really for adding asserts,
> but maybe we could/should add some new field that would trigger an error
> call.
>
>>
>> I don't see a good way of testing this.  I manually tested it using the
>> downstream ROCm-GDB port, removed the return_value hook from the amdgcn
>> arch, and tried to "finish" a function:
>>
>>     (gdb) finish
>>     Architecture amdgcn:gfx900 does not implement extracting return values from the inferior.
>>
>> This hits the gdbarch_return_value_as_value_p call in finish_command
>> (which triggers when trying to figure out the return value convention,
>> before resuming the inferior).  I don't know how if the other errors
>> paths I added leave GDB in a sane state though, they are a bit more
>> difficult to test.
>
> If almost feels like we should push these error checks to the beginning
> of the command, e.g. when the user tries to 'finish' we error before
> doing anything because we know we can't extract the return value.
>
> Or maybe we shouldn't be throwing an error in some of these cases, maybe
> in some cases we could warn and continue, just without the return value
> fetching?
>
> As I said above, I don't actually like GDB trying to handling this case
> at all, I'd much rather we just force the port to stub these functions
> during bring up.
>
>>
>> Change-Id: I8e0728bf9c42520dac7952bfa3f6f50c5737fa7b
>> ---
>>  gdb/arch-utils.c          | 11 +++++++++++
>>  gdb/arch-utils.h          |  6 ++++++
>>  gdb/elfread.c             |  4 ++++
>>  gdb/gdbarch-gen.h         |  2 ++
>>  gdb/gdbarch.c             | 15 ++++++++++++---
>>  gdb/gdbarch_components.py |  4 ++--
>>  gdb/infcall.c             |  4 ++++
>>  gdb/infcmd.c              |  6 ++++++
>>  gdb/stack.c               |  4 ++++
>>  gdb/value.c               |  3 +++
>>  10 files changed, 54 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
>> index e3af9ce2dbce..1282b4f8a773 100644
>> --- a/gdb/arch-utils.c
>> +++ b/gdb/arch-utils.c
>> @@ -1478,6 +1478,17 @@ target_gdbarch (void)
>>    return current_inferior ()->gdbarch;
>>  }
>>  
>> +/* See arch-utils.h.  */
>> +
>> +void
>> +error_arch_no_return_value (gdbarch *arch)
>> +{
>> +  const bfd_arch_info *info = gdbarch_bfd_arch_info (arch);
>> +
>> +  error (_("Architecture %s does not implement extracting return values from "
>> +	   "the inferior."), info->printable_name);
>> +}
>> +
>>  void _initialize_gdbarch_utils ();
>>  void
>>  _initialize_gdbarch_utils ()
>> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
>> index 56690f0fd435..d00753ec32b4 100644
>> --- a/gdb/arch-utils.h
>> +++ b/gdb/arch-utils.h
>> @@ -314,4 +314,10 @@ extern enum return_value_convention default_gdbarch_return_value
>>        struct regcache *regcache, struct value **read_value,
>>        const gdb_byte *writebuf);
>>  
>> +/* Error out with a message saying that ARCH does not support extracting return
>> +   values from the inferior.  */
>> +
>> +extern void error_arch_no_return_value (gdbarch *arch)
>> +  ATTRIBUTE_NORETURN;
>> +
>>  #endif /* ARCH_UTILS_H */
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index ca684aab57ea..24df62b1fccb 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -1038,6 +1038,10 @@ elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
>>    func_func->set_address (b->loc->related_address);
>>  
>>    value = value::allocate (value_type);
>> +
>> +  if (!gdbarch_return_value_as_value_p (gdbarch))
>> +    error_arch_no_return_value (gdbarch);
>> +
>>    gdbarch_return_value_as_value (gdbarch, func_func, value_type, regcache,
>>  				 &value, NULL);
>>    resolved_address = value_as_address (value);
>> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
>> index ddb97f60315f..00e6960f9cef 100644
>> --- a/gdb/gdbarch-gen.h
>> +++ b/gdb/gdbarch-gen.h
>> @@ -453,6 +453,8 @@ extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_va
>>     to force the value returned by a function (see the "return" command
>>     for instance). */
>>  
>> +extern bool gdbarch_return_value_as_value_p (struct gdbarch *gdbarch);
>> +
>>  typedef enum return_value_convention (gdbarch_return_value_as_value_ftype) (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf);
>>  extern enum return_value_convention gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf);
>>  extern void set_gdbarch_return_value_as_value (struct gdbarch *gdbarch, gdbarch_return_value_as_value_ftype *return_value_as_value);
>> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
>> index 7e4e34d5aca0..42c4f16a67c8 100644
>> --- a/gdb/gdbarch.c
>> +++ b/gdb/gdbarch.c
>> @@ -112,7 +112,7 @@ struct gdbarch
>>    gdbarch_address_to_pointer_ftype *address_to_pointer = unsigned_address_to_pointer;
>>    gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
>>    gdbarch_return_value_ftype *return_value = nullptr;
>> -  gdbarch_return_value_as_value_ftype *return_value_as_value = default_gdbarch_return_value;
>> +  gdbarch_return_value_as_value_ftype *return_value_as_value = nullptr;
>>    gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = default_get_return_buf_addr;
>>    gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
>>    gdbarch_skip_prologue_ftype *skip_prologue = 0;
>> @@ -367,8 +367,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>>    /* Skip verify of address_to_pointer, invalid_p == 0 */
>>    /* Skip verify of integer_to_address, has predicate.  */
>>    /* Skip verify of return_value, invalid_p == 0 */
>> -  if ((gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr))
>> -    log.puts ("\n\treturn_value_as_value");
>> +  /* Skip verify of return_value_as_value, has predicate.  */
>>    /* Skip verify of get_return_buf_addr, invalid_p == 0 */
>>    /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
>>    if (gdbarch->skip_prologue == 0)
>> @@ -778,6 +777,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>>    gdb_printf (file,
>>  	      "gdbarch_dump: return_value = <%s>\n",
>>  	      host_address_to_string (gdbarch->return_value));
>> +  gdb_printf (file,
>> +	      "gdbarch_dump: gdbarch_return_value_as_value_p() = %d\n",
>> +	      gdbarch_return_value_as_value_p (gdbarch));
>>    gdb_printf (file,
>>  	      "gdbarch_dump: return_value_as_value = <%s>\n",
>>  	      host_address_to_string (gdbarch->return_value_as_value));
>> @@ -2574,6 +2576,13 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
>>    gdbarch->return_value = return_value;
>>  }
>>  
>> +bool
>> +gdbarch_return_value_as_value_p (struct gdbarch *gdbarch)
>> +{
>> +  gdb_assert (gdbarch != NULL);
>> +  return gdbarch->return_value_as_value != NULL;
>> +}
>> +
>>  enum return_value_convention
>>  gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf)
>>  {
>> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
>> index caa65c334eca..7ceecbf5d223 100644
>> --- a/gdb/gdbarch_components.py
>> +++ b/gdb/gdbarch_components.py
>> @@ -902,11 +902,11 @@ for instance).
>>          ("struct value **", "read_value"),
>>          ("const gdb_byte *", "writebuf"),
>>      ],
>> -    predefault="default_gdbarch_return_value",
>>      # If we're using the default, then the other method must be set;
>>      # but if we aren't using the default here then the other method
>>      # must not be set.
>
> I don't think this comment aligns with the postdefault code.  It says
> "If we're using the default, ..." but "we" here is
> 'return_value_as_value', but we're actually checking 'return_value'.
>
>> -    invalid="(gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr)",
>> +    postdefault="gdbarch->return_value != nullptr ? default_gdbarch_return_value : nullptr",
>
> If you search for the postdefault string you'll notice this code is not
> actually generated anywhere!  This is a consequence of also having
> defined a predicate.
>
> As I say above, the gdbarch.py algorithm could do with some updating in
> a few cases.
>
> The good news is, I already have a patch that fixes this problem.  I was
> going to include a link to it here, but turns out I never actually
> posted it!  I'm going to try to get that post on the list today, I've
> tried it locally, and with my patch your postdefault code is generated
> correctly.

Got the patch posted, its this one:

https://sourceware.org/pipermail/gdb-patches/2023-February/197537.html

Thanks,
Andrew