* [PATCH] gdb: error out if architecture does not implement any "return_value" hook @ 2023-02-27 21:28 Simon Marchi 2023-02-28 14:50 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Simon Marchi @ 2023-02-27 21:28 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi 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 <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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook 2023-02-27 21:28 [PATCH] gdb: error out if architecture does not implement any "return_value" hook Simon Marchi @ 2023-02-28 14:50 ` Andrew Burgess 2023-02-28 16:53 ` Andrew Burgess ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Andrew Burgess @ 2023-02-28 14:50 UTC (permalink / raw) To: Simon Marchi via Gdb-patches, gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook 2023-02-28 14:50 ` Andrew Burgess @ 2023-02-28 16:53 ` Andrew Burgess 2023-02-28 19:53 ` Simon Marchi 2023-02-28 20:20 ` Pedro Alves 2 siblings, 0 replies; 6+ messages in thread From: Andrew Burgess @ 2023-02-28 16:53 UTC (permalink / raw) To: Simon Marchi via Gdb-patches, gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook 2023-02-28 14:50 ` Andrew Burgess 2023-02-28 16:53 ` Andrew Burgess @ 2023-02-28 19:53 ` Simon Marchi 2023-02-28 20:20 ` Pedro Alves 2 siblings, 0 replies; 6+ messages in thread From: Simon Marchi @ 2023-02-28 19:53 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi via Gdb-patches; +Cc: Simon Marchi >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook 2023-02-28 14:50 ` Andrew Burgess 2023-02-28 16:53 ` Andrew Burgess 2023-02-28 19:53 ` Simon Marchi @ 2023-02-28 20:20 ` Pedro Alves 2023-03-01 3:14 ` Simon Marchi 2 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2023-02-28 20:20 UTC (permalink / raw) To: Andrew Burgess, Simon Marchi via Gdb-patches; +Cc: Simon Marchi 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdb: error out if architecture does not implement any "return_value" hook 2023-02-28 20:20 ` Pedro Alves @ 2023-03-01 3:14 ` Simon Marchi 0 siblings, 0 replies; 6+ messages in thread From: Simon Marchi @ 2023-03-01 3:14 UTC (permalink / raw) To: Pedro Alves, Andrew Burgess, Simon Marchi via Gdb-patches; +Cc: Simon Marchi 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-01 3:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-27 21:28 [PATCH] gdb: error out if architecture does not implement any "return_value" hook Simon Marchi 2023-02-28 14:50 ` Andrew Burgess 2023-02-28 16:53 ` Andrew Burgess 2023-02-28 19:53 ` Simon Marchi 2023-02-28 20:20 ` Pedro Alves 2023-03-01 3:14 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).