Re: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection)

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2022-06-28 11:37, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>> 
>>> On 2022-06-27 17:27, Andrew Burgess wrote:
>>>> Pedro Alves <pedro@palves.net> 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 <pedro@palves.net>
> 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 <signal.h>
> @@ -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