gdb: error out if architecture does not implement any "return_value" hook

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

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.

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.

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.

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(-)
  

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.

Thanks,
Andrew

> +    predicate=True,
>  )
>  
>  Function(
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 9ed17bf4f8bc..70a00f20ba60 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -43,6 +43,7 @@
>  #include <algorithm>
>  #include "gdbsupport/scope-exit.h"
>  #include <list>
> +#include "arch-utils.h"
>  
>  /* True if we are debugging inferior calls.  */
>  
> @@ -480,6 +481,9 @@ get_call_return_value (struct call_return_meta_info *ri)
>      }
>    else
>      {
> +      if (!gdbarch_return_value_as_value_p (ri->gdbarch))
> +	  error_arch_no_return_value (ri->gdbarch);
> +
>        gdbarch_return_value_as_value (ri->gdbarch, ri->function, ri->value_type,
>  				     get_current_regcache (),
>  				     &retval, NULL);
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index e3436470b7cd..821aa2b320d3 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1487,6 +1487,9 @@ get_return_value (struct symbol *func_symbol, struct value *function)
>        return nullptr;
>      }
>  
> +  if (!gdbarch_return_value_as_value_p (gdbarch))
> +      error_arch_no_return_value (gdbarch);
> +
>    /* FIXME: 2003-09-27: When returning from a nested inferior function
>       call, it's possible (with no help from the architecture vector)
>       to locate and return/print a "struct return" value.  This is just
> @@ -1884,6 +1887,9 @@ finish_command (const char *arg, int from_tty)
>        struct type * val_type
>  	= check_typedef (sm->function->type ()->target_type ());
>  
> +      if (!gdbarch_return_value_as_value_p (gdbarch))
> +	error_arch_no_return_value (gdbarch);
> +
>        return_value
>  	= gdbarch_return_value_as_value (gdbarch,
>  					 read_var_value (sm->function, nullptr,
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 03e903d901b6..4029adfc1983 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -56,6 +56,7 @@
>  #include "cli/cli-option.h"
>  #include "cli/cli-style.h"
>  #include "gdbsupport/buildargv.h"
> +#include "arch-utils.h"
>  
>  /* The possible choices of "set print frame-arguments", and the value
>     of this setting.  */
> @@ -2813,6 +2814,9 @@ return_command (const char *retval_exp, int from_tty)
>        struct type *return_type = return_value->type ();
>        struct gdbarch *cache_arch = get_current_regcache ()->arch ();
>  
> +      if (!gdbarch_return_value_as_value_p (cache_arch))
> +	error_arch_no_return_value (cache_arch);
> +
>        gdb_assert (rv_conv != RETURN_VALUE_STRUCT_CONVENTION
>  		  && rv_conv != RETURN_VALUE_ABI_RETURNS_ADDRESS);
>        gdbarch_return_value_as_value
> diff --git a/gdb/value.c b/gdb/value.c
> index 10a7ce033fda..69e63ea79d76 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -3691,6 +3691,9 @@ struct_return_convention (struct gdbarch *gdbarch,
>    if (code == TYPE_CODE_ERROR)
>      error (_("Function return type unknown."));
>  
> +  if (!gdbarch_return_value_as_value_p (gdbarch))
> +    error_arch_no_return_value (gdbarch);
> +
>    /* Probe the architecture for the return-value convention.  */
>    return gdbarch_return_value_as_value (gdbarch, function, value_type,
>  					NULL, NULL, NULL);
> -- 
> 2.39.2
  

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
  

>> 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'.

Oops, I should have removed it probably.

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

Wow, that's bad!  I really thought it was generated, maybe it was when
trying some other combination of attributes.

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

Thanks a lot.  I agree with pretty much all you said above.  I think a
GDB port could live without a "return value" hook, but again there might
be no point for us to support that extra complexity.  I'll go look at
your series.

Simon

Simon
  

On 2023-02-28 2:50 p.m., Andrew Burgess via Gdb-patches wrote:

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

+1

It seems a bit too harsh for "finish" to error out, when it could just
not print the return value.  get_return_value already returns NULL in
a couple scenarios where we can't get at the return value, for instance.

For the "return" command, the patch is erroring out _after_ poping the frame.
There are legitimate cases where GDB won't be able to retrieve the return
value, like in the TYPE_NO_RETURN+query case handled in the function, which is
checked _before_ frame_pop.  I'd think that if we are to error out when the arch
can't write the return value, we should do it before frame_pop too.  Or we should
warn that we can't write the return value, and proceed anyhow.

For ifuncs, calling the resolver and then erroring out is probably messing
up run control.  It would probably be better to not try to resolve the ifunc
at all.

For infcalls, I don't see a reason to error out if the user did "call func()"
instead of "print func()".

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

There are legitimate cases when GDB isn't able to extract the return value.
Say, the function does not follow normal ABI.  So we should already have
code in place that gracefully handles not being able to get at the return value.
If we code it right, then I guess in most cases an extra check for whether the
arch implements the return value hook wouldn't end up complicating the code,
it would just "fit right in".  I'm not sure it's worth the bother, though.
I kind of tend to agree with you.
  

On 2/28/23 15:20, Pedro Alves wrote:
> On 2023-02-28 2:50 p.m., Andrew Burgess via Gdb-patches wrote:
> 
>> 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?
> 
> +1
> 
> It seems a bit too harsh for "finish" to error out, when it could just
> not print the return value.  get_return_value already returns NULL in
> a couple scenarios where we can't get at the return value, for instance.
> 
> For the "return" command, the patch is erroring out _after_ poping the frame.
> There are legitimate cases where GDB won't be able to retrieve the return
> value, like in the TYPE_NO_RETURN+query case handled in the function, which is
> checked _before_ frame_pop.  I'd think that if we are to error out when the arch
> can't write the return value, we should do it before frame_pop too.  Or we should
> warn that we can't write the return value, and proceed anyhow.
> 
> For ifuncs, calling the resolver and then erroring out is probably messing
> up run control.  It would probably be better to not try to resolve the ifunc
> at all.
> 
> For infcalls, I don't see a reason to error out if the user did "call func()"
> instead of "print func()".

I agree, I wrote this quickly to fix the problem, but didn't think all
this through.

>>
>> 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.
> 
> There are legitimate cases when GDB isn't able to extract the return value.
> Say, the function does not follow normal ABI.  So we should already have
> code in place that gracefully handles not being able to get at the return value.
> If we code it right, then I guess in most cases an extra check for whether the
> arch implements the return value hook wouldn't end up complicating the code,
> it would just "fit right in".  I'm not sure it's worth the bother, though.
> I kind of tend to agree with you.

Yeah, that all makes sense.

If we en up with a way for gdbarch_return_value_as_value to return "I
don't know", then it will be easy for people who write ports to start with a
dummy implementation that always returns "I don't know".  That could
even be the default value for this gdbarch hook.  In either case, GDB
core indeed won't have to care about the case where the callback is
missing.

Simon

Simon