From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id A8E0F3885A1A for ; Tue, 28 Jun 2022 12:42:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A8E0F3885A1A 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-f41.google.com with SMTP id v193-20020a1cacca000000b003a051f41541so1141629wme.5 for ; Tue, 28 Jun 2022 05:42:54 -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=K5fCyhwQTGaJY3A/X4zNCx7qooUJqBt24J8fWQCzHEY=; b=JRFY1S848syVYqWmmQ/H1J2wIBUSlIqkA+3lv/6lZEYTK4jhhFdmrIN2ZQOa4bOUpu WbyOxwSTwr+o0cKhYgpBztknNwtQvaD9ZapdCAPv1OxFpP61xiLZzq2jrraOgL1iQHun N690F0HKQfwjd0Ij41XOr9zdNQfcySBCGOPaeCCORPmlfXMRpyLNhusv5RfOmp4L5gJM BLhgXszgkyLGu9mL5wL4a8+W6J8WVCyDZGroCBZw43xcFSopK7IOPgO8CqgSJpbuJaem sz/YGGOzBslG1gZv5/Bhv+9jJAsWfOvPrOs1/0TDKQ4tA3LknDBc9M5vjW4RXqIQxQeg g0Aw== X-Gm-Message-State: AJIora+KR1hbnapj3OaPuCY9jxeuAAHEr334wWavezX0CcgNaddOZo3b N7YRg0ZZqquyUuqR/gUWIsGYA+ZQGIg= X-Google-Smtp-Source: AGRyM1vuQv4MvvWzUh8VIQe7pg6hPgCtBS+QPvbjFGVLWoF+Snxenh1x1nhYrni/7GYJ75f5Up+42w== X-Received: by 2002:a05:600c:1c9d:b0:3a0:4e04:9475 with SMTP id k29-20020a05600c1c9d00b003a04e049475mr7829128wms.87.1656420173509; Tue, 28 Jun 2022 05:42:53 -0700 (PDT) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id s11-20020a5d4ecb000000b0020fe61acd09sm13812043wrv.12.2022.06.28.05.42.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Jun 2022 05:42:52 -0700 (PDT) Message-ID: Date: Tue, 28 Jun 2022 13:42:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: 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> <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> <87a69xdqfo.fsf@redhat.com> From: Pedro Alves In-Reply-To: <87a69xdqfo.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, 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: Tue, 28 Jun 2022 12:42:57 -0000 On 2022-06-28 11:37, Andrew Burgess wrote: > Pedro Alves writes: > >> On 2022-06-27 17:27, Andrew Burgess wrote: >>> Pedro Alves writes: >> 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. ... > > Thanks, this looks good. Maybe you should go ahead and merge this, and > the attach problem can be solved separately. There's the gdb vs gdbserver divergence issue I pointed out above, though. I was thinking we'd instead fix it with the alternative patch below, which works for both gdb and gdbserver. Tested with no regressions on x86-64 Ubuntu 20.04, native and gdbserver. WDYT? >From 1b7b50e7c8359553af8b8858a82f09d5389d4a1b Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 27 Jun 2022 20:41:50 +0100 Subject: [PATCH] gdb+gdbserver/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, in both gdb/linux-nat.c and gdbserver/linux-low.cc. However, while we're going through the shell after "run", in startup_inferior, we shouldn't be reading registers, as we haven't yet determined the target's architecture -- the shell's architecture may not even be the same as the final inferior's. In linux-nat.c, 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 could simply teach save_stop_reason to only record the stop pc when the thread stopped for a breakpoint. However, in gdbserver/linux-low.cc, lwp->stop_pc is used in more cases than breakpoint hits (e.g., it's used in tracepoints & the "while-stepping" feature). So to avoid GDB vs GDBserver divergence, we apply the same approach to both implementations. We set a flag in the inferior (process in gdbserver) whenever it is being nursed through the shell, and when that flag is set, save_stop_reason bails out early. While going through the shell, we'll only ever get process exits (normal or signalled), and exec events, so nothing is lost. Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad --- gdb/fork-child.c | 3 +++ gdb/inferior.h | 7 +++++++ gdb/linux-nat.c | 4 ++++ gdbserver/fork-child.cc | 5 +++++ gdbserver/inferiors.h | 7 +++++++ gdbserver/linux-low.cc | 16 ++++++++++++---- 6 files changed, 38 insertions(+), 4 deletions(-) diff --git a/gdb/fork-child.c b/gdb/fork-child.c index 89003fa617d..66b523e118d 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -126,6 +126,9 @@ gdb_startup_inferior (pid_t pid, int num_traps) inferior *inf = current_inferior (); process_stratum_target *proc_target = inf->process_target (); + scoped_restore save_starting_up + = make_scoped_restore (&inf->starting_up, true); + ptid_t ptid = startup_inferior (proc_target, pid, num_traps, NULL, NULL); /* Mark all threads non-executing. */ diff --git a/gdb/inferior.h b/gdb/inferior.h index f6e26a32feb..c376d780de0 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -551,6 +551,13 @@ class inferior : public refcounted_object, architecture/description. */ bool needs_setup = false; + /* True if the inferior is starting up (inside startup_inferior), + and we're nursing it along (through the shell) until it is ready + to execute its first instruction. Until that is done, we must + not access inferior memory or registers, as we haven't determined + the target architecture/description. */ + bool starting_up = false; + /* True when we are reading the library list of the inferior during an attach or handling a fork child. */ bool in_initial_library_scan = false; diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 008791c12dc..9ae02dfc093 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -2523,6 +2523,10 @@ save_stop_reason (struct lwp_info *lp) gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON); gdb_assert (lp->status != 0); + inferior *inf = find_inferior_ptid (linux_target, lp->ptid); + if (inf->starting_up) + return; + if (!linux_target->low_status_is_event (lp->status)) return; diff --git a/gdbserver/fork-child.cc b/gdbserver/fork-child.cc index 96dd4d009ab..7ea66f2a435 100644 --- a/gdbserver/fork-child.cc +++ b/gdbserver/fork-child.cc @@ -18,6 +18,7 @@ #include "server.h" #include "gdbsupport/job-control.h" +#include "gdbsupport/scoped_restore.h" #include "nat/fork-inferior.h" #ifdef HAVE_SIGNAL_H #include @@ -103,6 +104,10 @@ post_fork_inferior (int pid, const char *program) atexit (restore_old_foreground_pgrp); #endif + process_info *proc = find_process_pid (pid); + scoped_restore save_starting_up + = make_scoped_restore (&proc->starting_up, true); + startup_inferior (the_target, pid, START_INFERIOR_TRAPS_EXPECTED, &cs.last_status, &cs.last_ptid); diff --git a/gdbserver/inferiors.h b/gdbserver/inferiors.h index f3ba4d82f71..6de746cb228 100644 --- a/gdbserver/inferiors.h +++ b/gdbserver/inferiors.h @@ -75,6 +75,13 @@ struct process_info /* Flag to mark that the DLL list has changed. */ bool dlls_changed = false; + + /* True if the inferior is starting up (inside startup_inferior), + and we're nursing it along (through the shell) until it is ready + to execute its first instruction. Until that is done, we must + not access inferior memory or registers, as we haven't determined + the target architecture/description. */ + bool starting_up = false; }; /* Get the pid of PROC. */ diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc index 9305928bbbf..547e56f6448 100644 --- a/gdbserver/linux-low.cc +++ b/gdbserver/linux-low.cc @@ -747,8 +747,8 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp, CORE_ADDR linux_process_target::get_pc (lwp_info *lwp) { - struct regcache *regcache; - CORE_ADDR pc; + process_info *proc = get_thread_process (get_lwp_thread (lwp)); + gdb_assert (!proc->starting_up); if (!low_supports_breakpoints ()) return 0; @@ -756,8 +756,8 @@ linux_process_target::get_pc (lwp_info *lwp) scoped_restore_current_thread restore_thread; switch_to_thread (get_lwp_thread (lwp)); - regcache = get_thread_regcache (current_thread, 1); - pc = low_get_pc (regcache); + struct regcache *regcache = get_thread_regcache (current_thread, 1); + CORE_ADDR pc = low_get_pc (regcache); threads_debug_printf ("pc is 0x%lx", (long) pc); @@ -795,6 +795,14 @@ linux_process_target::save_stop_reason (lwp_info *lwp) if (!low_supports_breakpoints ()) return false; + process_info *proc = get_thread_process (get_lwp_thread (lwp)); + if (proc->starting_up) + { + /* Claim we have the stop PC so that the caller doesn't try to + fetch it itself. */ + return true; + } + pc = get_pc (lwp); sw_breakpoint_pc = pc - low_decr_pc_after_break (); base-commit: dbcbf67ca565ec29f13a2302dcdf9b01ef7832ca prerequisite-patch-id: 9656309f5c298727d3cf993bed0bb8e00c9e9d56 -- 2.36.0