This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v8 6/7] Remote fork catch
- From: Don Breazeal <donb at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>, "Breazeal, Don" <Don_Breazeal at mentor dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 8 May 2015 09:41:54 -0700
- Subject: Re: [PATCH v8 6/7] Remote fork catch
- Authentication-results: sourceware.org; auth=none
- References: <1430928624-26093-1-git-send-email-donb at codesourcery dot com> <554C8C67 dot 8010504 at redhat 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 = ¬if_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