This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Handle vfork in thread with follow-fork-mode child
- From: Pedro Alves <palves at redhat dot com>
- To: Tom de Vries <tdevries at suse dot de>, gdb-patches at sourceware dot org
- Date: Wed, 17 Apr 2019 18:45:28 +0100
- Subject: Re: [PATCH] Handle vfork in thread with follow-fork-mode child
- References: <20190416150652.GA4805@delia>
On 4/16/19 4:06 PM, Tom de Vries wrote:
> Hi,
Hi!
Comments below. As I was reviewing this, I kept experimenting,
so I ended up addressing my own comments myself. See updated patch
at the bottom.
>
> When debugging the test-case vfork-follow-child.c (which does a vfork in a
> thread) with follow-fork-mode child set, 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 event that the vfork child exits,
> 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:
> - stops all the threads
> - waits for all the threads to be stopped
> - detaches all the threads.
> 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 resulting in this
"which results"
> assert in x86_linux_update_debug_registers triggering during the third step:
> ...
> gdb_assert (lwp_is_stopped (lwp));
> ...
>
> Fix this by resetting the vfork parent's vfork_child field before calling
> target_detach in handle_vfork_child_exec_or_exit.
>
> Tested on x86_64-linux, using native and native-gdbserver.
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [gdb] Handle vfork in thread with follow-fork-mode child
>
> gdb/ChangeLog:
>
> 2019-04-16 Tom de Vries <tdevries@suse.de>
>
> PR gdb/24454
> * infrun.c (handle_vfork_child_exec_or_exit): Reset vfork parent's
> avfork_child field before calling target_detach.
Typo "avfork_child".
>
> gdb/testsuite/ChangeLog:
>
> 2019-04-16 Tom de Vries <tdevries@suse.de>
>
> PR gdb/24454
> * gdb.threads/vfork-follow-child.c: New test.
> * gdb.threads/vfork-follow-child.exp: New file.
>
> ---
> gdb/infrun.c | 13 ++++++++++++-
> gdb/testsuite/gdb.threads/vfork-follow-child.c | 19 +++++++++++++++++++
> gdb/testsuite/gdb.threads/vfork-follow-child.exp | 21 +++++++++++++++++++++
> 3 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 37713b24fe..b088138250 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -923,6 +923,7 @@ handle_vfork_child_exec_or_exit (int exec)
> struct thread_info *tp;
> struct program_space *pspace;
> struct address_space *aspace;
> + struct inferior *to_detach;
>
> /* follow-fork child, detach-on-fork on. */
>
> @@ -982,7 +983,17 @@ handle_vfork_child_exec_or_exit (int exec)
> }
> }
>
> - target_detach (inf->vfork_parent, 0);
> + /* Now that the vfork child has terminated, make sure during detach
This path is also reached if the vfork child execs, so the reference to
"terminated" above would better be exec. But also, the other paths in
the function already clear vfork_parent/vfork_child, so I think it's better
to refactor things a bit so that all paths share the code.
> + that we no longer consider the vfork parent to be a vfork parent,
> + but just a regular process that we're detaching from. If not, on
> + linux we would avoid waiting for threads to stop in
> + linux-nat.c:stop_wait_callback, while that was only necessary when
> + the vfork child was still active. */
I'd rather avoid talking about this deep linux-nat.c implementation detail
here. There are other target backends, and, linux-nat.c can change as well,
and it'd be easy to leave this comment stale.
> + to_detach = inf->vfork_parent;
> + inf->vfork_parent->vfork_child = NULL;
> + inf->vfork_parent = NULL;
> +
> + target_detach (to_detach, 0);
>
> /* Put it back. */
> inf->pspace = pspace;
> diff --git a/gdb/testsuite/gdb.threads/vfork-follow-child.c b/gdb/testsuite/gdb.threads/vfork-follow-child.c
> new file mode 100644
> index 0000000000..42c76edd01
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child.c
> @@ -0,0 +1,19 @@
Missing copyright headers in all the new test files.
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <pthread.h>
> +
> +static void *
> +f (void *arg)
> +{
> + vfork ();
vfork children must exit with '_exit', not by returning!
> + 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.exp b/gdb/testsuite/gdb.threads/vfork-follow-child.exp
> new file mode 100644
> index 0000000000..26896975af
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-follow-child.exp
> @@ -0,0 +1,21 @@
Copyright header.
> +if { ! [istarget "*-*-linux*"] } {
> + return 0
> +}
I understand that you probably copied this from elsewhere, but I'd
rather just remove it. Other ports support follow fork/vfork, and
nobody ever remembers to remove these restrictions in test cases.
That's why typically blacklisting is better than whitelisting.
In any case, I think a new supports_follow_fork procedure or
some such, and then use that in all relevant testcases would be
much better than sprinkling around istarget checks.
> +
> +standard_testfile
> +
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> + executable {debug}] != "" } {
> + return -1
> +}
> +
> +clean_restart ${binfile}
We can use prepare_for_testing.
> +
> +if ![runto_main] then {
> + fail "can't run to main"
> + return 0
> +}
> +
> +gdb_test "set follow-fork-mode child"
Here it's better to use gdb_test_no_output. As you have it,
any output, except a crash, would pass.
> +gdb_test "continue" "" "continue"
Here, I understand that you're only caring for not crashing
gdb, but I think it's good practice to make the test's regex
tighter. That'll will help with better coverage, helping
future development.
Also, as the name suggests, handle_vfork_child_exec_or_exit
handles both execs and exists similarly, but the new testcase
only handled exit. I've added a new variant of the testcase
for execs as well.
I've also done some minor tweaks to the commit log.
Here's the updated patch. WDYT?
>From a66460444e1d5a27f82d676b1f2d1b030ff7454e Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 17 Apr 2019 17:37:09 +0100
Subject: [PATCH] [gdb] Handle vfork in thread with follow-fork-mode child
When debugging any of the testcases added by this commit, which do a
vfork in a thread, with follow-fork-mode child set, 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:
#1 - stops all the threads
#2 - waits for all the threads to be stopped
#3 - detaches all the threads
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.
Tested on x86_64-linux, using native and native-gdbserver.
gdb/ChangeLog:
yyyy-mm-dd 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:
yyyy-mm-dd 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 | 27 ++++-----
.../gdb.threads/vfork-follow-child-exec.c | 66 ++++++++++++++++++++++
.../gdb.threads/vfork-follow-child-exec.exp | 42 ++++++++++++++
.../gdb.threads/vfork-follow-child-exit.c | 52 +++++++++++++++++
.../gdb.threads/vfork-follow-child-exit.exp | 39 +++++++++++++
5 files changed, 211 insertions(+), 15 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/vfork-follow-child-exec.c
create mode 100644 gdb/testsuite/gdb.threads/vfork-follow-child-exec.exp
create mode 100644 gdb/testsuite/gdb.threads/vfork-follow-child-exit.c
create mode 100644 gdb/testsuite/gdb.threads/vfork-follow-child-exit.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 37713b24fee..09049ca756a 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -915,10 +915,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;
@@ -926,7 +930,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;
@@ -941,7 +945,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
@@ -964,7 +968,7 @@ handle_vfork_child_exec_or_exit (int exec)
if (print_inferior_events)
{
std::string 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 ();
@@ -982,7 +986,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;
@@ -998,9 +1002,6 @@ handle_vfork_child_exec_or_exit (int exec)
set_current_program_space (inf->pspace);
resume_parent = inf->vfork_parent->pid;
-
- /* Break the bonds. */
- inf->vfork_parent->vfork_child = NULL;
}
else
{
@@ -1034,13 +1035,9 @@ handle_vfork_child_exec_or_exit (int exec)
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/gdb.threads/vfork-follow-child-exec.c b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.c
new file mode 100644
index 00000000000..80632d1772f
--- /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 00000000000..18c58d5743f
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exec.exp
@@ -0,0 +1,42 @@
+# 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 {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}]} {
+ return -1
+}
+
+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 "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.*"] \
+ "continue"
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 00000000000..6ae254cce96
--- /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 00000000000..9773afcbc11
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-follow-child-exit.exp
@@ -0,0 +1,39 @@
+# 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 {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}]} {
+ return -1
+}
+
+if ![runto_main] then {
+ fail "can't run to main"
+ return 0
+}
+
+gdb_test_no_output "set follow-fork-mode child"
+
+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.*"] \
+ "continue"
--
2.14.5