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 4CF9E3833796 for ; Tue, 28 Jun 2022 10:37:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4CF9E3833796 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-374-amfkTFDgPnab9Md8-EdRgw-1; Tue, 28 Jun 2022 06:37:34 -0400 X-MC-Unique: amfkTFDgPnab9Md8-EdRgw-1 Received: by mail-wm1-f69.google.com with SMTP id r4-20020a1c4404000000b003a02fa133ceso846869wma.2 for ; Tue, 28 Jun 2022 03:37:34 -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:subject:in-reply-to:references:date :message-id:mime-version; bh=ofPMZ+BMMoFIun/zVQYmJs3kq1oJEIckULJwB7bcA3k=; b=B9Hp3hMmHT7cXwBWP8NNAyPnBmS76WadHmdp0qXIbp8ygsf01DeIar1HvGm5pYFvob 0mp1xb0+jWKWXHGIMjkj33bFdLRg8vzxA1X4+hfAdZ8KuGdauLFm0OlJBvMcZ7NCxClv nwqWXSyRpWF0vPFbpH/7GwX0IBzggwnSGM/ZpWPjDqU+minhLnkImnKAWxRO1+shvzy8 SRiSBhxQbgtNd8dcqwbfhRFEEfDc8DMtOVcECgenORuL0WbE/WULSOdRBdf6yUpEiyg4 mOa60uz1BZCOTytI4au+8r2tsJ4zg+f6jRCArQ6Vur2x9aozB+6kxAOFGLiLThDfN5u7 vGsA== X-Gm-Message-State: AJIora/6vQcQahdvJSnp6Umck0Fsg8a4L+VT549As/yO7XWF7I2dxmHW nV4YMIh6z6vQcwKtbf1RdJyie6uPEjFihWI7QbwYBifFFXqf84FRvO9n9cOaPoB2YyEdds3/twO e7EumLf0P/dz+XO2BRLukVA== X-Received: by 2002:adf:ecc6:0:b0:21a:3dca:4dd7 with SMTP id s6-20020adfecc6000000b0021a3dca4dd7mr16522660wro.366.1656412653299; Tue, 28 Jun 2022 03:37:33 -0700 (PDT) X-Google-Smtp-Source: AGRyM1uaN16pwKuhfrOCQXejffk2eMVgnvHxehBdeKU6hUdWVzv1p2wmojHvrerH51lbSXRNC6nz9Q== X-Received: by 2002:adf:ecc6:0:b0:21a:3dca:4dd7 with SMTP id s6-20020adfecc6000000b0021a3dca4dd7mr16522634wro.366.1656412653010; Tue, 28 Jun 2022 03:37:33 -0700 (PDT) Received: from localhost (15.72.115.87.dyn.plus.net. [87.115.72.15]) by smtp.gmail.com with ESMTPSA id i6-20020adfe486000000b0021b892f4b35sm13637022wrm.98.2022.06.28.03.37.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Jun 2022 03:37:32 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection In-Reply-To: <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> References: <83ceec06f74f4ce6a2dc8d794031fb233567ba6d.1655136816.git.aburgess@redhat.com> <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> <87fsjqdqco.fsf@redhat.com> <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> Date: Tue, 28 Jun 2022 11:37:31 +0100 Message-ID: <87a69xdqfo.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.3 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, 28 Jun 2022 10:37:39 -0000 Pedro Alves writes: > On 2022-06-27 17:27, Andrew Burgess wrote: >> Pedro Alves writes: >>> On 2022-06-13 17:15, Andrew Burgess via Gdb-patches wrote: >>> I have another question. I'm confused on how if e.g., on x86-64, you >>> load a RISC-V binary into GDB, and the try to run it, we don't end up >>> with architecture of the target description instead. >> >> The problem is that we end up trying to read the registers before we >> read the target description. Which I suspect you will say sounds wrong, >> and I agree. >> >> I removed the error() calls from this patch, and reran GDB, here's the >> stack at the point of failure: >> > > ... > >> #11 0x0000000000bf43c4 in regcache_read_pc (regcache=0x30f5f10) at ../../src/gdb/regcache.c:1325 >> #12 0x00000000009c2e19 in save_stop_reason (lp=0x3a8ff80) at ../../src/gdb/linux-nat.c:2545 >> #13 0x00000000009c40f0 in linux_nat_filter_event (lwpid=3990223, status=1407) at ../../src/gdb/linux-nat.c:3003 >> #14 0x00000000009c471a in linux_nat_wait_1 (ptid=..., ourstatus=0x7fffffffa4f0, target_options=...) >> at ../../src/gdb/linux-nat.c:3178 >> #15 0x00000000009c535a in linux_nat_target::wait (this=0x2d27140 , ptid=..., >> ourstatus=0x7fffffffa4f0, target_options=...) at ../../src/gdb/linux-nat.c:3416 >> #16 0x0000000000dc5f3b in target_wait (ptid=..., status=0x7fffffffa4f0, options=...) >> at ../../src/gdb/target.c:2601 >> #17 0x0000000000abd3c4 in startup_inferior (proc_target=0x2d27140 , pid=3990223, >> ntraps=1, last_waitstatus=0x0, last_ptid=0x0) at ../../src/gdb/nat/fork-inferior.c:482 > > Alright, we're inside startup_inferior. > >> >> The initial architecture is riscv:rv32, and it is this architecture that >> is used for handling the initial stop. >> >> So my first thought was, pretty much as you suggest, that we should >> fetch the target description earlier, then we'll not run into this >> problem at all. >> > > I don't think we should fetch the tdesc earlier, I think that would be incorrect. Instead, > we should avoid reading registers while going through the shell. The shell's registers have > nothing to do with the final program's registers. The shell may even be of a different > architecture from the final inferior's architecture. Like, if your shell is a 64-bit > bash, and then the inferior you're going to debug, which the shell execs, is a 32-bit program. > >> My problem was figuring out the right place to add such a call. >> >> We know it has to be before we enter regcache_read_pc, as at this point >> the regcache is already created, and has the wrong architecture. >> >> And it can't really be earlier than linux_nat_filter_event I think, as >> before this point we don't know that the target has stopped. We can't >> probe the target for features (I would assume) if the target is not >> currently stopped. >> >> This seems to leave either save_stop_reason, or in the early stages of >> get_thread_regcache maybe. >> >> The problem with save_stop_reason is that it's Linux specific, so any >> non-Linux targets will need to add their own code to catch this >> situation. > > I think we should change save_stop_reason. But, change it to avoid reading > the PC instead. > > I don't think that the fact that other targets need to handle this too > is an issue. > >>> It kind of sounds like if GDB detected the incompatibility when we first fetch the >>> target description, similarly to how we detect it in the cases above, and GDB picked >>> the target arch or errored out after detecting the incompatibility, then we wouldn't >>> need a patch like yours? >> >> I agree. One other aspect is, do we have any native targets that don't >> support reading a target description? Hopefully not. > > If we do, we have an action plan -- add such support. I think a tdesc can be just an > element, for instance. > >> >>> How come the mismatch isn't detected then (when we first >>> retrieve the target description) today? >> >> Hopefully the above makes that clear, but the TLDR answer is: we attempt >> to read the registers using the current architecture before we fetch the >> target description. >> >> I suspected that, in part, this patch would be a bit of a straw-man, I'd >> appreciate any insight you have for how we might reorder things so that >> the target description can be read before we try to read the registers. >> > > So I gave the patch below a try, and it seems to works well -- readcache_read_pc > is no longer called before target_find_description, and I saw no testsuite > regressions (this is without your series, just pristine master). > > Now, this patch as is makes sense for linux-nat.c, but then I went to do the > same thing for gdbserver, which has an equivalent save_stop_reason in linux-low.cc, > and there this approach won't work as is for the simple fact that lwp->stop_pc > is used in a lot more places. > > So to avoid gdb vs gdbserver divergence, it may be better to not take this patch > as is, but instead tweak save_stop_reason to avoid reading the stop pc while > the inferior is going through the shell, based on some per-inferior flag or some > such. Maybe we can repurpose progspace->executing_startup for this? That > flag used to be used by Cell (which is gone), but seems unused nowadays. There's > also other similar flags in inferior itself, like needs_setup, and > in_initial_library_scan. So we'd add a flag like one of those, set it while the > inferior is going through the startup_inferior dance, and make save_stop_reason avoid > reading the PC while that flag is set. > > Note the patch below applies on top of: > > [PATCH] gdb+gdbserver/Linux: Remove USE_SIGTRAP_SIGINFO fallback > https://sourceware.org/pipermail/gdb-patches/2022-June/190372.html > > Attaching will need some other fix, because in that case, the procedure is that the core > requests an attach, and then the target reports an initial stop, and that stop > makes infrun read the thread's PC, before setup_inferior is called. It may be > we need to call setup_inferior earlier. Or maybe we can just add a target_find_description > call in linux_nat_target::attach, like remote.c does: > > void > extended_remote_target::attach (const char *args, int from_tty) > { > ... > /* Next, if the target can specify a description, read it. We do > this before anything involving memory or registers. */ > target_find_description (); > > I haven't looked at that in much detail, and I have to leave for tonight. > > From 303d216239ba45c8930bd4291b9f49aae105ada7 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Mon, 27 Jun 2022 20:41:50 +0100 > Subject: [PATCH] gdb/Linux: avoid reading registers while going through shell > > For every stop, linux-nat.c saves the stopped thread's PC, in > lwp->stop_pc. This is done in save_stop_reason. However, while we're > going through the shell after "run", in startup_inferior, we shouldn't > be reading registers, as the shell's architecture may not even be the > same as the final inferior's. > > Since lwp->stop_pc is only needed when the thread has stopped for a > breakpoint, and since when going through the shell, no breakpoint is > going to hit, we can simple teach save_stop_reason to only record the > stop pc when the thread stopped for a breakpoint. Thanks, this looks good. Maybe you should go ahead and merge this, and the attach problem can be solved separately. Thanks, Andrew > > Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad > --- > gdb/linux-nat.c | 38 ++++++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index 008791c12dc..27437da7c94 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -2514,24 +2514,20 @@ select_event_lwp_callback (struct lwp_info *lp, int *selector) > static void > save_stop_reason (struct lwp_info *lp) > { > - struct regcache *regcache; > - struct gdbarch *gdbarch; > - CORE_ADDR pc; > - CORE_ADDR sw_bp_pc; > - siginfo_t siginfo; > - > gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON); > gdb_assert (lp->status != 0); > > if (!linux_target->low_status_is_event (lp->status)) > return; > > - regcache = get_thread_regcache (linux_target, lp->ptid); > - gdbarch = regcache->arch (); > - > - pc = regcache_read_pc (regcache); > - sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch); > + /* Note we avoid reading this until we know we have a sw/hw > + breakpoint stop, which are the cases we actually need it, see > + status_callback. This is to avoid reading registers while we're > + going through the shell, before we're able to determine the > + debuggee's target description. */ > + lp->stop_pc = 0; > > + siginfo_t siginfo; > if (linux_nat_get_siginfo (lp->ptid, &siginfo)) > { > if (siginfo.si_signo == SIGTRAP) > @@ -2580,25 +2576,31 @@ save_stop_reason (struct lwp_info *lp) > linux_nat_debug_printf ("%s stopped by software breakpoint", > lp->ptid.to_string ().c_str ()); > > - /* Back up the PC if necessary. */ > - if (pc != sw_bp_pc) > - regcache_write_pc (regcache, sw_bp_pc); > + struct regcache *regcache = get_thread_regcache (linux_target, lp->ptid); > + > + lp->stop_pc = regcache_read_pc (regcache); > > - /* Update this so we record the correct stop PC below. */ > - pc = sw_bp_pc; > + /* Back up the PC if necessary. */ > + CORE_ADDR decr_pc = gdbarch_decr_pc_after_break (regcache->arch ()); > + if (decr_pc != 0) > + { > + lp->stop_pc -= decr_pc; > + regcache_write_pc (regcache, lp->stop_pc); > + } > } > else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT) > { > linux_nat_debug_printf ("%s stopped by hardware breakpoint", > lp->ptid.to_string ().c_str ()); > + > + struct regcache *regcache = get_thread_regcache (linux_target, lp->ptid); > + lp->stop_pc = regcache_read_pc (regcache); > } > else if (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT) > { > linux_nat_debug_printf ("%s stopped by hardware watchpoint", > lp->ptid.to_string ().c_str ()); > } > - > - lp->stop_pc = pc; > } > > > > base-commit: f18acc9c4e5d18f4783f3a7d59e3ec95d7af0199 > prerequisite-patch-id: 9656309f5c298727d3cf993bed0bb8e00c9e9d56 > -- > 2.36.0