[binutils-gdb] gdb+gdbserver/Linux: avoid reading registers while going through shell

[binutils-gdb] gdb+gdbserver/Linux: avoid reading registers while going through shell

[Pedro Alves]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a9deee17d33235eeba401eebed55c85a742e3358

commit a9deee17d33235eeba401eebed55c85a742e3358
Author: Pedro Alves <pedro@palves.net>
Date:   Mon Jun 27 20:41:50 2022 +0100

    gdb+gdbserver/Linux: avoid reading registers while going through shell
    
    For every stop, Linux GDB and GDBserver save 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 gdb/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), random
    signals, and exec events, so nothing is lost.
    
    Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad

Diff:
---
 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 @@ public:
      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 b9164e621db..0a93ab5c6ae 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2539,6 +2539,10 @@ save_stop_reason (struct lwp_info *lp)
   if (!linux_target->low_status_is_event (lp->status))
     return;
 
+  inferior *inf = find_inferior_ptid (linux_target, lp->ptid);
+  if (inf->starting_up)
+    return;
+
   regcache = get_thread_regcache (linux_target, lp->ptid);
   gdbarch = regcache->arch ();
 
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 8b8614f6ed4..2f71360d3bd 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);
 
@@ -797,6 +797,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 ();