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 46B5338D68D6 for ; Tue, 28 Jun 2022 14:21:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 46B5338D68D6 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-569--_mCXQvvNweZ60wF0VXvGw-1; Tue, 28 Jun 2022 10:21:53 -0400 X-MC-Unique: -_mCXQvvNweZ60wF0VXvGw-1 Received: by mail-wr1-f70.google.com with SMTP id o1-20020adfba01000000b0021b90bd28d2so1842592wrg.14 for ; Tue, 28 Jun 2022 07:21:53 -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=7BXG1A39m5x0jHRkdc7jy9+K/lv93gdy2UHrtdqSBDE=; b=Qo7hN9iqOW8+c1TGOI/0zpeo8BEBnYje8uoUjXfqMbWbl0sm97cVOjKcvy5fDqmVBd 53ezluygrNYoHANd4IghZ/M5PW+bovwcT1pElwFi2Do17rfig31o3q+REmEhXwjd4taG MTJyJjAR6lEgxrYUq8qv9l9Rj/symArPfaPmGq3l4A/AAs7FwsSyBHnMlECHmQCy3EaF ksHi9idFSjir+0qoaci8c++1XNFdwoeAgiGHJlFsTinHzl3Ne0hhgT1ATZJCOrsHxxkv 2/hnIfWz464yDD8z21oc+pudIz1oQV6TALfwHlUZlIi2EVIFcjcW8TCdBhaR070uhvcG 3rJw== X-Gm-Message-State: AJIora8q2YJIe2CQQYZhsrNfP5gyk3uL5rCCImvtSFNuu5NXmQDWtqX2 sFJcHWLQilt9/5pU5J7bsIVZKx2jzvdn+U8KZVm1ZdyvdT8PeShyKYAWZFXEfcVIrC1zy57XPj4 4TZUJWz/DVaXKI/iVl4FvQg== X-Received: by 2002:a5d:595d:0:b0:21b:84af:552a with SMTP id e29-20020a5d595d000000b0021b84af552amr18427439wri.656.1656426112112; Tue, 28 Jun 2022 07:21:52 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sY8h+5C1BEJrHzXh8xvUbQ/9tvZ+9v/VwsgOszrY3m6mvtyZWO7+n3oIBUjo4auTekd6vGiw== X-Received: by 2002:a5d:595d:0:b0:21b:84af:552a with SMTP id e29-20020a5d595d000000b0021b84af552amr18427418wri.656.1656426111856; Tue, 28 Jun 2022 07:21:51 -0700 (PDT) Received: from localhost (15.72.115.87.dyn.plus.net. [87.115.72.15]) by smtp.gmail.com with ESMTPSA id f8-20020a1c6a08000000b003a044fe7fe7sm11518518wmc.9.2022.06.28.07.21.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Jun 2022 07:21:51 -0700 (PDT) From: Andrew Burgess To: Pedro Alves , gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection) In-Reply-To: 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> Date: Tue, 28 Jun 2022 15:21:50 +0100 Message-ID: <874k04eum9.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.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_LOW, 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 14:21:58 -0000 Pedro Alves writes: > 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. LGTM. Thanks, Andrew > > 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