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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

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