This is the mail archive of the gdb-cvs@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[binutils-gdb] GDBserver: Don't assume a current process in D; PID implementation (PR gdb/23377)


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

commit 31445d1036f7fc41de2724cb016913c9b1461bb1
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Jul 11 23:31:44 2018 +0100

    GDBserver: Don't assume a current process in D;PID implementation (PR gdb/23377)
    
    This fixes a gdb.base/multi-forks.exp regression with GDBserver.
    
    Git commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global") caused
    the regression by exposing a latent bug in gdbserver.
    
    The bug is that GDBserver's implementation of the D;PID packet
    incorrectly assumes that the selected thread points to the process
    being detached.  This happens via the any_persistent_commands call,
    which calls current_process:
    
      (gdb) bt
      #0  0x000000000040a57e in internal_error(char const*, int, char const*, ...)
      (file=0x4a53c0 "src/gdb/gdbserver/inferiors.c", line=212, fmt=0x4a539e "%s:
      Assertion `%s' failed.") at src/gdb/gdbserver/../common/errors.c:54
      #1  0x0000000000420acf in current_process() () at
      src/gdb/gdbserver/inferiors.c:212
      #2  0x00000000004226a0 in any_persistent_commands() () at
      gdb/gdbserver/mem-break.c:308
      #3  0x000000000042cb43 in handle_detach(char*) (own_buf=0x6f0280 "D;62ea") at
      src/gdb/gdbserver/server.c:1210
      #4  0x0000000000433af3 in process_serial_event() () at
      src/gdb/gdbserver/server.c:4055
      #5  0x0000000000434878 in handle_serial_event(int, void*) (err=0,
      client_data=0x0)
    
    The "eliminate stop_pc" commit exposes the problem because before that
    commit, GDB's switch_to_thread always read the newly-selected thread's
    PC, and that would end up forcing GDBserver's selected thread to
    change accordingly as side effect.  After that commit, GDB no longer
    reads the thread's PC, and GDBserver does not switch the thread.
    
    Fix this by removing the assumption from GDBserver.
    
    gdb/gdbserver/ChangeLog:
    2018-07-11  Pedro Alves  <palves@redhat.com>
    
    	PR gdb/23377
    	* mem-break.c (any_persistent_commands): Add process_info
    	parameter and use it instead of relying on the current process.
    	Change return type to bool.
    	* mem-break.h (any_persistent_commands): Add process_info
    	parameter and change return type to bool.
    	* server.c (handle_detach): Remove require_running_or_return call.
    	Look up the process_info for the process we're about to detach.
    	If not found, return back error to GDB.  Adjust
    	any_persistent_commands call to pass down a process pointer.

Diff:
---
 gdb/gdbserver/ChangeLog   | 13 +++++++++++++
 gdb/gdbserver/mem-break.c |  9 ++++-----
 gdb/gdbserver/mem-break.h |  3 ++-
 gdb/gdbserver/server.c    | 39 +++++++++++++++++++++------------------
 4 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 7c482b0..f29ecea 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,18 @@
 2018-07-11  Pedro Alves  <palves@redhat.com>
 
+	PR gdb/23377
+	* mem-break.c (any_persistent_commands): Add process_info
+	parameter and use it instead of relying on the current process.
+	Change return type to bool.
+	* mem-break.h (any_persistent_commands): Add process_info
+	parameter and change return type to bool.
+	* server.c (handle_detach): Remove require_running_or_return call.
+	Look up the process_info for the process we're about to detach.
+	If not found, return back error to GDB.  Adjust
+	any_persistent_commands call to pass down a process pointer.
+
+2018-07-11  Pedro Alves  <palves@redhat.com>
+
 	* i387-fp.c (i387_cache_to_fsave, cache_to_fxsave)
 	(i387_cache_to_xsave): Use regcache_raw_get_unsigned_by_name
 	instead of collect_register_by_name.
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 447afc7..60f64bd 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -302,10 +302,9 @@ is_gdb_breakpoint (enum bkpt_type type)
 	  || type == gdb_breakpoint_Z4);
 }
 
-int
-any_persistent_commands (void)
+bool
+any_persistent_commands (process_info *proc)
 {
-  struct process_info *proc = current_process ();
   struct breakpoint *bp;
   struct point_command_list *cl;
 
@@ -317,11 +316,11 @@ any_persistent_commands (void)
 
 	  for (cl = gdb_bp->command_list; cl != NULL; cl = cl->next)
 	    if (cl->persistence)
-	      return 1;
+	      return true;
 	}
     }
 
-  return 0;
+  return false;
 }
 
 /* Find low-level breakpoint of type TYPE at address ADDR that is not
diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
index 019a87d..7430c99 100644
--- a/gdb/gdbserver/mem-break.h
+++ b/gdb/gdbserver/mem-break.h
@@ -125,7 +125,8 @@ int add_breakpoint_condition (struct gdb_breakpoint *bp,
 int add_breakpoint_commands (struct gdb_breakpoint *bp, const char **commands,
 			     int persist);
 
-int any_persistent_commands (void);
+/* Return true if PROC has any persistent command.  */
+bool any_persistent_commands (process_info *proc);
 
 /* Evaluation condition (if any) at breakpoint BP.  Return 1 if
    true and 0 otherwise.  */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index e5d226c..bf6302b 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1195,34 +1195,37 @@ static void
 handle_detach (char *own_buf)
 {
   client_state &cs = get_client_state ();
-  require_running_or_return (own_buf);
 
-  int pid;
+  process_info *process;
 
   if (cs.multi_process)
     {
       /* skip 'D;' */
-      pid = strtol (&own_buf[2], NULL, 16);
+      int pid = strtol (&own_buf[2], NULL, 16);
+
+      process = find_process_pid (pid);
     }
   else
-    pid = current_ptid.pid ();
-
-  if ((tracing && disconnected_tracing) || any_persistent_commands ())
     {
-      struct process_info *process = find_process_pid (pid);
+      process = (current_thread != nullptr
+		 ? get_thread_process (current_thread)
+		 : nullptr);
+    }
 
-      if (process == NULL)
-	{
-	  write_enn (own_buf);
-	  return;
-	}
+  if (process == NULL)
+    {
+      write_enn (own_buf);
+      return;
+    }
 
+  if ((tracing && disconnected_tracing) || any_persistent_commands (process))
+    {
       if (tracing && disconnected_tracing)
 	fprintf (stderr,
 		 "Disconnected tracing in effect, "
 		 "leaving gdbserver attached to the process\n");
 
-      if (any_persistent_commands ())
+      if (any_persistent_commands (process))
 	fprintf (stderr,
 		 "Persistent commands are present, "
 		 "leaving gdbserver attached to the process\n");
@@ -1250,13 +1253,13 @@ handle_detach (char *own_buf)
       return;
     }
 
-  fprintf (stderr, "Detaching from process %d\n", pid);
+  fprintf (stderr, "Detaching from process %d\n", process->pid);
   stop_tracing ();
-  if (detach_inferior (pid) != 0)
+  if (detach_inferior (process->pid) != 0)
     write_enn (own_buf);
   else
     {
-      discard_queued_stop_replies (ptid_t (pid));
+      discard_queued_stop_replies (ptid_t (process->pid));
       write_ok (own_buf);
 
       if (extended_protocol || target_running ())
@@ -1266,7 +1269,7 @@ handle_detach (char *own_buf)
 	     and instead treat this like a normal program exit.  */
 	  cs.last_status.kind = TARGET_WAITKIND_EXITED;
 	  cs.last_status.value.integer = 0;
-	  cs.last_ptid = ptid_t (pid);
+	  cs.last_ptid = ptid_t (process->pid);
 
 	  current_thread = NULL;
 	}
@@ -1278,7 +1281,7 @@ handle_detach (char *own_buf)
 	  /* If we are attached, then we can exit.  Otherwise, we
 	     need to hang around doing nothing, until the child is
 	     gone.  */
-	  join_inferior (pid);
+	  join_inferior (process->pid);
 	  exit (0);
 	}
     }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]