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

[Pedro Alves <palves@redhat.com>]

On 09/12/2014 05:50 PM, Breazeal, Don wrote:
> On 9/9/2014 4:09 AM, Pedro Alves wrote:
>> On 09/09/2014 12:54 AM, Breazeal, Don wrote:

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

Thanks.

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

Right.  Linux also used to handle that in-line, before 6c95b8df.

> 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". :-)

Excellent.  That sure made it easier.


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

Yeah, I can't imagine any way a TTEVT_VFORK could ever be reported to
the parent first.  When the parent vforks, it blocks until the child
execs or exits.  And the child won't do that until GDB resumes
the child after handling the TTEVT_VFORK that is reported for the
child.  So the kernel must always report the child's TTEVT_VFORK first.


>    if (follow_child)
>      {
>        struct thread_info *ti;
> @@ -533,17 +423,22 @@ inf_ttrace_follow_fork (struct target_ops *ops,
> int follow_child,
>        inf_ttrace_num_lwps = 1;
>        inf_ttrace_num_lwps_in_syscall = 0;
> 
> -      /* Delete parent.  */
> -      delete_thread_silent (ptid_build (pid, lwpid, 0));
> -      detach_inferior (pid);
> -
> -      /* Add child thread.  inferior_ptid was already set above.  */
> -      ti = add_thread_silent (inferior_ptid);
> +      ti = find_thread_ptid (inferior_ptid);
> +      gdb_assert (ti != NULL);

Replace these with:

         ti = inferior_thread ();

>        ti->private =
>  	xmalloc (sizeof (struct inf_ttrace_private_thread_info));
>        memset (ti->private, 0,
>  	      sizeof (struct inf_ttrace_private_thread_info));
>      }
> +  else
> +    {
> +      pid_t child_pid;
> +
> +      /* Following parent.  Detach child now.  */
> +      child_pid = ptid_get_pid (tp->pending_follow.value.related_pid);
> +      if (ttrace (TT_PROC_DETACH, child_pid, 0, 0, 0, 0) == -1)
> +	perror_with_name (("ttrace"));
> +    }
> 
>    return 0;
>  }
> @@ -661,7 +556,6 @@ inf_ttrace_create_inferior (struct target_ops *ops,
> char *exec_file,
>    gdb_assert (inf_ttrace_num_lwps_in_syscall == 0);
>    gdb_assert (inf_ttrace_page_dict.count == 0);
>    gdb_assert (inf_ttrace_reenable_page_protections == 0);
> -  gdb_assert (inf_ttrace_vfork_ppid == -1);
> 
>    pid = fork_inferior (exec_file, allargs, env, inf_ttrace_me, NULL,
>  		       inf_ttrace_prepare, NULL, NULL);
> @@ -772,7 +666,6 @@ inf_ttrace_attach (struct target_ops *ops, const
> char *args, int from_tty)
> 
>    gdb_assert (inf_ttrace_num_lwps == 0);
>    gdb_assert (inf_ttrace_num_lwps_in_syscall == 0);
> -  gdb_assert (inf_ttrace_vfork_ppid == -1);
> 
>    if (ttrace (TT_PROC_ATTACH, pid, 0, TT_KILL_ON_EXIT, TT_VERSION, 0)
> == -1)
>      perror_with_name (("ttrace"));
> @@ -822,11 +715,12 @@ inf_ttrace_detach (struct target_ops *ops, const
> char *args, int from_tty)
>    if (ttrace (TT_PROC_DETACH, pid, 0, 0, sig, 0) == -1)
>      perror_with_name (("ttrace"));
> 
> -  if (inf_ttrace_vfork_ppid != -1)
> +  if (current_inferior ()->vfork_parent != NULL)
>      {
> -      if (ttrace (TT_PROC_DETACH, inf_ttrace_vfork_ppid, 0, 0, 0, 0) == -1)
> +      pid_t ppid = current_inferior ()->vfork_parent->pid;
> +      if (ttrace (TT_PROC_DETACH, ppid, 0, 0, 0, 0) == -1)
>  	perror_with_name (("ttrace"));
> -      inf_ttrace_vfork_ppid = -1;
> +      detach_inferior (ppid);
>      }

I think we should just delete this block instead.
We shouldn't be blindly detaching from the parent here.
The user may well still want to debug it.  The case of the
user doing "detach" on the child when the parent is waiting
for the the vfork-done should be handled by common code.
(and we should be going through the whole target_detach).

> 
>    inf_ttrace_num_lwps = 0;
> @@ -850,11 +744,12 @@ inf_ttrace_kill (struct target_ops *ops)
>      perror_with_name (("ttrace"));
>    /* ??? Is it necessary to call ttrace_wait() here?  */
> 
> -  if (inf_ttrace_vfork_ppid != -1)
> +  if (current_inferior ()->vfork_parent != NULL)
>      {
> -      if (ttrace (TT_PROC_DETACH, inf_ttrace_vfork_ppid, 0, 0, 0, 0) == -1)
> +      pid_t ppid = current_inferior ()->vfork_parent->pid;
> +      if (ttrace (TT_PROC_DETACH, ppid, 0, 0, 0, 0) == -1)
>  	perror_with_name (("ttrace"));
> -      inf_ttrace_vfork_ppid = -1;
> +      detach_inferior (ppid);
>      }

This too.

> 
>    target_mourn_inferior ();
> @@ -967,20 +862,6 @@ inf_ttrace_wait (struct target_ops *ops,
>        if (ttrace_wait (pid, lwpid, TTRACE_WAITOK, &tts, sizeof tts) == -1)
>  	perror_with_name (("ttrace_wait"));
> 
> -      if (tts.tts_event == TTEVT_VFORK && tts.tts_u.tts_fork.tts_isparent)
> -	{
> -	  if (inf_ttrace_vfork_ppid != -1)
> -	    {
> -	      gdb_assert (inf_ttrace_vfork_ppid == tts.tts_pid);
> -
> -	      if (ttrace (TT_PROC_DETACH, tts.tts_pid, 0, 0, 0, 0) == -1)
> -		perror_with_name (("ttrace"));
> -	      inf_ttrace_vfork_ppid = -1;
> -	    }
> -
> -	  tts.tts_event = TTEVT_NONE;
> -	}
> -
>        clear_sigint_trap ();
>      }
>    while (tts.tts_event == TTEVT_NONE);
> @@ -1075,17 +956,23 @@ inf_ttrace_wait (struct target_ops *ops,
>        break;
> 
>      case TTEVT_VFORK:
> -      gdb_assert (!tts.tts_u.tts_fork.tts_isparent);
> -
> -      related_ptid = ptid_build (tts.tts_u.tts_fork.tts_fpid,
> -				 tts.tts_u.tts_fork.tts_flwpid, 0);
> +      if (tts.tts_u.tts_fork.tts_isparent)
> +	{
> +	  if (current_inferior ()->waiting_fork_vfork_done)

I don't think we should have this waiting_fork_vfork_done check here.
This should always be a TARGET_WAITKIND_VFORK_DONE?

> +	    ourstatus->kind = TARGET_WAITKIND_VFORK_DONE;
> +	}
> +      else
> +	{
> +	  related_ptid = ptid_build (tts.tts_u.tts_fork.tts_fpid,
> +				     tts.tts_u.tts_fork.tts_flwpid, 0);
> 
> -      ourstatus->kind = TARGET_WAITKIND_VFORKED;
> -      ourstatus->value.related_pid = related_ptid;
> +	  ourstatus->kind = TARGET_WAITKIND_VFORKED;
> +	  ourstatus->value.related_pid = related_ptid;
> 
> -      /* HACK: To avoid touching the parent during the vfork, switch
> -	 away from it.  */
> -      inferior_ptid = ptid;
> +	  /* HACK: To avoid touching the parent during the vfork, switch
> +	     away from it.  */
> +	  inferior_ptid = ptid;

The core is now aware of the vfork parent inferior.  Just delete
this hack.

> +	}
>        break;
> 
>      case TTEVT_LWP_CREATE:
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c18267f..af9cbf8 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -60,6 +60,7 @@
>  #include "completer.h"
>  #include "target-descriptions.h"
>  #include "target-dcache.h"
> +#include "terminal.h"
> 
>  /* Prototypes for local functions */
> 
> @@ -79,6 +80,10 @@ static int restore_selected_frame (void *);
> 
>  static int follow_fork (void);
> 
> +static int follow_fork_inferior (int follow_child, int detach_fork);
> +
> +static void follow_inferior_reset_breakpoints (void);
> +
>  static void set_schedlock_func (char *args, int from_tty,
>  				struct cmd_list_element *c);
> 
> @@ -486,9 +491,11 @@ follow_fork (void)
>  	parent = inferior_ptid;
>  	child = tp->pending_follow.value.related_pid;
> 
> -	/* Tell the target to do whatever is necessary to follow
> -	   either parent or child.  */
> -	if (target_follow_fork (follow_child, detach_fork))
> +	/* Set up inferior(s) as specified by the caller, and tell the
> +	   target to do whatever is necessary to follow either parent
> +	   or child.  */
> +	if (follow_fork_inferior (follow_child, detach_fork)
> +	    || target_follow_fork (follow_child, detach_fork))

I don't think we should ever call follow_fork_inferior without
calling target_follow_fork, right?  I think it'd be clearer if
we tail called target_follow_fork inside follow_fork_inferior.

>  	  {
>  	    /* Target refused to follow, or there's some other reason
>  	       we shouldn't resume.  */
> @@ -560,7 +567,242 @@ follow_fork (void)
>    return should_resume;
>  }
> 
> -void
> +/* Handle changes to the inferior list based on the type of fork,
> +   which process is being followed, and whether the other process
> +   should be detached.  On entry inferior_ptid must be the ptid of
> +   the fork parent.  At return inferior_ptid is the ptid of the
> +   followed inferior.  */
> +
> +int
> +follow_fork_inferior (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 (has_vforked
> +      && !non_stop /* Non-stop always resumes both branches.  */
> +      && (!target_is_async_p () || sync_execution)
> +      && !(follow_child || detach_fork || sched_multi))
> +    {
> +      /* The parent stays blocked inside the vfork syscall until the
> +	 child execs or exits.  If we don't let the child run, then
> +	 the parent stays blocked.  If we're telling the parent to run
> +	 in the foreground, the user will not be able to ctrl-c to get
> +	 back the terminal, effectively hanging the debug session.  */
> +      fprintf_filtered (gdb_stderr, _("\
> +Can not resume the parent process over vfork in the foreground while\n\
> +holding the child stopped.  Try \"set detach-on-fork\" or \
> +\"set schedule-multiple\".\n"));
> +      /* FIXME output string > 80 columns.  */
> +      return 1;
> +    }
> +

Moving this to common code isn't strictly correct, as we'd probably
be able to interrupt the vfork parent in this case when remote
debugging, or not sharing the terminal with the inferior.  But let's
put this here for now.


> +	  else
> +	    {
> +	      child_inf->aspace = new_address_space ();
> +	      child_inf->pspace = add_program_space (child_inf->aspace);
> +	      child_inf->removable = 1;
> +	      set_current_program_space (child_inf->pspace);
> +	      clone_program_space (child_inf->pspace, parent_inf->pspace);
> +
> +	      /* Let the shared library layer (solib-svr4) learn about

It's not always solib-svr4, so make that e.g., "e.g., solib-svr4" now.

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

Likewise.

Otherwise this looks good to me.

Thanks,
Pedro Alves