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

["Breazeal, Don" <donb at codesourcery dot com>]

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.  On entry, linux_child_follow_fork expects inferior_ptid to be
the followed inferior.  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?

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?

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?

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
()->pending_follow.value.related_pid);

> 
> 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 theory I should be able
to make similar changes to these that maintains the existing
functionality.  I don't currently have a way (that I know of) to test
either of them.  Testing requires a non-Linux version of Unix and an
HP-UX system, correct?  I'll start work on the changes in spite of that.

> 
> Thanks,
> Pedro Alves
> 

Thanks,
--Don