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

[Pedro Alves <palves@redhat.com>]

On 09/09/2014 12:54 AM, Breazeal, Don wrote:
> Hi Pedro,
> 
> I've consolidated my responses to the issues you raised in this email,
> and attached an updated patch containing the proposed solutions.
> 
> 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.  Then on entry, linux_child_follow_fork expects inferior_ptid
> to be the followed inferior.  So I think it is getting the expected
> inferior from inferior_ptid in these cases.
> 
> In the original code, inferior_ptid was the parent on entry to
> target_follow_fork, and the followed inferior after return.  I made
> follow_fork_inferior do the same thing (return the followed inferior),
> but it would be no problem to have it return the parent and have
> linux_child_follow_fork expect inferior_ptid to be the parent and return
> the followed inferior (like it did before) if that would be preferable.
> Let me know if you'd like me to make that change.
> 
> Regarding check_for_thread_db, there is something unrelated that I don't
> understand.  Maybe you can clear this up for me.  If we have reached
> linux_child_follow_fork, 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?
> 
>   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?
> 
> static int
> linux_child_follow_fork (struct target_ops *ops, int follow_child,
>                          int detach_fork)
> {
>   if (!follow_child)
>     {
>       struct lwp_info *child_lp = NULL;
>       int status = W_STOPCODE (0);
>       struct cleanup *old_chain;
>       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
>>
>> These files / targets also have to_follow_fork implementations:
>>
>>  inf-ptrace.c:  t->to_follow_fork = inf_ptrace_follow_fork;
>>  inf-ttrace.c:  t->to_follow_fork = inf_ttrace_follow_fork;
>>
>> which will break if we don't adjust them as well.  Did you
>> check whether the refactored code (follow_fork_inferior)
>> makes sense for those?
> 
> I completely missed these; sorry about that.  In my initial response to
> your review, I thought I should be able to make similar changes to these
> functions that maintained the existing functionality.  After digging into
> this, I still think it is possible, but that a more prudent approach
> would be to make follow_fork_inferior into a new target hook that just
> returns zero for the non-Linux cases.

I'd rather not.  That'll just add more technical debt.  :-)  E.g.,
if we can't model this on the few native targets we have, then that'd
indicate that remote debugging against one of those targets (when
we get to gdb/gdbserver parity), wouldn't be correctly modelled, as
you'd be installing those methods in remote.c too.  Plus, if we have
target_follow_fork_inferior as an extra method in addition
to target_follow_fork_inferior, and both are always called in
succession, then we might as well _not_ introduce a new target hook,
and just call infrun_follow_fork_inferior from the
top of linux-nat.c:linux_child_follow_fork, and the top of whatever
other target's to_follow_fork method that wants to use it.

> The changes to inf_ptrace_follow_fork to get it to work with
> follow_fork_inferior are straightforward.  Making those changes to
> inf_ttrace_follow_fork is problematic, primarily because it handles the
> moral equivalent of the vfork-done event differently.

Can you summarize the differences ?

> The changes
> required to make it work with follow_fork_inferior are more significant
> than I'd like, given that I don't have any way to test how the OS
> reports the events.  

Don't worry so much about the testing.  We can go best effort, and
if someone can help with testing, good.  If not, I'd still push
forward and have interested parties fix things up when they
stumble on issues.  I'm mostly interested in checking whether
the model we're committing to makes sense and is at the right level.

Thanks,
Pedro Alves