Re: [PATCH 01/16 v2] Refactor native follow-fork

Pedro Alves
2014-09-09T10:57:34Z

On 09/05/2014 07:56 PM, Breazeal, Don wrote:
> Hi Pedro,
> Thanks for reviewing this.
>
> On 9/5/2014 7:20 AM, Pedro Alves wrote:
>> linux_child_follow_fork ends up with:
>>
>> static int
>> linux_child_follow_fork (struct target_ops *ops, int follow_child,
>> int detach_fork)
>> {
>> int has_vforked;
>> int parent_pid, child_pid;
>>
>> has_vforked = (inferior_thread ()->pending_follow.kind
>> == TARGET_WAITKIND_VFORKED);
>> parent_pid = ptid_get_lwp (inferior_ptid);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> if (parent_pid == 0)
>> parent_pid = ptid_get_pid (inferior_ptid);
>> child_pid
>> = ptid_get_pid (inferior_thread ()->pending_follow.value.related_pid);
>>
>> if (!follow_child)
>> {
>> ...
>> }
>> else
>> {
>> struct lwp_info *child_lp;
>>
>> child_lp = add_lwp (inferior_ptid);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> child_lp->stopped = 1;
>> child_lp->last_resume_kind = resume_stop;
>>
>> /* Let the thread_db layer learn about this new process. */
>> check_for_thread_db ();
>> }
>> }
>>
>> Nothing appears to switch inferior_ptid to the child, so seems
>> like we're adding the child_lp with the wrong lwp (and calling
>> check_for_thread_db in the wrong context) ? Is this managing
>> to work by chance because follow_fork_inferior leaves inferior_ptid
>> pointing to the child?
>
> Yes, follow_fork_inferior always sets inferior_ptid to the followed
> inferior.

Ah.

> On entry, linux_child_follow_fork expects inferior_ptid to be
> the followed inferior.

I see. It does make sense.

> So I think it is getting the correct inferior
> from inferior_ptid in these cases. I can change that if you prefer; see
> my question below about acceptable solutions.
>
> Regarding check_for_thread_db, there is something unrelated that I don't
> understand here. If we have reached this function, then aren't we
> guaranteed that PTRACE_O_TRACECLONE is supported, and that we are using
> that instead of libthread_db for detecting thread events? If so, why do
> we need to call check_for_thread_db at all?

Unlike GDBserver, GDB actually still use libthread_db's create/exit event
breakpoints even if PTRACE_O_TRACECLONE is supported. I think this is
just historical at this point, and we could skip those event breakpoints,
optimizing out a bunch of internal stops.

The check_for_thread_db call has another effect -- as mentioned in the
comment:

/* Let the thread_db layer learn about this new process. */
check_for_thread_db ();

if we don't call it, then linux-thread-db.c never adds the child
process to its global thread_db_list list:

/* List of known processes using thread_db, and the required
bookkeeping. */
struct thread_db_info *thread_db_list;

We don't really need to try all the available libthread_db's found
in the path, we could just try try_thread_db_load with the same
libthread_db we had loaded for the parent, or even skip that
and share/refcount the bookkeeping in 'struct thread_db_info'
(dlopen handle, functions pointers, etc.) between parent and child.

This is about the same issue as mentioned just above:

/* Let the shared library layer (solib-svr4) learn about
this new process, relocate the cloned exec, pull in
shared libraries, and install the solib event breakpoint.
If a "cloned-VM" event was propagated better throughout
the core, this wouldn't be required. */

>
> Then this at the top uses the wrong
>> inferior_thread ():
>>
>> has_vforked = (inferior_thread ()->pending_follow.kind
>> == TARGET_WAITKIND_VFORKED);
>>
>>
>> and we're lucky that nothing end up using has_vforked in the
>> follow child path?
>
> You are right, this is incorrect and unnecessary in the case where we
> are following the child.
>
>>
>> I'd much rather we don't have these assumptions in place.
>
> Would an acceptable solution be to move the definitions and assignments
> of has_vforked, parent_pid, and child_pid into the follow-parent case,
> as below?

Yes.

>
> Would you also prefer that on entry to linux_child_follow_fork,
> inferior_ptid is set to the parent like it was before, or would a
> comment explaining that inferior_ptid is expected to be the followed
> inferior be sufficient?

The comment would be sufficient.

Thanks,
Pedro Alves