[binutils-gdb] gdbserver: Eliminate prepare_to_access_memory

[binutils-gdb] gdbserver: Eliminate prepare_to_access_memory

[Pedro Alves]

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

commit 8e347faf8f1556a0f1afc33bd53099ec5f2f8efe
Author: Pedro Alves <pedro@palves.net>
Date:   Tue Mar 29 13:32:48 2022 +0100

    gdbserver: Eliminate prepare_to_access_memory
    
    Given:
    
     - The prepare_to_access_memory machinery was added for non-stop mode.
    
     - Only Linux supports non-stop.
    
     - Linux no longer needs the prepare_to_access_memory machinery.  In
       fact, after the previous patch,
       linux_process_target::prepare_to_access_memory became a nop.
    
    Thus, prepare_to_access_memory can go away, simplifying core GDBserver
    code.
    
    Change-Id: I93ac8bfe66bd61c3d1c4a0e7d419335163120ecf

Diff:
---
 gdbserver/mem-break.cc  | 101 +++++++-----------------------------------------
 gdbserver/server.cc     |  30 +++++---------
 gdbserver/target.cc     |  94 --------------------------------------------
 gdbserver/target.h      |  21 ----------
 gdbserver/tracepoint.cc |  13 +------
 5 files changed, 24 insertions(+), 235 deletions(-)

diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
index 5f5cdb18797..72ce8c8a5cb 100644
--- a/gdbserver/mem-break.cc
+++ b/gdbserver/mem-break.cc
@@ -1000,13 +1000,19 @@ z_type_supported (char z_type)
    failure returns NULL and sets *ERR to either -1 for error, or 1 if
    Z_TYPE breakpoints are not supported on this target.  */
 
-static struct gdb_breakpoint *
-set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
+struct gdb_breakpoint *
+set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
 {
   struct gdb_breakpoint *bp;
   enum bkpt_type type;
   enum raw_bkpt_type raw_type;
 
+  if (!z_type_supported (z_type))
+    {
+      *err = 1;
+      return nullptr;
+    }
+
   /* If we see GDB inserting a second code breakpoint at the same
      address, then either: GDB is updating the breakpoint's conditions
      or commands; or, the first breakpoint must have disappeared due
@@ -1074,110 +1080,31 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
 						   kind, NULL, err);
 }
 
-static int
-check_gdb_bp_preconditions (char z_type, int *err)
-{
-  /* As software/memory breakpoints work by poking at memory, we need
-     to prepare to access memory.  If that operation fails, we need to
-     return error.  Seeing an error, if this is the first breakpoint
-     of that type that GDB tries to insert, GDB would then assume the
-     breakpoint type is supported, but it may actually not be.  So we
-     need to check whether the type is supported at all before
-     preparing to access memory.  */
-  if (!z_type_supported (z_type))
-    {
-      *err = 1;
-      return 0;
-    }
-
-  return 1;
-}
-
-/* See mem-break.h.  This is a wrapper for set_gdb_breakpoint_1 that
-   knows to prepare to access memory for Z0 breakpoints.  */
-
-struct gdb_breakpoint *
-set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err)
-{
-  struct gdb_breakpoint *bp;
-
-  if (!check_gdb_bp_preconditions (z_type, err))
-    return NULL;
-
-  /* If inserting a software/memory breakpoint, need to prepare to
-     access memory.  */
-  if (z_type == Z_PACKET_SW_BP)
-    {
-      if (prepare_to_access_memory () != 0)
-	{
-	  *err = -1;
-	  return NULL;
-	}
-    }
-
-  bp = set_gdb_breakpoint_1 (z_type, addr, kind, err);
-
-  if (z_type == Z_PACKET_SW_BP)
-    done_accessing_memory ();
-
-  return bp;
-}
-
 /* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously
    inserted at ADDR with set_gdb_breakpoint_at.  Returns 0 on success,
    -1 on error, and 1 if Z_TYPE breakpoints are not supported on this
    target.  */
 
-static int
-delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind)
+int
+delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
 {
-  struct gdb_breakpoint *bp;
-  int err;
+  if (!z_type_supported (z_type))
+    return 1;
 
-  bp = find_gdb_breakpoint (z_type, addr, kind);
+  gdb_breakpoint *bp = find_gdb_breakpoint (z_type, addr, kind);
   if (bp == NULL)
     return -1;
 
   /* Before deleting the breakpoint, make sure to free its condition
      and command lists.  */
   clear_breakpoint_conditions_and_commands (bp);
-  err = delete_breakpoint ((struct breakpoint *) bp);
+  int err = delete_breakpoint ((struct breakpoint *) bp);
   if (err != 0)
     return -1;
 
   return 0;
 }
 
-/* See mem-break.h.  This is a wrapper for delete_gdb_breakpoint that
-   knows to prepare to access memory for Z0 breakpoints.  */
-
-int
-delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind)
-{
-  int ret;
-
-  if (!check_gdb_bp_preconditions (z_type, &ret))
-    return ret;
-
-  /* If inserting a software/memory breakpoint, need to prepare to
-     access memory.  */
-  if (z_type == Z_PACKET_SW_BP)
-    {
-      int err;
-
-      err = prepare_to_access_memory ();
-      if (err != 0)
-	return -1;
-    }
-
-  ret = delete_gdb_breakpoint_1 (z_type, addr, kind);
-
-  if (z_type == Z_PACKET_SW_BP)
-    done_accessing_memory ();
-
-  return ret;
-}
-
 /* Clear all conditions associated with a breakpoint.  */
 
 static void
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index e43a40a9b1f..24925cbbb5f 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1067,19 +1067,12 @@ gdb_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
       /* (assume no half-trace half-real blocks for now) */
     }
 
-  res = prepare_to_access_memory ();
-  if (res == 0)
-    {
-      if (set_desired_thread ())
-	res = read_inferior_memory (memaddr, myaddr, len);
-      else
-	res = 1;
-      done_accessing_memory ();
-
-      return res == 0 ? len : -1;
-    }
+  if (set_desired_thread ())
+    res = read_inferior_memory (memaddr, myaddr, len);
   else
-    return -1;
+    res = 1;
+
+  return res == 0 ? len : -1;
 }
 
 /* Write trace frame or inferior memory.  Actually, writing to trace
@@ -1095,15 +1088,10 @@ gdb_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
     {
       int ret;
 
-      ret = prepare_to_access_memory ();
-      if (ret == 0)
-	{
-	  if (set_desired_thread ())
-	    ret = target_write_memory (memaddr, myaddr, len);
-	  else
-	    ret = EIO;
-	  done_accessing_memory ();
-	}
+      if (set_desired_thread ())
+	ret = target_write_memory (memaddr, myaddr, len);
+      else
+	ret = EIO;
       return ret;
     }
 }
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index e9d1e1aa38c..883242377c0 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -39,88 +39,6 @@ set_desired_thread ()
   return (current_thread != NULL);
 }
 
-/* The thread that was current before prepare_to_access_memory was
-   called.  done_accessing_memory uses this to restore the previous
-   selected thread.  */
-static ptid_t prev_general_thread;
-
-/* See target.h.  */
-
-int
-prepare_to_access_memory (void)
-{
-  client_state &cs = get_client_state ();
-
-  /* The first thread found.  */
-  struct thread_info *first = NULL;
-  /* The first stopped thread found.  */
-  struct thread_info *stopped = NULL;
-  /* The current general thread, if found.  */
-  struct thread_info *current = NULL;
-
-  /* Save the general thread value, since prepare_to_access_memory could change
-     it.  */
-  prev_general_thread = cs.general_thread;
-
-  int res = the_target->prepare_to_access_memory ();
-  if (res != 0)
-    return res;
-
-  for_each_thread (prev_general_thread.pid (), [&] (thread_info *thread)
-    {
-      if (mythread_alive (thread->id))
-	{
-	  if (stopped == NULL && the_target->supports_thread_stopped ()
-	      && target_thread_stopped (thread))
-	    stopped = thread;
-
-	  if (first == NULL)
-	    first = thread;
-
-	  if (current == NULL && prev_general_thread == thread->id)
-	    current = thread;
-	}
-    });
-
-  /* The thread we end up choosing.  */
-  struct thread_info *thread;
-
-  /* Prefer a stopped thread.  If none is found, try the current
-     thread.  Otherwise, take the first thread in the process.  If
-     none is found, undo the effects of
-     target->prepare_to_access_memory() and return error.  */
-  if (stopped != NULL)
-    thread = stopped;
-  else if (current != NULL)
-    thread = current;
-  else if (first != NULL)
-    thread = first;
-  else
-    {
-      done_accessing_memory ();
-      return 1;
-    }
-
-  switch_to_thread (thread);
-  cs.general_thread = ptid_of (thread);
-
-  return 0;
-}
-
-/* See target.h.  */
-
-void
-done_accessing_memory (void)
-{
-  client_state &cs = get_client_state ();
-
-  the_target->done_accessing_memory ();
-
-  /* Restore the previous selected thread.  */
-  cs.general_thread = prev_general_thread;
-  switch_to_thread (the_target, cs.general_thread);
-}
-
 int
 read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
@@ -373,18 +291,6 @@ process_stratum_target::post_create_inferior ()
   /* Nop.  */
 }
 
-int
-process_stratum_target::prepare_to_access_memory ()
-{
-  return 0;
-}
-
-void
-process_stratum_target::done_accessing_memory ()
-{
-  /* Nop.  */
-}
-
 void
 process_stratum_target::look_up_symbols ()
 {
diff --git a/gdbserver/target.h b/gdbserver/target.h
index aaa9dab742c..f3172e2ed7e 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -141,21 +141,6 @@ public:
      If REGNO is -1, store all registers; otherwise, store at least REGNO.  */
   virtual void store_registers (regcache *regcache, int regno) = 0;
 
-  /* Prepare to read or write memory from the inferior process.
-     Targets use this to do what is necessary to get the state of the
-     inferior such that it is possible to access memory.
-
-     This should generally only be called from client facing routines,
-     such as gdb_read_memory/gdb_write_memory, or the GDB breakpoint
-     insertion routine.
-
-     Like `read_memory' and `write_memory' below, returns 0 on success
-     and errno on failure.  */
-  virtual int prepare_to_access_memory ();
-
-  /* Undo the effects of prepare_to_access_memory.  */
-  virtual void done_accessing_memory ();
-
   /* Read memory from the inferior process.  This should generally be
      called through read_inferior_memory, which handles breakpoint shadowing.
 
@@ -691,12 +676,6 @@ target_read_btrace_conf (struct btrace_target_info *tinfo,
 ptid_t mywait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	       target_wait_flags options, int connected_wait);
 
-/* Prepare to read or write memory from the inferior process.  See the
-   corresponding process_stratum_target methods for more details.  */
-
-int prepare_to_access_memory (void);
-void done_accessing_memory (void);
-
 #define target_core_of_thread(ptid)		\
   the_target->core_of_thread (ptid)
 
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 5459dc34cbb..18b2b0b3d77 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -2784,21 +2784,10 @@ cmd_qtenable_disable (char *own_buf, int enable)
 
       if (tp->type == fast_tracepoint || tp->type == static_tracepoint)
 	{
-	  int ret;
 	  int offset = offsetof (struct tracepoint, enabled);
 	  CORE_ADDR obj_addr = tp->obj_addr_on_target + offset;
 
-	  ret = prepare_to_access_memory ();
-	  if (ret)
-	    {
-	      trace_debug ("Failed to temporarily stop inferior threads");
-	      write_enn (own_buf);
-	      return;
-	    }
-
-	  ret = write_inferior_int8 (obj_addr, enable);
-	  done_accessing_memory ();
-	  
+	  int ret = write_inferior_int8 (obj_addr, enable);
 	  if (ret)
 	    {
 	      trace_debug ("Cannot write enabled flag into "