[Patch] Broken multi-process detach

[Patch] Broken multi-process detach

[Marc Khouzam]

Hi,

Detaching from an inferior in multi-process is broken.
The problem seems to be that in linux_nat.c:linux_nat_detach()
GDB first unregisters from the event loop by calling 
linux_nat_async (NULL, 0).  In multi-process, further
events received from running inferiors are missed; breakpoint
hits are not reported, interrupting a thread is not reported,
and so on.  See below for a broken session example.

We could remove the call to linux_nat_async (NULL, 0) but I didn't
know if that would have bad side-effects.  Here is the patch that
does that, if it is felt that is the correct solution.

The testsuite hangs on HEAD so I wasn't able to run it yet.
But I'll do so once it works again.

Thanks

Marc

2012-07-16  Marc Khouzam  <marc.khouzam@ericsson.com>

        * linux_nat.c (linux_nat_detach): Don't unregister from the event
        loop.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.254
diff -u -r1.254 linux-nat.c
--- gdb/linux-nat.c     7 Jul 2012 12:13:56 -0000       1.254
+++ gdb/linux-nat.c     16 Jul 2012 12:44:38 -0000
@@ -1848,8 +1848,8 @@
 
   pid = GET_PID (inferior_ptid);
 
-  if (target_can_async_p ())
-    linux_nat_async (NULL, 0);
+  /* Don't unregister from the event loop, as there may be other
+     inferiors running. */
 
   /* Stop all threads before detaching.  ptrace requires that the
      thread is stopped to sucessfully detach.  */


Broken session:
==============
> gdb.7.5
GNU gdb (GDB) 7.4.50.20120714-cvs

==> set non-stop and start two inferiors
[...]

(gdb) inf thread
  Id   Target Id         Frame 
  2    process 23634     (running)
* 1    process 23629     main () at loopfirst.noprint.cc:5
(gdb) detach
Detaching from program: /home/lmckhou/testing/loopfirst.noprint, process 23629
(gdb) inf thread
  Id   Target Id         Frame 
  2    process 23634     (running)

No selected thread.  See `help thread'.
(gdb) thread 2
[Switching to thread 2 (process 23634)](running)

=> interrupt the running thread

(gdb) interrupt
(gdb) inf thread
  Id   Target Id         Frame 
* 2    process 23634     (running)

=> event not received, thread still marked running

(gdb) c
Continuing.
Cannot execute this command while the selected thread is running.

Re: [Patch] Broken multi-process detach

[Pedro Alves]

On 07/16/2012 02:54 PM, Marc Khouzam wrote:
> Hi,
> 
> Detaching from an inferior in multi-process is broken.
> The problem seems to be that in linux_nat.c:linux_nat_detach()
> GDB first unregisters from the event loop by calling 
> linux_nat_async (NULL, 0).  In multi-process, further
> events received from running inferiors are missed; breakpoint
> hits are not reported, interrupting a thread is not reported,
> and so on.  See below for a broken session example.
> 
> We could remove the call to linux_nat_async (NULL, 0) but I didn't
> know if that would have bad side-effects.  Here is the patch that
> does that, if it is felt that is the correct solution.

I'm not thinking of any.  Looks like I added it in the first
async mode version of linux-nat.c (which was much rewritten
later on).  Let's remove it.

Remove also the target_async call below:

  if (forks_exist_p ())
    {
      /* Multi-fork case.  The current inferior_ptid is being detached
	 from, but there are other viable forks to debug.  Detach from
	 the current fork, and context-switch to the first
	 available.  */
      linux_fork_detach (args, from_tty);

      if (non_stop && target_can_async_p ())
 	target_async (inferior_event_handler, 0);


as this was re-installing the target on the event loop if there were
other checkpoints (checkpoint/info checkpoint) to debug.  It is now
unnecessary if we don't unregister from the event loop in the first place.

> 
> The testsuite hangs on HEAD so I wasn't able to run it yet.
> But I'll do so once it works again.

It doesn't hang for me.  I've tested the patch on x86_64 Fedora 17, both
sync and async modes.  No regressions.

Thanks!

-- 
Pedro Alves

RE: [Patch] Broken multi-process detach

[Marc Khouzam]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com] 
> Sent: Monday, July 16, 2012 1:24 PM
> To: Marc Khouzam
> Cc: 'gdb-patches@sourceware.org'
> Subject: Re: [Patch] Broken multi-process detach
> 
> On 07/16/2012 02:54 PM, Marc Khouzam wrote:
> > Hi,
> > 
> > Detaching from an inferior in multi-process is broken.
> > The problem seems to be that in linux_nat.c:linux_nat_detach()
> > GDB first unregisters from the event loop by calling 
> > linux_nat_async (NULL, 0).  In multi-process, further
> > events received from running inferiors are missed; breakpoint
> > hits are not reported, interrupting a thread is not reported,
> > and so on.  See below for a broken session example.
> > 
> > We could remove the call to linux_nat_async (NULL, 0) but I didn't
> > know if that would have bad side-effects.  Here is the patch that
> > does that, if it is felt that is the correct solution.
> 
> I'm not thinking of any.  Looks like I added it in the first
> async mode version of linux-nat.c (which was much rewritten
> later on).  Let's remove it.
> 
> Remove also the target_async call below:
> 
>   if (forks_exist_p ())
>     {
>       /* Multi-fork case.  The current inferior_ptid is being detached
> 	 from, but there are other viable forks to debug.  Detach from
> 	 the current fork, and context-switch to the first
> 	 available.  */
>       linux_fork_detach (args, from_tty);
> 
>       if (non_stop && target_can_async_p ())
>  	target_async (inferior_event_handler, 0);
> 
> 
> as this was re-installing the target on the event loop if there were
> other checkpoints (checkpoint/info checkpoint) to debug.  It is now
> unnecessary if we don't unregister from the event loop in the 
> first place.
> 
> > 
> > The testsuite hangs on HEAD so I wasn't able to run it yet.
> > But I'll do so once it works again.
> 
> It doesn't hang for me.  I've tested the patch on x86_64 
> Fedora 17, both
> sync and async modes.  No regressions.

Thanks for the help!

I checked-in the following:

2012-07-16  Marc Khouzam  <marc.khouzam@ericsson.com>
	    Pedro Alves  <palves@redhat.com>

	* linux-nat.c (linux_nat_detach): Don't unregister from the event
	loop.

### Eclipse Workspace Patch 1.0
#P src
Index: gdb/linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/linux-nat.c,v
retrieving revision 1.254
diff -u -r1.254 linux-nat.c
--- gdb/linux-nat.c     7 Jul 2012 12:13:56 -0000       1.254
+++ gdb/linux-nat.c     16 Jul 2012 19:04:03 -0000
@@ -1848,8 +1848,8 @@
 
   pid = GET_PID (inferior_ptid);
 
-  if (target_can_async_p ())
-    linux_nat_async (NULL, 0);
+  /* Don't unregister from the event loop, as there may be other
+     inferiors running. */
 
   /* Stop all threads before detaching.  ptrace requires that the
      thread is stopped to sucessfully detach.  */
@@ -1892,9 +1892,6 @@
         the current fork, and context-switch to the first
         available.  */
       linux_fork_detach (args, from_tty);
-
-      if (non_stop && target_can_async_p ())
-       target_async (inferior_event_handler, 0);
     }
   else
     linux_ops->to_detach (ops, args, from_tty);