Re: [PATCH v8 6/7] Remote fork catch

[Don Breazeal <donb at codesourcery dot com>]

On 5/8/2015 3:13 AM, Pedro Alves wrote:
> On 05/06/2015 05:10 PM, Don Breazeal wrote:
> 
>> I've reimplemented the mechanisms that (a) ignore new threads
>> that are as-yet-unreported fork children, and (b) kill as-yet-
>> unreported fork children when killing the fork parent.  These both
>> now check the stop reply queue for unreported fork events.  I changed
>> the names of the new functions to use 'ignore' instead of 'remove'
>> to more accurately reflect what they are doing, and moved
>> remote_update_thread_list to avoid having to add prototypes and
>> forward declarations to satisfy all the dependences.
> 
> I understand what you were trying to avoid, but I really don't like
> the move: the function is currently alongside a bunch of related code
> and ends up somewhere quite unrelated.  I'd rather have the
> forward declarations.  You're already adding other forward
> declarations anyway.
> 
> Also, why does "ignore" more accurately reflect what the functions
> are doing?  E.g.,:
> 
>> +/* Remove the thread specified as the related_pid field of WS
>> +   from the CONTEXT list.  */
>> +
>> +static void
>> +ignore_new_fork_child (struct target_waitstatus *ws,
>> +		       struct threads_listing_context *context)
>> +{
>> +  struct thread_item *item;
>> +  int i;
>> +  ptid_t child_ptid = ws->value.related_pid;
>> +
>> +  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;
>> +	}
>> +    }
>> +}
>> +
> 
> That's clearly removing something from a list, as even the comment says.
> 
> How about instead adding:
> 
> static void
> threads_listing_context_remove (struct threads_listing_context *context,
> 				ptid_t remove_ptid)
> {
>   struct thread_item *item;
>   int i;
> 
>   for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
>     {
>       if (ptid_equal (item->ptid, remove_ptid))
> 	{
> 	  VEC_ordered_remove (thread_item_t, context->items, i);
> 	  break;
> 	}
>     }
> }
> 
> after clear_threads_listing_context, and then doing:
> 
> /* Check whether EVENT is a fork event, and if it is, remove the
>    fork child from the context list passed in DATA.  */
> 
> static int
> remove_child_of_pending_fork (QUEUE (stop_reply_p) *q,
> 			      QUEUE_ITER (stop_reply_p) *iter,
> 			      stop_reply_p event,
> 			      void *data)
> {
>   struct queue_iter_param *param = data;
>   struct threads_listing_context *context = param->input;
> 
>   if (event->ws.kind == TARGET_WAITKIND_FORKED
>       || event->ws.kind == TARGET_WAITKIND_VFORKED)
>     {
>       threads_listing_context_remove (context,
> 				      &event->ws->value.related_pid);
>     }
> 
>   return 1;
> }
> 
> etc.
> 
>> +
>> +static void
>> +kill_new_fork_children (int pid, struct remote_state *rs)
>> +{
>> +  struct thread_info *thread;
>> +  struct notif_client *notif = &notif_client_stop;
>> +  struct queue_iter_param param;
>> +
>> +  /* Kill the fork child threads of any threads in process PID
>> +     that are stopped at a fork event.  */
>> +  ALL_NON_EXITED_THREADS (thread)
>> +    {
>> +      struct target_waitstatus *ws = &thread->pending_follow;
>> +
>> +      if (is_pending_fork_parent(ws, pid, thread->ptid))
> 
> Space before parens.
> 
> Otherwise this is good to go.  Thanks!
> 
Thanks Pedro.  Your suggested function naming makes sense; it eliminates
any ambiguity about whether the thread is the actual running thread or
an element in the thread list.

I think that with this patch completed we are done with this patchset.
I will make these changes and push it all in.  My plan after that is to:

 -> resurrect the extended-remote follow exec patches (minus the exit
event stuff) and work through any issues with those

 -> then revisit the 'target remote' patch for both follow fork and
follow exec

Let me know if there's any reason for me to do things in a different order.

Thanks again for all the reviews and help!
--Don