From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by sourceware.org (Postfix) with ESMTPS id A05B63858D32 for ; Mon, 27 Jun 2022 21:38:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A05B63858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f48.google.com with SMTP id k129so4806695wme.0 for ; Mon, 27 Jun 2022 14:38:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=ns7Pq6T3bkrbSdF0oIA2Xf231FS4hqko6Z3MunDzPEQ=; b=tVAp5odBGSBueTZtB1r6MZde9efoaBZ2YtOJTR/QgoWSkGZ8qqaLv/vAQk/QjtsfO9 3JQEiLlPHnVRkwHmFnhCClUpbiGI+9u7qkOgYoZ5JUqaVrhAgxrIV/SfqPOIYdnErE35 1H4G7IjhlseUcBFhA6gLFD35PL7k5doQjE/u5Xulq/+TaogKANKThEmQ5jp7QbfzwNnv /8Zxj7UBM1ccfyv+cmQXrNEuc4BJFrMihQdtyyjOUK9vljOXRMwaQvxjS7fMQMkqkZ/u MoyKNUPjb7Wnjizd2dV/grir3F/kEt29oMiBjF22pyG78dNDW3F4s4FSEQLq3N28Hz0L epAQ== X-Gm-Message-State: AJIora+ktjtn423S8Mn/eTaKSiXdbaD7gJwXE/OkblXAcqJaAMCorA73 PoTEZcvyvBTbrqhO1S4rRo90GBphKqs= X-Google-Smtp-Source: AGRyM1u2j/t3W8HKhsRwVmCslRg+3moYVv9xe/7uql3V4HUOWrf8LEBgdJL+e+mFcJdWjSIauga/sg== X-Received: by 2002:a1c:7403:0:b0:3a0:4d65:b1a5 with SMTP id p3-20020a1c7403000000b003a04d65b1a5mr4741981wmc.197.1656365916340; Mon, 27 Jun 2022 14:38:36 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:5b14:8ad0:780f:bdda? ([2001:8a0:f924:2600:5b14:8ad0:780f:bdda]) by smtp.gmail.com with ESMTPSA id y12-20020a5d4acc000000b0021b9416fa13sm13129486wrs.90.2022.06.27.14.38.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 27 Jun 2022 14:38:35 -0700 (PDT) Message-ID: <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> Date: Mon, 27 Jun 2022 22:38:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <83ceec06f74f4ce6a2dc8d794031fb233567ba6d.1655136816.git.aburgess@redhat.com> <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> <87fsjqdqco.fsf@redhat.com> From: Pedro Alves In-Reply-To: <87fsjqdqco.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, 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: Mon, 27 Jun 2022 21:38:40 -0000 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. 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