From patchwork Mon Feb 27 21:28:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 65701 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 315D93858C52 for ; Mon, 27 Feb 2023 21:28:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 315D93858C52 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1677533321; bh=hZ46lCoh5YN6vrZIZx5PFSXuNyyC7ajwGxblGL7ZiDA=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=G12lsOxpF80b4z4H8sieGMn+DlzCoGPXhyp89dzIlp7M2Y97IJGKnxlaLq8UKdNvD m8FgMWo/4W3UDndhanrBsjsemaBETJpj/LoO2rojN49F08xtlY/PWP8lfFxQEUP7Nf 2Po8i5Cp2wHlqQVwe79FFuhpgJKBTMwLjngS9WNo= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id BDA7D3858C2B for ; Mon, 27 Feb 2023 21:28:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BDA7D3858C2B Received: from smarchi-efficios.internal.efficios.com (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1BF3D1E110; Mon, 27 Feb 2023 16:28:13 -0500 (EST) To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH] gdb: error out if architecture does not implement any "return_value" hook Date: Mon, 27 Feb 2023 16:28:06 -0500 Message-Id: <20230227212806.68474-1-simon.marchi@efficios.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-Spam-Status: No, score=-3497.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_SOFTFAIL, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" 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 #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);