From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 46F5838293C6 for ; Fri, 10 Jun 2022 13:08:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 46F5838293C6 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-342-wNu5sGvYN-msjFaDFDlBsw-1; Fri, 10 Jun 2022 09:08:31 -0400 X-MC-Unique: wNu5sGvYN-msjFaDFDlBsw-1 Received: by mail-wm1-f69.google.com with SMTP id p22-20020a05600c359600b0039c7b23a1c7so185861wmq.2 for ; Fri, 10 Jun 2022 06:08:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=qarmD1Tnz+v7vLoF+X6Li5/Aa/ElRIxLUEYnmTnK9qU=; b=S7aRs9iN8SsTJx043LzgKhMl+GLTjDhJ9fzsBmIpUmHFVF55GhF2GSAYRweNiXQxOn uxa7eBQdOUo9UBXCnCWitjEfAYJXrvyY39PfAuOObwV2owFnoyR5/vFtp3jVEpmzzEHQ eYpJ2YuWCHZHbDr433EikGQcu6OuQKS0vvzQYqbZ1BLGXMfcpkauOkJwtl6WRuToyL3p TVqBPlZCZkK06B3ky+yC8mTTZ/JbnHLXbNE+4hTT/neOElyspZrxGnGF4O10PxiPmyOc CdMeGNW+9nfoCdgr2Vf9i/SC950Kp1u41itfWd9JFy9jye7ATzCbWD/GjUhtM418/X8i ykpA== X-Gm-Message-State: AOAM533qcX1aCHoOsP/xjUKPz08jFVUK61Y/Cnh2P3cz3n5KM5wq2CmV qqK2MRNkSw6vB7NJHjg1bo08ySYTtLPUa5+3fVQ/oW9pNcamCcvNO33ZXBk6hqW3vnctru4yliY llICPXOB+EP5GF7epI8hqAeYkZdgTPEk/3lZV474RUXbV8zNvbqtc7BgMDJ3ykWW8ZjGmOaLfyg == X-Received: by 2002:a1c:f416:0:b0:39c:4778:74cd with SMTP id z22-20020a1cf416000000b0039c477874cdmr8648348wma.128.1654866509777; Fri, 10 Jun 2022 06:08:29 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwke460DvRJzh/j6mjR26zKvRUPcv/5Rd4d/Q9CWEiP23qr5WcA6YRB0If8IGzPT5WFyvvI/w== X-Received: by 2002:a1c:f416:0:b0:39c:4778:74cd with SMTP id z22-20020a1cf416000000b0039c477874cdmr8648294wma.128.1654866509089; Fri, 10 Jun 2022 06:08:29 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id p4-20020a05600c430400b0039c3b05540fsm2795891wme.27.2022.06.10.06.08.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jun 2022 06:08:28 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv2 6/6] gdb: native target invalid architecture detection Date: Fri, 10 Jun 2022 14:08:14 +0100 Message-Id: <644591dc859ea741999f73aaa28a1bffa83ceab5.1654866188.git.aburgess@redhat.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE 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-List-Received-Date: Fri, 10 Jun 2022 13:08:36 -0000 If GDB is asked to start a new inferior, or attach to an existing process, using a binary file for an architecture that does not match the current native target, then, currently, GDB will assert. Here's an example session using current HEAD of master with GDB built for an x86-64 GNU/Linux native target, the binary being used is a RISC-V ELF: $ ./gdb/gdb -q --data-directory ./gdb/data-directory/ (gdb) file /tmp/hello.rv32imc.x Reading symbols from /tmp/hello.rv32imc.x... (gdb) start Temporary breakpoint 1 at 0x101b2: file hello.rv32.c, line 23. Starting program: /tmp/hello.rv32imc.x ../../src/gdb/gdbarch.h:166: internal-error: gdbarch_tdep: Assertion `dynamic_cast (tdep) != nullptr' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. The same error is encountered if, instead of starting a new inferior, the user tries to attach to an x86-64 process with a RISC-V binary set as the current executable. These errors are not specific to the x86-64/RISC-V pairing I'm using here, any attempt to use a binary for one architecture with a native target of a different architecture will result in a similar error. Clearly, attempting to use this cross-architecture combination is a user error, but I think GDB should do better than an assert; ideally a nice error should be printed. The problem we run into is that, when the user starts a new inferior, or attaches to an inferior, the inferior stops. At this point GDB attempts to handle the stop, and this involves reading registers from the inferior. These register reads end up being done through the native target, so in the example above, we end up in the amd64_supply_fxsave function. However, these functions need a gdbarch. The gdbarch is fetched from the register set, which was constructed using the gdbarch from the binary currently in use. And so we end up in amd64_supply_fxsave using a RISC-V gdbarch. When we call: i386_gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); this will assert as the gdbarch_tdep data within the RISC-V gdbarch is of the type riscv_gdbarch_tdep not i386_gdbarch_tdep. The solution I propose in this commit is to add a new target_ops method supports_architecture_p. This method will return true if a target can safely be used with a specific architecture, otherwise, the method returns false. I imagine that a result of true from this method doesn't guarantee that GDB can start an inferior of a given architecture, it just means that GDB will not crash if such an attempt is made. A result of false is a hard stop; attempting to use this target with this architecture is not supported, and may cause GDB to crash. This distinction is important I think for things like remote targets, and possibly simulator targets. We might imagine that GDB can ask a remote (or simulator) to start with a particular executable, and the target might still refuse for some reason. But my thinking is that these refusals should be well handled (i.e. GDB should give a user friendly error), rather than crashing, as is the case with the native targets. For example, if I start gdbserver on an x86-64 machine like this: gdbserver --multi :54321 Then use GDB to try and load a RISC-V binary, like this: $ ./gdb/gdb -q --data-directory ./gdb/data-directory/ (gdb) file /tmp/hello.rv32imc.x Reading symbols from /tmp/hello.rv32imc.x... (gdb) target extended-remote :54321 Remote debugging using :54321 (gdb) run Starting program: /tmp/hello.rv32imc.x Running the default executable on the remote target failed; try "set remote exec-file"? (gdb) Though the error is not very helpful in diagnosing the problem, we can see that GDB has not crashed, but has given the user an error. And so, the supports_architecture_p method is created to return true by default, then I override this in inf_child_target, where I compare the architecture in question with the default_bfd_arch. Finally, I've added calls to supports_architecture_p for the run (which covers run, start, starti) and attach commands. This leaves just one question, what about native targets that support multiple architectures? These targets can be split into two groups. First, we have targets like x86-64, which also supports i386 binaries. This case is easy to handle, as far as BFD is concerned there is only one architecture, bfd_arch_i386, and we then use machine types to split this architecture into x86-64 and i386 (and others). As the new supports_architecture_p function only checks the bfd architecture, then there is nothing additional needed for this case. The second group of multi-architecture targets requires more work. The only targets that I'm aware of that fall into this group are the rs6000-aix-nat.c, ppc-*-nat.c targets, and the aarch64-linux-nat.c target. The first group (rs600/ppc) support bfd_arch_rs6000 and bfd_arch_powerpc, while the second (aarch64) supports bfd_arch_arm and bfd_arch_aarch64. To deal with these targets I have overridden the supports_architecture_p function in each of the separate target files, these overrides check both of the supported architectures. One final note, in the rs6000/ppc case, the FreeBSD target supports both architectures, and so we override supports_architecture_p. In contrast, the aarch64_fbsd_nat_target target does not (yet) support bfd_arch_arm, and so there is no supports_architecture_p here. This can always be added later if/when support is added. You will notice a lack of tests for this change. I'm not sure of a good way that I can build a binary for a different architecture as part of a test, but if anyone has any ideas then I'll be happy to add a test here. The gdb.base/multi-arch.exp test exists, which for AArch64 will test compiling and running something as both AArch64 and ARM, but this doesn't cover the error case, just that the overridden supports_architecture_p works in that case. --- gdb/aarch64-linux-nat.c | 8 ++++++++ gdb/arch-utils.c | 8 ++++++++ gdb/arch-utils.h | 5 +++++ gdb/inf-child.c | 19 +++++++++++++++++++ gdb/inf-child.h | 2 ++ gdb/infcmd.c | 8 ++++++++ gdb/ppc-fbsd-nat.c | 8 ++++++++ gdb/ppc-linux-nat.c | 8 ++++++++ gdb/ppc-obsd-nat.c | 8 ++++++++ gdb/rs6000-aix-nat.c | 8 ++++++++ gdb/target-delegates.c | 28 ++++++++++++++++++++++++++++ gdb/target.h | 13 ++++++++++++- 12 files changed, 122 insertions(+), 1 deletion(-) diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index a457fcd48ad..6114c4c6638 100644 --- a/gdb/aarch64-linux-nat.c +++ b/gdb/aarch64-linux-nat.c @@ -108,6 +108,14 @@ class aarch64_linux_nat_target final /* Write allocation tags to memory via PTRACE. */ bool store_memtags (CORE_ADDR address, size_t len, const gdb::byte_vector &tags, int type) override; + + /* This target supports two architectures, check for them both here. */ + + bool supports_architecture_p (struct gdbarch *gdbarch) override + { + bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch; + return (the_arch == bfd_arch_aarch64 || the_arch == bfd_arch_arm); + } }; static aarch64_linux_nat_target the_aarch64_linux_nat_target; diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c index 360b8d694be..efba17e1d5d 100644 --- a/gdb/arch-utils.c +++ b/gdb/arch-utils.c @@ -1570,6 +1570,14 @@ target_gdbarch (void) return current_inferior ()->gdbarch; } +/* See arch-utils.h. */ + +bool +gdbarch_matches_default_arch (struct gdbarch *gdbarch) +{ + return gdbarch_bfd_arch_info (gdbarch)->arch == default_bfd_arch->arch; +} + void _initialize_gdbarch_utils (); void _initialize_gdbarch_utils () diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h index f850e5fd6e7..5d61be36c26 100644 --- a/gdb/arch-utils.h +++ b/gdb/arch-utils.h @@ -300,4 +300,9 @@ extern void default_read_core_file_mappings struct bfd *cbfd, read_core_file_mappings_pre_loop_ftype pre_loop_cb, read_core_file_mappings_loop_ftype loop_cb); + +/* Return true if the bfd architecture of GDBARCH matches the default bfd + architecture. */ + +extern bool gdbarch_matches_default_arch (struct gdbarch *gdbarch); #endif /* ARCH_UTILS_H */ diff --git a/gdb/inf-child.c b/gdb/inf-child.c index 56ebd2a5549..0c47a367c68 100644 --- a/gdb/inf-child.c +++ b/gdb/inf-child.c @@ -30,6 +30,8 @@ #include "inferior.h" #include #include "inf-child.h" +#include "gdbarch.h" +#include "arch-utils.h" #include "gdbsupport/fileio.h" #include "gdbsupport/agent.h" #include "gdbsupport/gdb_wait.h" @@ -412,6 +414,23 @@ inf_child_target::follow_exec (inferior *follow_inf, ptid_t ptid, } } +/* The inf_child_target represents the native target built into GDB, of + which there is only ever one. If we attempt to start a native inferior + using a binary for an architecture not matching the native target then, + when we handle the first stop, we will end up trying to read registers + using the gdbarch functions from the native target, but passing in a + gdbarch object based on the architecture of the binary file. This will + result in errors. + + This check prevents this so long as everywhere user command that might + cause a new inferior to be created calls this function. */ + +bool +inf_child_target::supports_architecture_p (struct gdbarch *gdbarch) +{ + return gdbarch_matches_default_arch (gdbarch); +} + /* See inf-child.h. */ void diff --git a/gdb/inf-child.h b/gdb/inf-child.h index ae5ace46f30..01a8164b0c2 100644 --- a/gdb/inf-child.h +++ b/gdb/inf-child.h @@ -92,6 +92,8 @@ class inf_child_target bool can_use_agent () override; + bool supports_architecture_p (struct gdbarch *gdbarch) override; + protected: /* Unpush the target if it wasn't explicitly open with "target native" and there are no live inferiors left. Note: if calling this as a diff --git a/gdb/infcmd.c b/gdb/infcmd.c index e909d4d4c81..b8acf858b3a 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -413,6 +413,10 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) if (non_stop && !run_target->supports_non_stop ()) error (_("The target does not support running in non-stop mode.")); + if (!run_target->supports_architecture_p (get_current_arch ())) + error (_("The target does not support architecture \"%s\"."), + gdbarch_bfd_arch_info (get_current_arch ())->printable_name); + /* Done. Can now set breakpoints, change inferior args, etc. */ /* Insert temporary breakpoint in main function if requested. */ @@ -2590,6 +2594,10 @@ attach_command (const char *args, int from_tty) if (non_stop && !attach_target->supports_non_stop ()) error (_("Cannot attach to this target in non-stop mode")); + if (!attach_target->supports_architecture_p (get_current_arch ())) + error (_("The target does not support architecture \"%s\"."), + gdbarch_bfd_arch_info (get_current_arch ())->printable_name); + attach_target->attach (args, from_tty); /* to_attach should push the target, so after this point we shouldn't refer to attach_target again. */ diff --git a/gdb/ppc-fbsd-nat.c b/gdb/ppc-fbsd-nat.c index d0a5778e2d3..f7c36c33958 100644 --- a/gdb/ppc-fbsd-nat.c +++ b/gdb/ppc-fbsd-nat.c @@ -41,6 +41,14 @@ struct ppc_fbsd_nat_target final : public fbsd_nat_target { void fetch_registers (struct regcache *, int) override; void store_registers (struct regcache *, int) override; + + /* This target supports two architectures, check for them both here. */ + + bool supports_architecture_p (struct gdbarch *gdbarch) override + { + bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch; + return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc); + } }; static ppc_fbsd_nat_target the_ppc_fbsd_nat_target; diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c index de4158c411a..e302b15925a 100644 --- a/gdb/ppc-linux-nat.c +++ b/gdb/ppc-linux-nat.c @@ -551,6 +551,14 @@ struct ppc_linux_nat_target final : public linux_nat_target void low_prepare_to_resume (struct lwp_info *) override; + /* This target supports two architectures, check for them both here. */ + + bool supports_architecture_p (struct gdbarch *gdbarch) override + { + bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch; + return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc); + } + private: void copy_thread_dreg_state (const ptid_t &parent_ptid, diff --git a/gdb/ppc-obsd-nat.c b/gdb/ppc-obsd-nat.c index e480f19dc73..2fdccc3085d 100644 --- a/gdb/ppc-obsd-nat.c +++ b/gdb/ppc-obsd-nat.c @@ -39,6 +39,14 @@ struct ppc_obsd_nat_target final : public obsd_nat_target { void fetch_registers (struct regcache *, int) override; void store_registers (struct regcache *, int) override; + + /* This target supports two architectures, check for them both here. */ + + bool supports_architecture_p (struct gdbarch *gdbarch) override + { + bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch; + return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc); + } }; static ppc_obsd_nat_target the_ppc_obsd_nat_target; diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c index 8697d27b4ed..603caa8fb1e 100644 --- a/gdb/rs6000-aix-nat.c +++ b/gdb/rs6000-aix-nat.c @@ -91,6 +91,14 @@ class rs6000_nat_target final : public inf_ptrace_target ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override; + /* This target supports two architectures, check for them both here. */ + + bool supports_architecture_p (struct gdbarch *gdbarch) override + { + bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch; + return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc); + } + protected: void post_startup_inferior (ptid_t ptid) override diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 8a9986454dd..cb7bb1f2062 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -196,6 +196,7 @@ struct dummy_target : public target_ops bool supports_memory_tagging () override; bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; + bool supports_architecture_p (struct gdbarch *arg0) override; }; struct debug_target : public target_ops @@ -370,6 +371,7 @@ struct debug_target : public target_ops bool supports_memory_tagging () override; bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override; bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override; + bool supports_architecture_p (struct gdbarch *arg0) override; }; void @@ -4536,3 +4538,29 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector return result; } +bool +target_ops::supports_architecture_p (struct gdbarch *arg0) +{ + return this->beneath ()->supports_architecture_p (arg0); +} + +bool +dummy_target::supports_architecture_p (struct gdbarch *arg0) +{ + return true; +} + +bool +debug_target::supports_architecture_p (struct gdbarch *arg0) +{ + bool result; + gdb_printf (gdb_stdlog, "-> %s->supports_architecture_p (...)\n", this->beneath ()->shortname ()); + result = this->beneath ()->supports_architecture_p (arg0); + gdb_printf (gdb_stdlog, "<- %s->supports_architecture_p (", this->beneath ()->shortname ()); + target_debug_print_struct_gdbarch_p (arg0); + gdb_puts (") = ", gdb_stdlog); + target_debug_print_bool (result); + gdb_puts ("\n", gdb_stdlog); + return result; +} + diff --git a/gdb/target.h b/gdb/target.h index 18559feef89..5f3ae77f559 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1320,7 +1320,18 @@ struct target_ops virtual bool store_memtags (CORE_ADDR address, size_t len, const gdb::byte_vector &tags, int type) TARGET_DEFAULT_NORETURN (tcomplain ()); - }; + + /* Return false if the target does not support GDBARCH, otherwise, + return true. + + The default reply of true does not guarantee that debugging an + inferior with architecture GDBARCH will succeed, other errors might + be thrown later on, but a return value of false is a clear + indication that we should not proceed attempting to use this + architecture with this target. */ + virtual bool supports_architecture_p (struct gdbarch *gdbarch) + TARGET_DEFAULT_RETURN (true); +}; /* Deleter for std::unique_ptr. See comments in target_ops::~target_ops and target_ops::close about heap-allocated -- 2.25.4