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

  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?

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
  

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

On 7/23/19 4:04 PM, Tom de Vries wrote:

> Is this assertion fix ok for 8.3.1?

OK.

Thanks,
Pedro Alves