Patchwork [v8,6/7] Remote fork catch

login
register
mail settings
Submitter Don Breazeal
Date April 17, 2015, 8:58 p.m.
Message ID <1429304325-13878-7-git-send-email-donb@codesourcery.com>
Download mbox | patch
Permalink /patch/6311/
State New
Headers show

Comments

Don Breazeal - April 17, 2015, 8:58 p.m.
Hi Pedro,
This version of the patch incorporates changes based on your comments on
the previous version, as outlined below.

On 4/15/2015 8:39 AM, Pedro Alves wrote:
> On 04/10/2015 06:09 PM, Don Breazeal wrote:

---snip snip---

>> +/* Determine if THREAD is a pending fork parent thread.  ARG contains
>> +   the pid of the process who's threads we want to check, or -1 if
>> +   we want to check all threads.  */
>> +
>> +static int
>> +pending_fork_parent_callback (struct thread_info *thread, void *arg)
>> +{
>> +  int pid = *(int *) arg;
>> +
>> +  if (thread->pending_follow.kind == TARGET_WAITKIND_FORKED
>> +      || thread->pending_follow.kind == TARGET_WAITKIND_VFORKED)
>> +    {
>> +      if ((pid == -1) || (pid == ptid_get_pid (thread->ptid)))
> 
> Unnecessary parens:
> 
>       if (pid == -1 || pid == ptid_get_pid (thread->ptid))

Fixed.

> 
> 
>> +	return 1;
>> +    }
>> +
>> +  return 0;
>> +}
>> +
>> +/* If CONTEXT contains any fork child threads that have not been
>> +   reported yet, remove them from the CONTEXT list.  If such a
>> +   thread exists it is because we are stopped at a fork catchpoint
>> +   and have not yet called follow_fork, which will set up the
>> +   host-side data structures for the new process.  */
>> +
>> +static void
>> +remove_new_fork_child (struct threads_listing_context *context)
>> +{
>> +  struct thread_info * thread;
>> +  int pid = -1;
>> +
>> +  /* Check to see if there is an in-progress fork parent.  */
>> +  thread = iterate_over_threads (pending_fork_parent_callback, &pid);
>> +  if (thread != NULL)
> 
> In non-stop mode, if you're debugging multiple process, multiple
> processes can fork at the same, and then we end up with multiple
> threads with an in-progress fork parent.  So this needs to walk
> the whole thread list, not just stop at the first.  Either
> use ALL_NON_EXITED_THREADS, or move the loop below to
> pending_fork_parent_callback (or to a helper function called
> by that).

Good point, thanks.  I have reworked this to use ALL_NON_EXITED_THREADS.

> 
>> +    {
>> +      ptid_t child_ptid = thread->pending_follow.value.related_pid;
>> +      struct thread_item *item;
>> +      int i;
>> +
>> +      for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
>> +	{
>> +	  if (ptid_equal (item->ptid, child_ptid))
>> +	    {
>> +	      VEC_ordered_remove (thread_item_t, context->items, i);
>> +	      break;
>> +	    }
>> +	}
>> +    }
>> +}
>> +
>>  /* Implement the to_update_thread_list function for the remote
>>     targets.  */
>>  
>> @@ -2874,6 +2964,10 @@ remote_update_thread_list (struct target_ops *ops)
>>  	    }
>>          }
>>  
>> +      /* Remove any unreported fork child from CONTEXT so that
>> +	 we don't interfere with follow fork.  */
>> +      remove_new_fork_child (&context);
> 
> I think there's a race here, in non-stop mode.  Consider:
> 
>  #1 - process forks just before gdb starts fetching the remote thread
>       list.
>  #2 - gdbserver adds the fork child  its thread list.
>  #3 - gdbserver queues the fork event, sends vStopped notification
>  #4 - gdb/remote_update_thread_list pulls the thread list
>  #5 - we're now in remove_new_fork_child, but we don't know
>       about the fork event yet.  It's still pending in the vStopped
>       queue.
> 
> So I think that we need to make remote_update_thread_list do,
> in this order:
> 
>  #1 - fetch the remote thread list
>  #2 - fetch the pending vStopped notifications
>         (remote_notif_get_pending_events)
>  #3 - call remove_new_fork_child
>  #4 - add threads we don't know about yet to our list.
> 
> and make remove_new_fork_child also peek at the
> pending vStopped events queue (and in the future at
> any other layers of pending events in the core side.)

I think all that was necessary here was to insert a call to
remote_notif_get_pending_events before the call to remove_new_fork_child,
since remote_notif_get_pending_events ultimately calls
remote_parse_stop_reply on the responses from the target, handling the
pending events like normal events.  Or am I missing something?  I 
inferred from your comment above that there might be more to it.

> 
>> +      child_pid = ptid_get_pid (thread->pending_follow.value.related_pid);
>> +      res = remote_vkill (child_pid, rs);
>> +      if (res != 0)
>> +	error (_("Can't kill fork child process"));
> 
> It'll probably be good to include the PID in the error message.

Done.

> 
>> +    }
> 
> Thanks,
> Pedro Alves
> 


Thanks
--Don

This patch implements catchpoints for fork events on extended-remote
Linux targets.

Implementation appeared to be straightforward, requiring four new functions
in remote.c to implement insert/remove of fork/vfork catchpoints.  These
functions are essentially stubs that just return 0 ('success') if the
required features are enabled.  If the fork events are being reported, then
catchpoints are set and hit.

However, there are some extra issues that arise with catchpoints.

1) Thread creation reporting -- fork catchpoints are hit before the
   follow_fork has been completed.  When stopped at a fork catchpoint
   in the native implementation, the new process is not 'reported'
   until after the follow is done.  It doesn't show up in the inferiors
   list or the threads list.  However, in the gdbserver case, an
   'info threads' while stopped at a fork catchpoint will retrieve the
   new thread info from the target and add it to GDB's data structures,
   prior to the follow operations.  Because of this premature report,
   things on the GDB side eventually get very confused.

   So in remote.c:remote_update_thread_list, we check to see if there
   are any pending fork parent threads.  If there are we remove the
   related fork child thread from the thread list sent by the target.

2) Kill process before fork is followed -- on the native side in
   linux-nat.c:linux_nat_kill, there is some code to handle the case where
   a fork has occurred but follow_fork hasn't been called yet.  It does
   this by using the last status to determine if a follow is pending, and
   if it is, to kill the child task.  The use of last_status is fragile
   in situations like non-stop mode where other events may have occurred
   after the fork event.  This patch identifies a fork parent
   in remote.c:extended_remote_kill in a way similar to that used in
   thread creation reporting above.  If one is found, it kills the new
   child as well.

Tested on x64 Ubuntu Lucid, native, remote, extended-remote.  Tested the
case of killing the forking process before the fork has been followed
manually.

Thanks
--Don

gdb/
2015-04-17  Don Breazeal  <donb@codesourcery.com>
	* remote.c (remote_insert_fork_catchpoint): New function.
	(remote_remove_fork_catchpoint): New function.
	(remote_insert_vfork_catchpoint): New function.
	(remote_remove_vfork_catchpoint): New function.
	(pending_fork_parent_callback): New function.
	(remove_new_fork_child): New function.
	(remote_update_thread_list): Call remote_notif_get_pending_events
	and remove_new_fork_child.
	(extended_remote_kill): Kill fork child when killing the
	parent before follow_fork completes.
	(init_extended_remote_ops): Initialize target vector with
	new fork catchpoint functions.


---
 gdb/remote.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)
Pedro Alves - April 23, 2015, 2:19 p.m.
On 04/17/2015 09:58 PM, Don Breazeal wrote:

>> > I think there's a race here, in non-stop mode.  Consider:
>> > 
>> >  #1 - process forks just before gdb starts fetching the remote thread
>> >       list.
>> >  #2 - gdbserver adds the fork child  its thread list.
>> >  #3 - gdbserver queues the fork event, sends vStopped notification
>> >  #4 - gdb/remote_update_thread_list pulls the thread list
>> >  #5 - we're now in remove_new_fork_child, but we don't know
>> >       about the fork event yet.  It's still pending in the vStopped
>> >       queue.
>> > 
>> > So I think that we need to make remote_update_thread_list do,
>> > in this order:
>> > 
>> >  #1 - fetch the remote thread list
>> >  #2 - fetch the pending vStopped notifications
>> >         (remote_notif_get_pending_events)
>> >  #3 - call remove_new_fork_child
>> >  #4 - add threads we don't know about yet to our list.
>> > 
>> > and make remove_new_fork_child also peek at the
>> > pending vStopped events queue (and in the future at
>> > any other layers of pending events in the core side.)
> I think all that was necessary here was to insert a call to
> remote_notif_get_pending_events before the call to remove_new_fork_child,
> since remote_notif_get_pending_events ultimately calls
> remote_parse_stop_reply on the responses from the target, handling the
> pending events like normal events.  Or am I missing something?  I 
> inferred from your comment above that there might be more to it.

It fetches the vStopped notifications out of the remote,
and parses them, but the events are left in the stop reply
queue, they're not handled.  They're only handled once the
core next calls target_wait -> remote_wait_ns -> queued_stop_reply.

So we need to peek into the stop reply queue, and check if we have
pending fork events.  We have peek_stop_reply for a similar use,
but it's not the same.

Thanks,
Pedro Alves

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index c0fa3b9..ec88afe 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1477,6 +1477,46 @@  remote_vfork_event_p (struct remote_state *rs)
   return packet_support (PACKET_vfork_event_feature) == PACKET_ENABLE;
 }
 
+/* Insert fork catchpoint target routine.  If fork events are enabled
+   then return success, nothing more to do.  */
+
+static int
+remote_insert_fork_catchpoint (struct target_ops *ops, int pid)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  return !remote_fork_event_p (rs);
+}
+
+/* Remove fork catchpoint target routine.  Nothing to do, just
+   return success.  */
+
+static int
+remote_remove_fork_catchpoint (struct target_ops *ops, int pid)
+{
+  return 0;
+}
+
+/* Insert vfork catchpoint target routine.  If vfork events are enabled
+   then return success, nothing more to do.  */
+
+static int
+remote_insert_vfork_catchpoint (struct target_ops *ops, int pid)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  return !remote_vfork_event_p (rs);
+}
+
+/* Remove vfork catchpoint target routine.  Nothing to do, just
+   return success.  */
+
+static int
+remote_remove_vfork_catchpoint (struct target_ops *ops, int pid)
+{
+  return 0;
+}
+
 /* Tokens for use by the asynchronous signal handlers for SIGINT.  */
 static struct async_signal_handler *async_sigint_remote_twice_token;
 static struct async_signal_handler *async_sigint_remote_token;
@@ -2815,6 +2855,59 @@  remote_get_threads_with_qthreadinfo (struct target_ops *ops,
   return 0;
 }
 
+/* Determine if THREAD is a pending fork parent thread.  ARG contains
+   the pid of the process who's threads we want to check, or -1 if
+   we want to check all threads.  */
+
+static struct thread_info *
+is_pending_fork_parent (struct thread_info *thread, void *arg)
+{
+  int pid = *(int *) arg;
+
+  if (thread->pending_follow.kind == TARGET_WAITKIND_FORKED
+      || thread->pending_follow.kind == TARGET_WAITKIND_VFORKED)
+    {
+      if (pid == -1 || pid == ptid_get_pid (thread->ptid))
+	return thread;
+    }
+
+  return NULL;
+}
+
+/* If CONTEXT contains any fork child threads that have not been
+   reported yet, remove them from the CONTEXT list.  If such a
+   thread exists it is because we are stopped at a fork catchpoint
+   and have not yet called follow_fork, which will set up the
+   host-side data structures for the new process.  */
+
+static void
+remove_new_fork_child (struct threads_listing_context *context)
+{
+  struct thread_info * thread;
+  int pid = -1;
+
+  /* For any in-progress fork parent threads, delete the corresponding
+     fork child threads from the CONTEXT list.  */
+  ALL_NON_EXITED_THREADS (thread)
+    {
+      if (is_pending_fork_parent (thread, &pid) != NULL)
+	{
+	  ptid_t child_ptid = thread->pending_follow.value.related_pid;
+	  struct thread_item *item;
+	  int i;
+
+	  for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
+	    {
+	      if (ptid_equal (item->ptid, child_ptid))
+		{
+		  VEC_ordered_remove (thread_item_t, context->items, i);
+		  break;
+		}
+	    }
+	}
+    }
+}
+
 /* Implement the to_update_thread_list function for the remote
    targets.  */
 
@@ -2839,6 +2932,7 @@  remote_update_thread_list (struct target_ops *ops)
       int i;
       struct thread_item *item;
       struct thread_info *tp, *tmp;
+      struct notif_client *notif = &notif_client_stop;
 
       got_list = 1;
 
@@ -2874,6 +2968,14 @@  remote_update_thread_list (struct target_ops *ops)
 	    }
         }
 
+      /* Remove any unreported fork child threads from CONTEXT so
+	 that we don't interfere with follow fork, which is where
+	 creation of such threads is handled.  We first call
+	 remote_notif_get_pending_events to make sure that we
+	 know about any pending (unreported) fork events.  */
+      remote_notif_get_pending_events (notif);
+      remove_new_fork_child (&context);
+
       /* And now add threads we don't know about yet to our list.  */
       for (i = 0;
 	   VEC_iterate (thread_item_t, context.items, i, item);
@@ -8002,6 +8104,27 @@  extended_remote_kill (struct target_ops *ops)
   int res;
   int pid = ptid_get_pid (inferior_ptid);
   struct remote_state *rs = get_remote_state ();
+  struct thread_info *thread;
+  struct notif_client *notif = &notif_client_stop;
+
+  /* If we're stopped while forking and we haven't followed yet, kill the
+     child task.  We need to do this before killing the parent task
+     because if this is a vfork then the parent will be sleeping.  First
+     we need to call remote_notif_get_pending_events to make sure that we
+     know about any pending (unreported) fork events.  */
+  remote_notif_get_pending_events (notif);
+  ALL_NON_EXITED_THREADS (thread)
+    {
+      if (is_pending_fork_parent(thread, &pid))
+	{
+	  int child_pid;
+
+	  child_pid = ptid_get_pid (thread->pending_follow.value.related_pid);
+	  res = remote_vkill (child_pid, rs);
+	  if (res != 0)
+	    error (_("Can't kill fork child process %d"), child_pid);
+	}
+    }
 
   res = remote_vkill (pid, rs);
   if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
@@ -11917,6 +12040,14 @@  Specify the serial device it is connected to (e.g. /dev/ttya).";
   extended_remote_ops.to_supports_disable_randomization
     = extended_remote_supports_disable_randomization;
   extended_remote_ops.to_follow_fork = remote_follow_fork;
+  extended_remote_ops.to_insert_fork_catchpoint
+    = remote_insert_fork_catchpoint;
+  extended_remote_ops.to_remove_fork_catchpoint
+    = remote_remove_fork_catchpoint;
+  extended_remote_ops.to_insert_vfork_catchpoint
+    = remote_insert_vfork_catchpoint;
+  extended_remote_ops.to_remove_vfork_catchpoint
+    = remote_remove_vfork_catchpoint;
 }
 
 static int