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

[Pedro Alves <palves at redhat dot com>]

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!

-- 
Pedro Alves