[01/16,v2] Refactor native follow-fork

  On 9/9/2014 4:09 AM, Pedro Alves wrote:
> 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?

This change was made per your previous comments, and comments added to
specify how inferior_ptid is changed by follow_fork_inferior and the
target follow_fork routines.

>>
>> 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.

I've made modifications to inf_ttrace_follow_fork, inf_ttrace_wait,
and inf_ptrace_follow_fork in the included patch that I think should
allow them to work with follow_fork_inferior.  Some other adjustments
were necessary in inf-ttrace.c.  Some of the changes weren't strictly
necessary to get things working with follow_fork_inferior, but it
seemed appropriate to make all the implementations more consistent.

> 
>> 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 ?

HP-UX apparently delivers a TTEVT_VFORK event for the parent inferior
where Linux would report PTRACE_EVENT_VFORK_DONE -- when the child
process has either execd or exited.  inf_ttrace_follow_fork was handling
the parent VFORK event in-line, where the Linux implementation expects
a TARGET_WAITKIND_VFORK_DONE event to be reported so it can be handled
in the generic event code.

> 
>> 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.

I haven't tested the changes to either of inf-ptrace.c or inf-ttrace.c,
or even built them, lacking a suitable environment.  I did re-test on
x64 Ubuntu.

I've included annotated versions of the original implementations of
inf_ttrace_follow_fork and inf-ptrace_follow_fork below for reference,
to explain how I made decisions on the changes.  Each annotation is
a comment beginning with "BLOCK n".  If you want to skip past all of
that you can just search for "Don". :-)

> 
> Thanks,
> Pedro Alves
> 

My assumptions about how HP-UX ttrace events work:
- FORK:
  - TTEVT_FORK is reported for both the parent and child processes.
    The event can be reported for the parent or child in any order.

- VFORK:
  - TTEVT_VFORK is reported for both the parent and child
    processes.
  - TTEVT_VFORK is reported for the child first.  It is reported
    for the parent when the vfork is "done" as with the Linux
    PTRACE_EVENT_VFORK_DONE event, meaning that the parent has
    called exec or exited.  See this comment in inf_ttrace_follow_fork:

  /* Wait till we get the TTEVT_VFORK event in the parent.
     This indicates that the child has called exec(3) or has
     exited and that the parent is ready to be traced again.  */

    The online HP-UX ttrace documentation doesn't really make this
    ordering explicit, but it doesn't contradict the implementation.

 	  gdb_assert (linux_supports_tracefork () >= 0);
@@ -628,98 +523,12 @@ holding the child stopped.  Try \"set
detach-on-fork\" or \
     }
   else
     {
-      struct inferior *parent_inf, *child_inf;
       struct lwp_info *child_lp;
-      struct program_space *parent_pspace;
-
-      if (info_verbose || debug_linux_nat)
-	{
-	  target_terminal_ours ();
-	  if (has_vforked)
-	    fprintf_filtered (gdb_stdlog,
-			      _("Attaching after process %d "
-				"vfork to child process %d.\n"),
-			      parent_pid, child_pid);
-	  else
-	    fprintf_filtered (gdb_stdlog,
-			      _("Attaching after process %d "
-				"fork to child process %d.\n"),
-			      parent_pid, child_pid);
-	}
-
-      /* Add the new inferior first, so that the target_detach below
-	 doesn't unpush the target.  */
-
-      child_inf = add_inferior (child_pid);
-
-      parent_inf = current_inferior ();
-      child_inf->attach_flag = parent_inf->attach_flag;
-      copy_terminal_info (child_inf, parent_inf);
-      child_inf->gdbarch = parent_inf->gdbarch;
-      copy_inferior_target_desc_info (child_inf, parent_inf);
-
-      parent_pspace = parent_inf->pspace;
-
-      /* If we're vforking, we want to hold on to the parent until the
-	 child exits or execs.  At child exec or exit time we can
-	 remove the old breakpoints from the parent and detach or
-	 resume debugging it.  Otherwise, detach the parent now; we'll
-	 want to reuse it's program/address spaces, but we can't set
-	 them to the child before removing breakpoints from the
-	 parent, otherwise, the breakpoints module could decide to
-	 remove breakpoints from the wrong process (since they'd be
-	 assigned to the same address space).  */

-      if (has_vforked)
-	{
-	  gdb_assert (child_inf->vfork_parent == NULL);
-	  gdb_assert (parent_inf->vfork_child == NULL);
-	  child_inf->vfork_parent = parent_inf;
-	  child_inf->pending_detach = 0;
-	  parent_inf->vfork_child = child_inf;
-	  parent_inf->pending_detach = detach_fork;
-	  parent_inf->waiting_for_vfork_done = 0;
-	}
-      else if (detach_fork)
-	target_detach (NULL, 0);
-
-      /* Note that the detach above makes PARENT_INF dangling.  */
-
-      /* Add the child thread to the appropriate lists, and switch to
-	 this new thread, before cloning the program space, and
-	 informing the solib layer about this new process.  */
-
-      inferior_ptid = ptid_build (child_pid, child_pid, 0);
-      add_thread (inferior_ptid);
       child_lp = add_lwp (inferior_ptid);
       child_lp->stopped = 1;
       child_lp->last_resume_kind = resume_stop;

-      /* If this is a vfork child, then the address-space is shared
-	 with the parent.  If we detached from the parent, then we can
-	 reuse the parent's program/address spaces.  */
-      if (has_vforked || detach_fork)
-	{
-	  child_inf->pspace = parent_pspace;
-	  child_inf->aspace = child_inf->pspace->aspace;
-	}
-      else
-	{
-	  child_inf->aspace = new_address_space ();
-	  child_inf->pspace = add_program_space (child_inf->aspace);
-	  child_inf->removable = 1;
-	  child_inf->symfile_flags = SYMFILE_NO_READ;
-	  set_current_program_space (child_inf->pspace);
-	  clone_program_space (child_inf->pspace, parent_pspace);
-
-	  /* 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.  */
-	  solib_create_inferior_hook (0);
-	}
-
       /* Let the thread_db layer learn about this new process.  */
       check_for_thread_db ();
     }
  

[01/16,v2] Refactor native follow-fork

Commit Message

Comments

Patch