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.129.124]) by sourceware.org (Postfix) with ESMTPS id EBD6C395A03A for ; Tue, 31 May 2022 14:31:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EBD6C395A03A 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-503-aZ25eq_FNKyq8Cn13fPv8A-1; Tue, 31 May 2022 10:31:04 -0400 X-MC-Unique: aZ25eq_FNKyq8Cn13fPv8A-1 Received: by mail-wm1-f69.google.com with SMTP id m19-20020a05600c4f5300b003974eba88c0so1385141wmq.9 for ; Tue, 31 May 2022 07:31:04 -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=UyD8n99OdWUg6Px4ICChc95se+rgNlqDEnpXXvwa9/k=; b=Xw1jyBVLkUHBpaNyTUEJyT6PZwLFtIqNDJU7VOhpTot1JduEd4CjAGzYDSgyXXKRyg yKldFkjr3iaPdKtOMCEJEx1y9EzNIo5uY/1bVOnzfcDwFjxi1gTsWilJ61Y7D7/ZQ5/6 8E3Nv9MQbXghgHwzZ333Nm+1iDJUget9JBKImGob6lsKtaq4htSLHv/2Gq3Hoxb6QJaI Nbf+og1KRYq4t1U8b5FhxLo9Btgl2XHXDqDWBweJG0yU8ZLdskuShxDH1c2HB97QAX1f eM6gdnPkO23xLTRw038I8X7uX4vXrpQEX3bSPuaCnN1kf+jvgPUQOoejZACW93ff6QkG /86w== X-Gm-Message-State: AOAM533ROHcKhf+J9VTWfqTDBfjiXWcLYqiUhASypFqgEwtq6cDCz0Dw 0QIwn+Gt7ixR8mqiznPL+eaGulc3P5+0PIwY5bxH4ABQw/76WXE8XgRVkdoLw36gJtqhpPDOtTO ff9tyVcfFrU4ApfVoSegmyHgqhnSjRpGPoBR+v98vwSVORJpD+y3lLDx1LW2f+DPiK80JL6/E5w == X-Received: by 2002:a5d:6847:0:b0:20f:c0b6:d783 with SMTP id o7-20020a5d6847000000b0020fc0b6d783mr44454160wrw.101.1654007462629; Tue, 31 May 2022 07:31:02 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxXTzsUQ3ajLhtktWprcymP/hqZxMPStuax2WENnfJHSKluRezBpZWeYJv1blhhbimLOyFyYQ== X-Received: by 2002:a5d:6847:0:b0:20f:c0b6:d783 with SMTP id o7-20020a5d6847000000b0020fc0b6d783mr44454125wrw.101.1654007462131; Tue, 31 May 2022 07:31:02 -0700 (PDT) Received: from localhost (host109-152-215-36.range109-152.btcentralplus.com. [109.152.215.36]) by smtp.gmail.com with ESMTPSA id y3-20020adffa43000000b0020d07d90b71sm9163835wrr.66.2022.05.31.07.31.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 May 2022 07:31:01 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH 5/5] gdb: native target invalid architecture detection Date: Tue, 31 May 2022 15:30:50 +0100 Message-Id: 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.6 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: Tue, 31 May 2022 14:31:12 -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 make use of this from a GDB session 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. 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. --- 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/target-delegates.c | 28 ++++++++++++++++++++++++++++ gdb/target.h | 13 ++++++++++++- 7 files changed, 82 insertions(+), 1 deletion(-) 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/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