[PING][8.3 backport] Handle vfork in thread with follow-fork-mode child

[Tom de Vries <tdevries at suse dot de>]

On 23-07-19 17:04, Tom de Vries wrote:
> On 18-04-19 18:17, Pedro Alves wrote:
>> On 4/18/19 4:48 PM, Tom de Vries wrote:
>>
>>> I've tested this both with native and native-gdbserver and found no
>>> regressions.
>>>
>>> I've also verified that both blocks mentioned above are triggered by the
>>> new test-cases.
>>>
>>> LGTM.
>>>
>> Great, I've pushed it in, ...
>>
>>>> From 575fecd185d07cd0d2f9d9aed5325e7b09b675e0 Mon Sep 17 00:00:00 2001
>>>> From: Pedro Alves <palves@redhat.com>
>>>> Date: Thu, 18 Apr 2019 09:57:45 +0100
>>>> Subject: [PATCH] [gdb] Handle vfork in thread with follow-fork-mode child
>> ... after fixing the authorship back to you.  Looks like squashing in
>> my fixes made stgit/git reset the author and I hadn't noticed.
>>
> Is this assertion fix ok for 8.3.1?
> 

Ping.

Thanks,
- Tom

> The patch doesn't apply cleanly, but the merge conflict is trivial
> (showing here the relevant diff between the patch on master and the
> backport one):
> ...
> $ diff -u master.patch bp.patch
> -@@ -964,7 +968,7 @@ handle_vfork_child_exec_or_exit (int exec)
> +@@ -963,7 +967,7 @@ handle_vfork_child_exec_or_exit (int exec)
>           if (print_inferior_events)
>             {
> -             std::string pidstr
> +             const char *pidstr
>  -              = target_pid_to_str (ptid_t (inf->vfork_parent->pid));
>  +              = target_pid_to_str (ptid_t (vfork_parent->pid));
> 
> @@ -164,7 +162,7 @@
> 
>         if (non_stop && resume_parent != -1)
> ...
> 
> Build on x86_64-linux, and ran test-cases belonging to the patch.
> 
> Thanks,
> - Tom
> 
> 
> 0001-gdb-Handle-vfork-in-thread-with-follow-fork-mode-child.patch
> 
> [gdb] Handle vfork in thread with follow-fork-mode child
> 
> [ Backport of master commit b73715df01. ]
> 
> When debugging any of the testcases added by this commit, which do a
> vfork in a thread with "set follow-fork-mode child" + "set
> detach-on-fork on", we run into this assertion:
> 
> ...
> src/gdb/nat/x86-linux-dregs.c:146: internal-error: \
>   void x86_linux_update_debug_registers(lwp_info*): \
>   Assertion `lwp_is_stopped (lwp)' failed.
> ...
> 
> The assert is caused by the following: the vfork-child exit or exec
> event is handled by handle_vfork_child_exec_or_exit, which calls
> target_detach to detach from the vfork parent.  During target_detach
> we call linux_nat_target::detach, which:
> 
> However, during the second step we run into this code in
> stop_wait_callback:
> 
> ...
>   /* If this is a vfork parent, bail out, it is not going to report
>      any SIGSTOP until the vfork is done with.  */
>   if (inf->vfork_child != NULL)
>     return 0;
> ...
> 
> and we don't wait for the threads to be stopped, which results in this
> assert in x86_linux_update_debug_registers triggering during the third
> step:
> 
> ...
>   gdb_assert (lwp_is_stopped (lwp));
> ...
> 
> The fix is to reset the vfork parent's vfork_child field before
> calling target_detach in handle_vfork_child_exec_or_exit.  There's
> already similar code for the other paths handled by
> handle_vfork_child_exec_or_exit, so this commit refactors the code a
> bit so that all paths share the same code.
> 
> The new tests cover both a vfork child exiting, and a vfork child
> execing, since both cases would trigger the assertion.
> 
> The new testcases also exercise following the vfork children with "set
> detach-on-fork off", since it doesn't seem to be tested anywhere.
> 
> Tested on x86_64-linux, using native and native-gdbserver.
> 
> gdb/ChangeLog:
> 2019-04-18  Tom de Vries  <tdevries@suse.de>
> 	    Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/24454
> 	* infrun.c (handle_vfork_child_exec_or_exit): Reset vfork parent's
> 	vfork_child field before calling target_detach.
> 
> gdb/testsuite/ChangeLog:
> 2019-04-18  Tom de Vries  <tdevries@suse.de>
> 	    Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/24454
> 	* gdb.threads/vfork-follow-child-exec.c: New file.
> 	* gdb.threads/vfork-follow-child-exec.exp: New file.
> 	* gdb.threads/vfork-follow-child-exit.c: New file.
> 	* gdb.threads/vfork-follow-child-exit.exp: New file.
> 
> ---
>  gdb/infrun.c                                       | 31 +++++-----
>  gdb/testsuite/ChangeLog                            |  9 +++
>  .../gdb.threads/vfork-follow-child-exec.c          | 66 ++++++++++++++++++++++
>  .../gdb.threads/vfork-follow-child-exec.exp        | 64 +++++++++++++++++++++
>  .../gdb.threads/vfork-follow-child-exit.c          | 52 +++++++++++++++++
>  .../gdb.threads/vfork-follow-child-exit.exp        | 60 ++++++++++++++++++++
>  6 files changed, 265 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 1efb8d5dc7..9d20036fcf 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -914,10 +914,14 @@ handle_vfork_child_exec_or_exit (int exec)
>        int resume_parent = -1;
>  
>        /* This exec or exit marks the end of the shared memory region
> -	 between the parent and the child.  If the user wanted to
> -	 detach from the parent, now is the time.  */
> +	 between the parent and the child.  Break the bonds.  */
> +      inferior *vfork_parent = inf->vfork_parent;
> +      inf->vfork_parent->vfork_child = NULL;
> +      inf->vfork_parent = NULL;
>  
> -      if (inf->vfork_parent->pending_detach)
> +      /* If the user wanted to detach from the parent, now is the
> +	 time.  */
> +      if (vfork_parent->pending_detach)
>  	{
>  	  struct thread_info *tp;
>  	  struct program_space *pspace;
> @@ -925,7 +929,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  
>  	  /* follow-fork child, detach-on-fork on.  */
>  
> -	  inf->vfork_parent->pending_detach = 0;
> +	  vfork_parent->pending_detach = 0;
>  
>  	  gdb::optional<scoped_restore_exited_inferior>
>  	    maybe_restore_inferior;
> @@ -940,7 +944,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  	    maybe_restore_thread.emplace ();
>  
>  	  /* We're letting loose of the parent.  */
> -	  tp = any_live_thread_of_inferior (inf->vfork_parent);
> +	  tp = any_live_thread_of_inferior (vfork_parent);
>  	  switch_to_thread (tp);
>  
>  	  /* We're about to detach from the parent, which implicitly
> @@ -963,7 +967,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  	  if (print_inferior_events)
>  	    {
>  	      const char *pidstr
> -		= target_pid_to_str (ptid_t (inf->vfork_parent->pid));
> +		= target_pid_to_str (ptid_t (vfork_parent->pid));
>  
>  	      target_terminal::ours_for_output ();
>  
> @@ -981,7 +985,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  		}
>  	    }
>  
> -	  target_detach (inf->vfork_parent, 0);
> +	  target_detach (vfork_parent, 0);
>  
>  	  /* Put it back.  */
>  	  inf->pspace = pspace;
> @@ -996,10 +1000,7 @@ handle_vfork_child_exec_or_exit (int exec)
>  	  inf->removable = 1;
>  	  set_current_program_space (inf->pspace);
>  
> -	  resume_parent = inf->vfork_parent->pid;
> -
> -	  /* Break the bonds.  */
> -	  inf->vfork_parent->vfork_child = NULL;
> +	  resume_parent = vfork_parent->pid;
>  	}
>        else
>  	{
> @@ -1029,17 +1030,13 @@ handle_vfork_child_exec_or_exit (int exec)
>  	  set_current_program_space (pspace);
>  	  inf->removable = 1;
>  	  inf->symfile_flags = SYMFILE_NO_READ;
> -	  clone_program_space (pspace, inf->vfork_parent->pspace);
> +	  clone_program_space (pspace, vfork_parent->pspace);
>  	  inf->pspace = pspace;
>  	  inf->aspace = pspace->aspace;
>  
> -	  resume_parent = inf->vfork_parent->pid;
> -	  /* Break the bonds.  */
> -	  inf->vfork_parent->vfork_child = NULL;
> +	  resume_parent = vfork_parent->pid;
>  	}
>  
> -      inf->vfork_parent = NULL;
> -
>        gdb_assert (current_program_space == inf->pspace);
>  
>        if (non_stop && resume_parent != -1)
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index ea1f78cc4b..5ef34148d9 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,12 @@
> +2019-04-18  Tom de Vries  <tdevries@suse.de>
> +	    Pedro Alves  <palves@redhat.com>
> +
> +	PR gdb/24454
> +	* gdb.threads/vfork-follow-child-exec.c: New file.
> +	* gdb.threads/vfork-follow-child-exec.exp: New file.
> +	* gdb.threads/vfork-follow-child-exit.c: New file.
> +	* gdb.threads/vfork-follow-child-exit.exp: New file.
> +
>  2019-04-30  Tom Tromey  <tromey@adacore.com>
>  
>  	PR c++/24470:
> diff --git a/gdb/testsuite/gdb.threads/vfork-follow-child-exec.c b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.c
> new file mode 100644
> index 0000000000..80632d1772
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.c
> @@ -0,0 +1,66 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2019 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +static char *program_name;
> +
> +static void *
> +f (void *arg)
> +{
> +  int res = vfork ();
> +
> +  if (res == -1)
> +    {
> +      perror ("vfork");
> +      return NULL;
> +    }
> +  else if (res == 0)
> +    {
> +      /* Child.  */
> +      execl (program_name, program_name, "1", NULL);
> +      perror ("exec");
> +      abort ();
> +    }
> +  else
> +    {
> +      /* Parent.  */
> +      return NULL;
> +    }
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  pthread_t tid;
> +
> +  if (argc > 1)
> +    {
> +      /* Getting here via execl.  */
> +      return 0;
> +    }
> +
> +  program_name = argv[0];
> +
> +  pthread_create (&tid, NULL, f, NULL);
> +  pthread_join (tid, NULL);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.threads/vfork-follow-child-exec.exp b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.exp
> new file mode 100644
> index 0000000000..5a28715fa0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.exp
> @@ -0,0 +1,64 @@
> +# Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +# Test following a vfork child that execs, when the vfork parent is a
> +# threaded program, and it's a non-main thread that vforks.
> +
> +standard_testfile
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug pthreads}]} {
> +    return -1
> +}
> +
> +# DETACH indicates whether "set detach-on-fork" is enabled.  It is
> +# either "on" or "off".
> +
> +proc test_vfork {detach} {
> +    global binfile
> +
> +    clean_restart $binfile
> +
> +    if ![runto_main] then {
> +	fail "can't run to main"
> +	return 0
> +    }
> +
> +    delete_breakpoints
> +
> +    gdb_test_no_output "set follow-fork-mode child"
> +    gdb_test_no_output "set detach-on-fork $detach"
> +
> +    if {$detach == "off"} {
> +	gdb_test "continue" \
> +	    [multi_line \
> +		 "Attaching after .* vfork to child .*" \
> +		 ".*New inferior 2 .*" \
> +		 ".* is executing new program: .*" \
> +		 ".*Inferior 2 .* exited normally.*"]
> +    } else {
> +	gdb_test "continue" \
> +	    [multi_line \
> +		 "Attaching after .* vfork to child .*" \
> +		 ".*New inferior 2 .*" \
> +		 ".*Detaching vfork parent process .* after child exec.*" \
> +		 ".*Inferior 1 .* detached.*" \
> +		 ".*is executing new program: .*" \
> +		 ".*Inferior 2 .*exited normally.*"]
> +    }
> +}
> +
> +foreach_with_prefix detach-on-fork {"off" "on"} {
> +    test_vfork ${detach-on-fork}
> +}
> diff --git a/gdb/testsuite/gdb.threads/vfork-follow-child-exit.c b/gdb/testsuite/gdb.threads/vfork-follow-child-exit.c
> new file mode 100644
> index 0000000000..6ae254cce9
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exit.c
> @@ -0,0 +1,52 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2019 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +
> +static void *
> +f (void *arg)
> +{
> +  int res = vfork ();
> +
> +  if (res == -1)
> +    {
> +      perror ("vfork");
> +      return NULL;
> +    }
> +  else if (res == 0)
> +    {
> +      /* Child.  */
> +      _exit (0);
> +    }
> +  else
> +    {
> +      /* Parent.  */
> +      return NULL;
> +    }
> +}
> +
> +int
> +main (void)
> +{
> +  pthread_t tid;
> +
> +  pthread_create (&tid, NULL, f, NULL);
> +  pthread_join (tid, NULL);
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.threads/vfork-follow-child-exit.exp b/gdb/testsuite/gdb.threads/vfork-follow-child-exit.exp
> new file mode 100644
> index 0000000000..f07215d41c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exit.exp
> @@ -0,0 +1,60 @@
> +# Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +# Test following a vfork child that exits, when the vfork parent is a
> +# threaded program, and it's a non-main thread that vforks.
> +
> +standard_testfile
> +
> +if {[build_executable "failed to prepare" $testfile $srcfile {debug pthreads}]} {
> +    return -1
> +}
> +
> +# DETACH indicates whether "set detach-on-fork" is enabled.  It is
> +# either "on" or "off".
> +
> +proc test_vfork {detach} {
> +    global binfile
> +
> +    clean_restart $binfile
> +
> +    if ![runto_main] then {
> +	fail "can't run to main"
> +	return 0
> +    }
> +
> +    gdb_test_no_output "set follow-fork-mode child"
> +    gdb_test_no_output "set detach-on-fork $detach"
> +
> +    if {$detach == "off"} {
> +	gdb_test "continue" \
> +	    [multi_line \
> +		 "Attaching after .* vfork to child .*" \
> +		 ".*New inferior 2 .*" \
> +		 ".*Inferior 2 .*exited normally.*"]
> +    } else {
> +	gdb_test "continue" \
> +	    [multi_line \
> +		 "Attaching after .* vfork to child .*" \
> +		 ".*New inferior 2 .*" \
> +		 ".*Detaching vfork parent process .* after child exit.*" \
> +		 ".*Inferior 1 .* detached.*" \
> +		 ".*Inferior 2 .*exited normally.*"]
> +    }
> +}
> +
> +foreach_with_prefix detach-on-fork {"off" "on"} {
> +    test_vfork ${detach-on-fork}
> +}
>