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

Simon Marchi via Gdb-patches
2023-02-27T21:28:39Z

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

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.
- invalid="(gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr)",
+ postdefault="gdbarch->return_value != nullptr ? default_gdbarch_return_value : nullptr",
+ 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
+#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