[PATCH 0/3] gdbserver/linux: access memory while threads are running without pausing

[PATCH 0/3] gdbserver/linux: access memory while threads are running without pausing

[Pedro Alves]

While working on teaching GDBserver to handle
stepping-over-thread-exit better, I stumbled on the following
scenario:

If you set a conditional breakpoint, and condition evaluation is done
on the target (which it is against gdbserver), GDB uploads the agent
expression for the condition to GDBserver.  When the breakpoint is
hit, GDBserver evaluates the condition, and if false, GDBserver steps
over the breakpoint itself, all without GDB involvement.  That
single-step is done in-line, GDBserver doesn't know how to do
displaced stepping.  That means that all threads are paused
temporarily for the single-step, the breakpoint is removed, and when
the single-step finishes, the breakpoint instruction is inserted
again.

Now, if that breakpoint was set on top of an exit syscall instruction,
then when the single-step over the breakpoint finishes, and GDBserver
tries to re-insert the breakpoint, the thread that was stepping is
gone.  And thus you get something like this:

  ...
  [threads] wait_1: Hit a gdbserver breakpoint.
  [threads] ax_vdebug: gdbserver/ax: About to interpret byte 0x22
  [threads] ax_vdebug: gdbserver/ax: Op const8 -> sp=1, top=0x0
  [threads] ax_vdebug: gdbserver/ax: About to interpret byte 0x27
  [threads] ax_vdebug: gdbserver/ax: At end of expression, sp=1, stack top cache=0x0
  [threads] wait_1: Hit a gdbserver breakpoint.
  [threads] wait_1: proceeding all threads.
  [threads] thread_needs_step_over: Need step over [LWP 864044]? Ignoring, not stopped
  [threads] thread_needs_step_over: Need step over [LWP 864578]? Ignoring, not stopped
  [threads] thread_needs_step_over: Need step over [LWP 864579]? Ignoring, has pending status.
  [threads] get_pc: pc is 0x5555555552a8
  [threads] ax_vdebug: gdbserver/ax: About to interpret byte 0x22
  [threads] ax_vdebug: gdbserver/ax: Op const8 -> sp=1, top=0x0
  [threads] ax_vdebug: gdbserver/ax: About to interpret byte 0x27
  [threads] ax_vdebug: gdbserver/ax: At end of expression, sp=1, stack top cache=0x0
  [threads] thread_needs_step_over: Need step over [LWP 864580]? yes, found breakpoint at 0x5555555552a8
  [threads] proceed_all_lwps: found thread 864580 needing a step-over
  [threads] start_step_over: Starting step-over on LWP 864580.  Stopping all threads
  [threads] stop_all_lwps: enter
  ...
  [threads] stop_all_lwps: exit
  [threads] start_step_over: Done stopping all threads for step-over.
  ...
  [threads] wait_for_event_filtered: waitpid(-1, ...) returned 864580, ERRNO-OK
  [threads] wait_for_event_filtered: waitpid 864580 received 0 (exited)
  [threads] filter_event: 864580 exited
  [threads] finish_step_over: Finished step over.
  [threads] insert_memory_breakpoint: Failed to read shadow memory of breakpoint at 0x5555555552a8 (No such process).
  [threads] reinsert_raw_breakpoint: Failed to reinsert breakpoint at 0x5555555552a8 (-1).
  [threads] reinsert_fast_tracepoint_jumps_at: Could not find fast tracepoint jump at 0x5555555552a8 in list (reinserting).
  [threads] delete_lwp: deleting 864580
  ...

Note the:

  [threads] insert_memory_breakpoint: Failed to read shadow memory of breakpoint at 0x5555555552a8 (No such process).
  [threads] reinsert_raw_breakpoint: Failed to reinsert breakpoint at 0x5555555552a8 (-1).

lines.  GDB fails to reinsert the breakpoint, because the event thread
is gone.  PTRACE_PEEKTEXT/PTRACE_POKETEXT don't work if you pass it a
dead thread.

OTOH, currently, from GDB's perspective, GDBserver/Linux does support
accessing memory while the program is running.  This is important for
e.g., for being able to set breakpoints while the inferior is running,
in non-stop mode.  The way this is implemented today, is that when
handling any remote protocol packet that requires accessing memory,
GDBserver temporarily pauses all threads.  This is done by wrapping
memory accesses with prepare_to_access_memory/done_accessing_memory.
prepare_to_access_memory ultimately calls into the target backend,
which does whatever is necessary to be able to prepare for accessing
memory.  For Linux, that currently translates to pausing everything,
via target_pause_all.  No other port supports non-stop, so these
prepare_to_access_memory/done_accessing_memory calls do nothing for
all other ports.  Now, this memory-access wrapping is only done in
GDB-facing code.  Internally in linux-low.cc, when handling an event,
we tend to know that the current thread is stopped, so we usually can
access memory without pausing everything else.  However, in the
scenario detailed above, the eventing thread has exited.  We would
need to find some other stopped thread to write memory through, and if
none found, stop some.

I guess you see where I'm going with this.  It's so much simpler if we
teach GDBserver to always access memory via /proc/PID/mem.  Then we
don't need to worry about whether the current thread is running, or is
gone.  We just access memory and it Just Works.

So that's what this series does -- it teaches GDBserver to always
access memory via /proc/PID/mem, and then eliminates the whole
prepare_to_access_memory mechanism.

Even without the scenario detailed above, not having to pause threads
all the time just makes sense.

Pedro Alves (3):
  gdbserver/qXfer::threads, prepare_to_access_memory=>target_pause_all
  gdbserver/linux: Access memory even if threads are running
  gdbserver: Eliminate prepare_to_access_memory

 gdbserver/linux-low.cc  | 237 ++++++++++++++--------------------------
 gdbserver/linux-low.h   |  11 +-
 gdbserver/mem-break.cc  | 101 +++--------------
 gdbserver/server.cc     |  81 +++++---------
 gdbserver/target.cc     |  94 ----------------
 gdbserver/target.h      |  21 ----
 gdbserver/tracepoint.cc |  13 +--
 7 files changed, 131 insertions(+), 427 deletions(-)


base-commit: 5321c31bc78379a33f07dc7bef9256d05b942ad7
-- 
2.26.2

[PATCH 1/3] gdbserver/qXfer::threads, prepare_to_access_memory=>target_pause_all

[Pedro Alves]

handle_qxfer_threads_proper needs to pause all threads even if the
target can read memory when threads are running, so use
target_pause_all instead, which is what the Linux implementation of
prepare_to_access_memory uses.  (Only Linux implements this hook.)

A following patch will make the Linux backend be able to access memory
when threads are running, and thus will also make
prepare_to_access_memory do nothing, which would cause testsuite
regressions without this change.

Change-Id: I127fec7246b7c45b60dfa7341e781606bf54b5da
---
 gdbserver/server.cc | 51 ++++++++++++++-------------------------------
 1 file changed, 16 insertions(+), 35 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 8e53f226d3c..60d0c2be17a 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1695,47 +1695,28 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer)
 static bool
 handle_qxfer_threads_proper (struct buffer *buffer)
 {
-  client_state &cs = get_client_state ();
-
-  scoped_restore_current_thread restore_thread;
-  scoped_restore save_current_general_thread
-    = make_scoped_restore (&cs.general_thread);
-
   buffer_grow_str (buffer, "<threads>\n");
 
-  process_info *error_proc = find_process ([&] (process_info *process)
-    {
-      /* The target may need to access memory and registers (e.g. via
-	 libthread_db) to fetch thread properties.  Prepare for memory
-	 access here, so that we potentially pause threads just once
-	 for all accesses.  Note that even if someday we stop needing
-	 to pause threads to access memory, we will need to be able to
-	 access registers, or other ptrace accesses like
-	 PTRACE_GET_THREAD_AREA.  */
-
-      /* Need to switch to each process in turn, because
-	 prepare_to_access_memory prepares for an access in the
-	 current process pointed to by general_thread.  */
-      switch_to_process (process);
-      cs.general_thread = current_thread->id;
-
-      int res = prepare_to_access_memory ();
-      if (res == 0)
-	{
-	  for_each_thread (process->pid, [&] (thread_info *thread)
-	    {
-	      handle_qxfer_threads_worker (thread, buffer);
-	    });
+  /* The target may need to access memory and registers (e.g. via
+     libthread_db) to fetch thread properties.  Even if don't need to
+     stop threads to access memory, we still will need to be able to
+     access registers, and other ptrace accesses like
+     PTRACE_GET_THREAD_AREA that require a paused thread.  Pause all
+     threads here, so that we pause each thread at most once for all
+     accesses.  */
+  if (non_stop)
+    target_pause_all (true);
 
-	  done_accessing_memory ();
-	  return false;
-	}
-      else
-	return true;
+  for_each_thread ([&] (thread_info *thread)
+    {
+      handle_qxfer_threads_worker (thread, buffer);
     });
 
+  if (non_stop)
+    target_unpause_all (true);
+
   buffer_grow_str0 (buffer, "</threads>\n");
-  return error_proc == nullptr;
+  return true;
 }
 
 /* Handle qXfer:threads:read.  */
-- 
2.26.2

[PATCH 2/3] gdbserver/linux: Access memory even if threads are running

[Pedro Alves]

Similarly to how the native Linux target was changed
and subsequently reworked in these commits:

 05c06f318fd9 Linux: Access memory even if threads are running
 8a89ddbda2ec Avoid /proc/pid/mem races (PR 28065)

... teach GDBserver to access memory even when the current thread is
running, by always accessing memory via /proc/PID/mem.

The existing comment:

  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
     running LWP.  */

... is incorrect for /proc/PID/mem does allow that.

Actually, from GDB's perspective, GDBserver could already access
memory while threads were running, but at the expense of pausing all
threads for the duration of the memory access, via
prepare_to_access_memory.  This new implementation does not require
pausing any thread, thus
linux_process_target::prepare_to_access_memory /
linux_process_target::done_accessing_memory become nops.  A subsequent
patch will remove the whole prepare_to_access_memory infrastructure
completely.

The GDBserver linux-low.cc implementation is simpler than GDB's
linux-nat.c's, because GDBserver always adds the unfollowed vfork/fork
children to the process list immediately when the fork/vfork event is
seen out of ptrace.  I.e., there's no need to keep the file descriptor
stored on a side map, we can store it directly in the process
structure.

Change-Id: I0abfd782ceaa4ddce8d3e5f3e2dfc5928862ef61
---
 gdbserver/linux-low.cc | 237 +++++++++++++++--------------------------
 gdbserver/linux-low.h  |  11 +-
 2 files changed, 91 insertions(+), 157 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 7726a4a0c36..3711f406519 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -403,8 +403,22 @@ linux_process_target::low_delete_thread (arch_lwp_info *info)
   gdb_assert (info == nullptr);
 }
 
+/* Open the /proc/PID/mem file for PROC.  */
+
+static void
+open_proc_mem_file (process_info *proc)
+{
+  gdb_assert (proc->priv->mem_fd == -1);
+
+  char filename[64];
+  xsnprintf (filename, sizeof filename, "/proc/%d/mem", proc->pid);
+
+  proc->priv->mem_fd
+    = gdb_open_cloexec (filename, O_RDWR | O_LARGEFILE, 0).release ();
+}
+
 process_info *
-linux_process_target::add_linux_process (int pid, int attached)
+linux_process_target::add_linux_process_no_mem_file (int pid, int attached)
 {
   struct process_info *proc;
 
@@ -412,7 +426,17 @@ linux_process_target::add_linux_process (int pid, int attached)
   proc->priv = XCNEW (struct process_info_private);
 
   proc->priv->arch_private = low_new_process ();
+  proc->priv->mem_fd = -1;
+
+  return proc;
+}
+
 
+process_info *
+linux_process_target::add_linux_process (int pid, int attached)
+{
+  process_info *proc = add_linux_process_no_mem_file (pid, attached);
+  open_proc_mem_file (proc);
   return proc;
 }
 
@@ -932,7 +956,11 @@ linux_process_target::create_inferior (const char *program,
 			 NULL, NULL, NULL, NULL);
   }
 
-  add_linux_process (pid, 0);
+  /* When spawning a new process, we can't open the mem file yet.  We
+     still have to nurse the process through the shell, and that execs
+     a couple times.  The address space a /proc/PID/mem file is
+     accessing is destroyed on exec.  */
+  process_info *proc = add_linux_process_no_mem_file (pid, 0);
 
   ptid = ptid_t (pid, pid);
   new_lwp = add_lwp (ptid);
@@ -940,6 +968,10 @@ linux_process_target::create_inferior (const char *program,
 
   post_fork_inferior (pid, program);
 
+  /* PROC is now past the shell running the program we want, so we can
+     open the /proc/PID/mem file.  */
+  open_proc_mem_file (proc);
+
   return pid;
 }
 
@@ -1095,7 +1127,9 @@ linux_process_target::attach (unsigned long pid)
   ptid_t ptid = ptid_t (pid, pid);
   int err;
 
-  proc = add_linux_process (pid, 1);
+  /* Delay opening the /proc/PID/mem file until we've successfully
+     attached.  */
+  proc = add_linux_process_no_mem_file (pid, 1);
 
   /* Attach to PID.  We will check for other threads
      soon.  */
@@ -1108,6 +1142,8 @@ linux_process_target::attach (unsigned long pid)
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
     }
 
+  open_proc_mem_file (proc);
+
   /* Don't ignore the initial SIGSTOP if we just attached to this
      process.  It will be collected by wait shortly.  */
   initial_thread = find_thread_ptid (ptid_t (pid, pid));
@@ -1542,6 +1578,7 @@ linux_process_target::mourn (process_info *process)
 
   /* Freeing all private data.  */
   priv = process->priv;
+  close (priv->mem_fd);
   low_delete_process (priv->arch_private);
   free (priv);
   process->priv = NULL;
@@ -5297,93 +5334,69 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
   return the_target->read_memory (memaddr, myaddr, len);
 }
 
-/* Copy LEN bytes from inferior's memory starting at MEMADDR
-   to debugger memory starting at MYADDR.  */
 
-int
-linux_process_target::read_memory (CORE_ADDR memaddr,
-				   unsigned char *myaddr, int len)
+/* Helper for read_memory/write_memory using /proc/PID/mem.  Because
+   we can use a single read/write call, this can be much more
+   efficient than banging away at PTRACE_PEEKTEXT.  Also, unlike
+   PTRACE_PEEKTEXT/PTRACE_POKETEXT, this works with running threads.
+   One an only one of READBUF and WRITEBUF is non-null.  If READBUF is
+   not null, then we're reading, otherwise we're writing.  */
+
+static int
+proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
+		  const gdb_byte *writebuf, int len)
 {
-  int pid = lwpid_of (current_thread);
-  PTRACE_XFER_TYPE *buffer;
-  CORE_ADDR addr;
-  int count;
-  char filename[64];
-  int i;
-  int ret;
-  int fd;
+  process_info *proc = current_process ();
 
-  /* Try using /proc.  Don't bother for one word.  */
-  if (len >= 3 * sizeof (long))
+  int fd = proc->priv->mem_fd;
+  if (fd == -1)
+    return EIO;
+
+  while (len > 0)
     {
       int bytes;
 
-      /* We could keep this file open and cache it - possibly one per
-	 thread.  That requires some juggling, but is even faster.  */
-      sprintf (filename, "/proc/%d/mem", pid);
-      fd = open (filename, O_RDONLY | O_LARGEFILE);
-      if (fd == -1)
-	goto no_proc;
-
       /* If pread64 is available, use it.  It's faster if the kernel
 	 supports it (only one syscall), and it's 64-bit safe even on
 	 32-bit platforms (for instance, SPARC debugging a SPARC64
 	 application).  */
 #ifdef HAVE_PREAD64
-      bytes = pread64 (fd, myaddr, len, memaddr);
+      bytes = (readbuf != nullptr
+	       ? pread64 (fd, readbuf, len, memaddr)
+	       : pwrite64 (fd, writebuf, len, memaddr));
 #else
       bytes = -1;
       if (lseek (fd, memaddr, SEEK_SET) != -1)
-	bytes = read (fd, myaddr, len);
+	bytes = (readbuf != nullptr
+		 ? read (fd, readbuf, len)
+		 ? write (fd, writebuf, len));
 #endif
 
-      close (fd);
-      if (bytes == len)
-	return 0;
-
-      /* Some data was read, we'll try to get the rest with ptrace.  */
-      if (bytes > 0)
+      if (bytes < 0)
+	return errno;
+      else if (bytes == 0)
 	{
-	  memaddr += bytes;
-	  myaddr += bytes;
-	  len -= bytes;
+	  /* EOF means the address space is gone, the whole process
+	     exited or execed.  */
+	  return EIO;
 	}
-    }
-
- no_proc:
-  /* Round starting address down to longword boundary.  */
-  addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  count = ((((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-	   / sizeof (PTRACE_XFER_TYPE));
-  /* Allocate buffer of that many longwords.  */
-  buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
 
-  /* Read all the longwords */
-  errno = 0;
-  for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
-    {
-      /* Coerce the 3rd arg to a uintptr_t first to avoid potential gcc warning
-	 about coercing an 8 byte integer to a 4 byte pointer.  */
-      buffer[i] = ptrace (PTRACE_PEEKTEXT, pid,
-			  (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-			  (PTRACE_TYPE_ARG4) 0);
-      if (errno)
-	break;
+      memaddr += bytes;
+      if (readbuf != nullptr)
+	readbuf += bytes;
+      else
+	writebuf += bytes;
+      len -= bytes;
     }
-  ret = errno;
 
-  /* Copy appropriate bytes out of the buffer.  */
-  if (i > 0)
-    {
-      i *= sizeof (PTRACE_XFER_TYPE);
-      i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
-      memcpy (myaddr,
-	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	      i < len ? i : len);
-    }
+  return 0;
+}
 
-  return ret;
+int
+linux_process_target::read_memory (CORE_ADDR memaddr,
+				   unsigned char *myaddr, int len)
+{
+  return proc_xfer_memory (memaddr, myaddr, nullptr, len);
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
@@ -5394,25 +5407,6 @@ int
 linux_process_target::write_memory (CORE_ADDR memaddr,
 				    const unsigned char *myaddr, int len)
 {
-  int i;
-  /* Round starting address down to longword boundary.  */
-  CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  int count
-    = (((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-    / sizeof (PTRACE_XFER_TYPE);
-
-  /* Allocate buffer of that many longwords.  */
-  PTRACE_XFER_TYPE *buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
-
-  int pid = lwpid_of (current_thread);
-
-  if (len == 0)
-    {
-      /* Zero length write always succeeds.  */
-      return 0;
-    }
-
   if (debug_threads)
     {
       /* Dump up to four bytes.  */
@@ -5420,7 +5414,7 @@ linux_process_target::write_memory (CORE_ADDR memaddr,
       char *p = str;
       int dump = len < 4 ? len : 4;
 
-      for (i = 0; i < dump; i++)
+      for (int i = 0; i < dump; i++)
 	{
 	  sprintf (p, "%02x", myaddr[i]);
 	  p += 2;
@@ -5428,54 +5422,10 @@ linux_process_target::write_memory (CORE_ADDR memaddr,
       *p = '\0';
 
       threads_debug_printf ("Writing %s to 0x%08lx in process %d",
-			    str, (long) memaddr, pid);
+			    str, (long) memaddr, current_process ()->pid);
     }
 
-  /* Fill start and end extra bytes of buffer with existing memory data.  */
-
-  errno = 0;
-  /* Coerce the 3rd arg to a uintptr_t first to avoid potential gcc warning
-     about coercing an 8 byte integer to a 4 byte pointer.  */
-  buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
-		      (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-		      (PTRACE_TYPE_ARG4) 0);
-  if (errno)
-    return errno;
-
-  if (count > 1)
-    {
-      errno = 0;
-      buffer[count - 1]
-	= ptrace (PTRACE_PEEKTEXT, pid,
-		  /* Coerce to a uintptr_t first to avoid potential gcc warning
-		     about coercing an 8 byte integer to a 4 byte pointer.  */
-		  (PTRACE_TYPE_ARG3) (uintptr_t) (addr + (count - 1)
-						  * sizeof (PTRACE_XFER_TYPE)),
-		  (PTRACE_TYPE_ARG4) 0);
-      if (errno)
-	return errno;
-    }
-
-  /* Copy data to be written over corresponding part of buffer.  */
-
-  memcpy ((char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  myaddr, len);
-
-  /* Write the entire buffer.  */
-
-  for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
-    {
-      errno = 0;
-      ptrace (PTRACE_POKETEXT, pid,
-	      /* Coerce to a uintptr_t first to avoid potential gcc warning
-		 about coercing an 8 byte integer to a 4 byte pointer.  */
-	      (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-	      (PTRACE_TYPE_ARG4) buffer[i]);
-      if (errno)
-	return errno;
-    }
-
-  return 0;
+  return proc_xfer_memory (memaddr, nullptr, myaddr, len);
 }
 
 void
@@ -6179,25 +6129,6 @@ linux_process_target::unpause_all (bool unfreeze)
   unstop_all_lwps (unfreeze, NULL);
 }
 
-int
-linux_process_target::prepare_to_access_memory ()
-{
-  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
-     running LWP.  */
-  if (non_stop)
-    target_pause_all (true);
-  return 0;
-}
-
-void
-linux_process_target::done_accessing_memory ()
-{
-  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
-     running LWP.  */
-  if (non_stop)
-    target_unpause_all (true);
-}
-
 /* Extract &phdr and num_phdr in the inferior.  Return 0 on success.  */
 
 static int
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 27cc9641f12..4284fb3348a 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -127,6 +127,9 @@ struct process_info_private
 
   /* &_r_debug.  0 if not yet determined.  -1 if no PT_DYNAMIC in Phdrs.  */
   CORE_ADDR r_debug;
+
+  /* The /proc/pid/mem file used for reading/writing memory.  */
+  int mem_fd;
 };
 
 struct lwp_info;
@@ -163,10 +166,6 @@ class linux_process_target : public process_stratum_target
 
   void store_registers (regcache *regcache, int regno) override;
 
-  int prepare_to_access_memory () override;
-
-  void done_accessing_memory () override;
-
   int read_memory (CORE_ADDR memaddr, unsigned char *myaddr,
 		   int len) override;
 
@@ -544,6 +543,10 @@ class linux_process_target : public process_stratum_target
      data.  */
   process_info *add_linux_process (int pid, int attached);
 
+  /* Same as add_linux_process, but don't open the /proc/PID/mem file
+     yet.  */
+  process_info *add_linux_process_no_mem_file (int pid, int attached);
+
   /* Add a new thread.  */
   lwp_info *add_lwp (ptid_t ptid);
 
-- 
2.26.2

Re: [PATCH 2/3] gdbserver/linux: Access memory even if threads are running

[Lancelot SIX]

> +/* Helper for read_memory/write_memory using /proc/PID/mem.  Because
> +   we can use a single read/write call, this can be much more
> +   efficient than banging away at PTRACE_PEEKTEXT.  Also, unlike
> +   PTRACE_PEEKTEXT/PTRACE_POKETEXT, this works with running threads.
> +   One an only one of READBUF and WRITEBUF is non-null.  If READBUF is
> +   not null, then we're reading, otherwise we're writing.  */

Hi,

Seems to me that this property can be asserted.  WDYT?

Something like

    gdb_assert ((readbuf == nullptr) != (writebuf == nullptr));

should do.

Best,
Lancelot.

> +
> +static int
> +proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
> +		  const gdb_byte *writebuf, int len)

Re: [PATCH 2/3] gdbserver/linux: Access memory even if threads are running

[Pedro Alves]

On 2022-03-30 14:22, Lancelot SIX wrote:
>> +/* Helper for read_memory/write_memory using /proc/PID/mem.  Because
>> +   we can use a single read/write call, this can be much more
>> +   efficient than banging away at PTRACE_PEEKTEXT.  Also, unlike
>> +   PTRACE_PEEKTEXT/PTRACE_POKETEXT, this works with running threads.
>> +   One an only one of READBUF and WRITEBUF is non-null.  If READBUF is
>> +   not null, then we're reading, otherwise we're writing.  */
> 
> Hi,
> 
> Seems to me that this property can be asserted.  WDYT?
> 
> Something like
> 
>     gdb_assert ((readbuf == nullptr) != (writebuf == nullptr));
> 
> should do.
> 

Doesn't hurt.  I've added that locally.  Thanks.

Re: [PATCH 2/3] gdbserver/linux: Access memory even if threads are running

[Pedro Alves]

On 2022-03-31 12:39, Pedro Alves wrote:
> On 2022-03-30 14:22, Lancelot SIX wrote:
>>> +/* Helper for read_memory/write_memory using /proc/PID/mem.  Because
>>> +   we can use a single read/write call, this can be much more
>>> +   efficient than banging away at PTRACE_PEEKTEXT.  Also, unlike
>>> +   PTRACE_PEEKTEXT/PTRACE_POKETEXT, this works with running threads.
>>> +   One an only one of READBUF and WRITEBUF is non-null.  If READBUF is
>>> +   not null, then we're reading, otherwise we're writing.  */
>>
>> Hi,
>>
>> Seems to me that this property can be asserted.  WDYT?
>>
>> Something like
>>
>>     gdb_assert ((readbuf == nullptr) != (writebuf == nullptr));
>>
>> should do.
>>
> 
> Doesn't hurt.  I've added that locally.  Thanks.

Funny enough, it does hurt...  Running the testsuite with that change
shows the assertion triggering.  This is because GDB may ask to write LEN==0
bytes, and we have this

 int
 target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
 		     ssize_t len)
 {
   /* Make a copy of the data because check_mem_write may need to
      update it.  */
   gdb::byte_vector buffer (myaddr, myaddr + len);
   check_mem_write (memaddr, buffer.data (), myaddr, len);
   return the_target->write_memory (memaddr, buffer.data (), len);
 }

... and buffer.data() may return NULL if the vector is empty.  I've added
a new patch to the series to add an early return if len==0.  I'll send
a v2 series in a bit.

[PATCH 3/3] gdbserver: Eliminate prepare_to_access_memory

[Pedro Alves]

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,
   target_ops::prepare_to_access_memory became a nop.

Thus, prepare_to_access_memory can go away, simplifying core GDBserver
code.

Change-Id: I93ac8bfe66bd61c3d1c4a0e7d419335163120ecf
---
 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 60d0c2be17a..64b8bb34b9a 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1071,19 +1071,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
@@ -1099,15 +1092,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 5009146d663..ddc0a08dd6d 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)
 {
@@ -360,18 +278,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 @@ class process_stratum_target
      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 "
-- 
2.26.2