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

login
register
mail settings
Submitter Pedro Alves
Date July 10, 2018, 7:06 p.m.
Message ID <20180710190623.13540-3-palves@redhat.com>
Download mbox | patch
Permalink /patch/28298/
State New
Headers show

Comments

Pedro Alves - July 10, 2018, 7:06 p.m.
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(+)
Simon Marchi - July 11, 2018, 12:57 a.m.
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
Sergio Durigan Junior - July 11, 2018, 7:50 p.m.
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,
Pedro Alves - July 11, 2018, 10:23 p.m.
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

Patch

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