[PATCH 0/2] Fix gdb.base/multi-forks.exp regression with gdbserver (PR gdb/23377)

[PATCH 0/2] Fix gdb.base/multi-forks.exp regression with gdbserver (PR gdb/23377)

[Pedro Alves]

This series includes two patches that each in isolation would fix
gdb.base/multi-forks.exp against GDBserver (PR gdb/23377).

The real bug is that GDBserver's implementation of D;PID packet has a
bad assumption.  That is fixed by patch #1.

However, with that fix alone, using GDB 8.2 to debug against older
GDBservers would crash GDBserver.  Since it's easy to do, I thought
I'd add a GDB workaround too.  That's patch #2.

Pedro Alves (2):
  GDBserver: Don't assume thread's a current process in D;PID
    implementation (PR gdb/23377)
  GDB: Workaround D;PID handling bug in older GDBservers (PR gdb/23377)

 gdb/gdbserver/mem-break.c |  9 ++++-----
 gdb/gdbserver/mem-break.h |  3 ++-
 gdb/gdbserver/server.c    | 39 +++++++++++++++++++++------------------
 gdb/remote.c              |  8 ++++++++
 4 files changed, 35 insertions(+), 24 deletions(-)

-- 
2.14.4

[PATCH 2/2] GDB: Workaround D;PID handling bug in older GDBservers (PR gdb/23377)

[Pedro Alves]

This commit adds a GDB workaround for the GDBserver bug exposed by
commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
newer GDBs can continue working with older GDBservers.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/23377
	* remote.c (remote_target::remote_detach_pid): Call
	set_current_process.
---
 gdb/remote.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 297c198ed6..a81d67e5ed 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5661,6 +5661,14 @@ remote_target::remote_detach_pid (int pid)
 {
   struct remote_state *rs = get_remote_state ();
 
+  /* This should not be necessary, but the handling for D;PID in
+     GDBserver versions prior to 8.2 incorrectly assumes that the
+     selected process points to the same process we're detaching,
+     leading to misbehavior (and possibly GDBserver crashing) when it
+     does not.  Since it's easy and cheap, work around it by forcing
+     GDBserver to select GDB's current process.  */
+  set_general_process ();
+
   if (remote_multi_process_p (rs))
     xsnprintf (rs->buf, get_remote_packet_size (), "D;%x", pid);
   else
-- 
2.14.4

[PATCH 1/2] GDBserver: Don't assume thread's a current process in D;PID implementation (PR gdb/23377)

[Pedro Alves]

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:
yyyy-mm-dd  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.
---
 gdb/gdbserver/mem-break.c |  9 ++++-----
 gdb/gdbserver/mem-break.h |  3 ++-
 gdb/gdbserver/server.c    | 39 +++++++++++++++++++++------------------
 3 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 447afc7c10..60f64bd3cb 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 019a87da09..7d764fbd0d 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 of 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 e5d226c1a4..bf6302ba2d 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);
 	}
     }
-- 
2.14.4

Re: [PATCH 1/2] GDBserver: Don't assume thread's a current process in D;PID implementation (PR gdb/23377)

[Simon Marchi]

On 2018-07-10 03:06 PM, Pedro Alves wrote:
> diff --git a/gdb/gdbserver/mem-break.h b/gdb/gdbserver/mem-break.h
> index 019a87da09..7d764fbd0d 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 of PROC has any persistent command.  */

of -> if

Otherwise, LGTM.

Simon

Re: [PATCH 2/2] GDB: Workaround D;PID handling bug in older GDBservers (PR gdb/23377)

[Simon Marchi]

On 2018-07-10 03:06 PM, Pedro Alves wrote:
> This commit adds a GDB workaround for the GDBserver bug exposed by
> commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
> newer GDBs can continue working with older GDBservers.

That makes sense.  It would perhaps be good to wait for Sergio to look
at it too, since he also tackled the problem.

Simon

Re: [PATCH 2/2] GDB: Workaround D;PID handling bug in older GDBservers (PR gdb/23377)

[Sergio Durigan Junior]

On Tuesday, July 10 2018, Simon Marchi wrote:

> On 2018-07-10 03:06 PM, Pedro Alves wrote:
>> This commit adds a GDB workaround for the GDBserver bug exposed by
>> commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
>> newer GDBs can continue working with older GDBservers.
>
> That makes sense.  It would perhaps be good to wait for Sergio to look
> at it too, since he also tackled the problem.

Thanks for that.  TBH I was pursuing a totally different approach to fix
this problem, which, according to Pedro, did not make sense.  Therefore,
I think Pedro has a better grasp at what the problem is here.  But
nevertheless, I looked at the code and it seems OK from my end.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH 2/2] GDB: Workaround D;PID handling bug in older GDBservers (PR gdb/23377)

[Pedro Alves]

On 07/11/2018 08:50 PM, Sergio Durigan Junior wrote:
> On Tuesday, July 10 2018, Simon Marchi wrote:
> 
>> On 2018-07-10 03:06 PM, Pedro Alves wrote:
>>> This commit adds a GDB workaround for the GDBserver bug exposed by
>>> commit f2ffa92bbce9 ("gdb: Eliminate the 'stop_pc' global"), so that
>>> newer GDBs can continue working with older GDBservers.
>>
>> That makes sense.  It would perhaps be good to wait for Sergio to look
>> at it too, since he also tackled the problem.
> 
> Thanks for that.  TBH I was pursuing a totally different approach to fix
> this problem, which, according to Pedro, did not make sense.  Therefore,
> I think Pedro has a better grasp at what the problem is here.  But
> nevertheless, I looked at the code and it seems OK from my end.

Right, your patch was restoring the bit in switch_to_thread that
would read the stop_pc.  I.e., this bit that was removed by
the original patch that eliminated the stop_pc global:

@@ -1357,13 +1367,6 @@ switch_to_thread (thread_info *thr)
   switch_to_thread_no_regs (thr);
 
   reinit_frame_cache ();
-
-  /* We don't check for is_stopped, because we're called at times
-     while in the TARGET_RUNNING state, e.g., while handling an
-     internal event.  */
-  if (thr->state != THREAD_EXITED
-      && !thr->executing)
-    stop_pc = regcache_read_pc (get_thread_regcache (thr));
 }

But as I explained in the commit log of that commit, the
main point of that commit was exactly to eliminate that bit:

~~~~
    gdb: Eliminate the 'stop_pc' global
    
    In my multi-target work, I need to add a few more
    scoped_restore_current_thread and switch_to_thread calls in some
    places, and in some lower-level places I was fighting against the fact
    that switch_to_thread reads/refreshes the stop_pc global.
    
    Instead of piling on workarounds, let's just finally eliminate the
    stop_pc global.  We already have the per-thread
    thread_info->suspend.stop_pc field, so it's mainly a matter of using
    that more/instead.
~~~~

I.e., switch_to_thread refreshing the stop_pc is problematic.
So putting it back would not be good.

Also, as revealed by this series, putting back that bit
would be fixing the regression by papering over gdbserver's
latent bug.

Thanks,
Pedro Alves