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

Andrew Burgess via Gdb-patches
2023-02-28T16:53:52Z

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