From patchwork Wed Apr 17 17:45:28 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 32322 Received: (qmail 75479 invoked by alias); 17 Apr 2019 17:45:38 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 75388 invoked by uid 89); 17 Apr 2019 17:45:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=sk:print_i, helping, Vries, 24454 X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Apr 2019 17:45:32 +0000 Received: by mail-wm1-f66.google.com with SMTP id y197so4650068wmd.0 for ; Wed, 17 Apr 2019 10:45:31 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4c97:6d52:2cea:997b? ([2001:8a0:f913:f700:4c97:6d52:2cea:997b]) by smtp.gmail.com with ESMTPSA id s189sm6636103wmf.45.2019.04.17.10.45.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 17 Apr 2019 10:45:28 -0700 (PDT) Subject: Re: [PATCH] Handle vfork in thread with follow-fork-mode child To: Tom de Vries , gdb-patches@sourceware.org References: <20190416150652.GA4805@delia> From: Pedro Alves Message-ID: Date: Wed, 17 Apr 2019 18:45:28 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <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 > > 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 > > 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 > +#include > +#include > + > +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 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 Pedro Alves 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 Pedro Alves 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 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 . */ + +#include +#include +#include +#include +#include + +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 . */ + +# 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 . */ + +#include +#include +#include + +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 . */ + +# 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"