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

  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.

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.  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.  So I opted for turning follow_fork_inferior into a
target routine.

The updated patch is below.  I tested it on native x64 Ubuntu.  Let me
know what you think.

> 
> Thanks,
> Pedro Alves
> 

Thanks again for your comments,
--Don

gdb/
2014-09-08  Don Breazeal  <donb@codesourcery.com>

	* infrun.c (follow_fork): Call target_follow_fork_inferior.
	(infrun_follow_fork_inferior): New function.
	(follow_inferior_reset_breakpoints): Make function static.
	* infrun.h (infrun_follow_fork_inferior): Declare.
	(follow_inferior_reset_breakpoints): Remove declaration.
	* linux-nat.c (linux_child_follow_fork): Move target-independent
	code to infrun.c:infrun_follow_fork_inferior.
	(linux_target_install_ops): Initialize new target_ops member.
	* target-delegates.c (delegate_follow_fork_inferior): New
	generated function.
	(debug_follow_fork_inferior): New generated function.
	(delegate_follow_fork): New generated function.
	(install_delegators): Initialize new target_ops member.
	(install_dummy_methods): Initialize new target_ops member.
	(init_debug_target): Initialize new target_ops member.
	* target.c (default_follow_fork_inferior): New function.
	(target_follow_fork_inferior): New function.
	* target.h (struct target_ops)<to_follow_fork_inferior>: New
	member.

---
 gdb/infrun.c           | 247
++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/infrun.h           |   5 +-
 gdb/linux-nat.c        | 238
+++++------------------------------------------
 gdb/target-delegates.c |  29 ++++++
 gdb/target.c           |  23 +++++
 gdb/target.h           |   7 +-
 6 files changed, 324 insertions(+), 225 deletions(-)

 /* On some targets, we can catch an inferior exec event when it

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

Commit Message

Comments

Patch