* [PATCH v2 1/9] gdb/infrun: add reason parameter to stop_all_threads
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 2/9] gdb/linux-nat: remove check based on current_inferior in linux_handle_extended_wait Simon Marchi
` (9 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
Add a "reason" parameter, only used to show in debug messages what is
the reason for stopping all threads. This helped me understand the
debug logs while adding some new uses of stop_all_threads, so I am
proposing to merge it.
Change-Id: I66c8c335ebf41836a7bc3d5fe1db92c195f65e55
---
gdb/infcmd.c | 2 +-
gdb/infrun.c | 10 +++++-----
gdb/infrun.h | 11 +++++++----
gdb/remote.c | 2 +-
4 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 9f4ed8bff138..16b1ca65d812 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2473,7 +2473,7 @@ attach_post_wait (int from_tty, enum attach_post_wait_mode mode)
{
struct thread_info *lowest = inferior_thread ();
- stop_all_threads ();
+ stop_all_threads ("attaching");
/* It's not defined which thread will report the attach
stop. For consistency, always select the thread with
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2335fc74dc02..46e14d720cce 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2391,7 +2391,7 @@ resume_1 (enum gdb_signal sig)
/* Fallback to stepping over the breakpoint in-line. */
if (target_is_non_stop_p ())
- stop_all_threads ();
+ stop_all_threads ("displaced stepping falling back on inline stepping");
set_step_over_info (regcache->aspace (),
regcache_read_pc (regcache), 0, tp->global_num);
@@ -4915,7 +4915,7 @@ handle_one (const wait_one_event &event)
/* See infrun.h. */
void
-stop_all_threads (void)
+stop_all_threads (const char *reason)
{
/* We may need multiple passes to discover all threads. */
int pass;
@@ -4923,7 +4923,7 @@ stop_all_threads (void)
gdb_assert (exists_non_stop_target ());
- infrun_debug_printf ("starting");
+ INFRUN_SCOPED_DEBUG_START_END ("reason=%s", reason);
scoped_restore_current_thread restore_thread;
@@ -7963,7 +7963,7 @@ stop_waiting (struct execution_control_state *ecs)
/* If all-stop, but there exists a non-stop target, stop all
threads now that we're presenting the stop to the user. */
if (!non_stop && exists_non_stop_target ())
- stop_all_threads ();
+ stop_all_threads ("presenting stop to user in all-stop");
}
/* Like keep_going, but passes the signal to the inferior, even if the
@@ -8063,7 +8063,7 @@ keep_going_pass_signal (struct execution_control_state *ecs)
we're about to step over, otherwise other threads could miss
it. */
if (step_over_info_valid_p () && target_is_non_stop_p ())
- stop_all_threads ();
+ stop_all_threads ("starting in-line step-over");
/* Stop stepping if inserting breakpoints fails. */
try
diff --git a/gdb/infrun.h b/gdb/infrun.h
index 3e84805accb5..be600fd8f7da 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -40,8 +40,8 @@ extern bool debug_infrun;
/* Print "infrun" start/end debug statements. */
-#define INFRUN_SCOPED_DEBUG_START_END(msg) \
- scoped_debug_start_end (debug_infrun, "infrun", msg)
+#define INFRUN_SCOPED_DEBUG_START_END(fmt, ...) \
+ scoped_debug_start_end (debug_infrun, "infrun", fmt, ##__VA_ARGS__)
/* Print "infrun" enter/exit debug statements. */
@@ -139,8 +139,11 @@ extern void set_last_target_status (process_stratum_target *target, ptid_t ptid,
target_wait(). */
extern void nullify_last_target_wait_ptid ();
-/* Stop all threads. Only returns after everything is halted. */
-extern void stop_all_threads (void);
+/* Stop all threads. Only returns after everything is halted.
+
+ REASON is a string indicating the reason why we stop all threads, used in
+ debug messages. */
+extern void stop_all_threads (const char *reason);
extern void prepare_for_detach (void);
diff --git a/gdb/remote.c b/gdb/remote.c
index b126532af45d..8a5cc1108007 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4641,7 +4641,7 @@ remote_target::process_initial_stop_replies (int from_tty)
gdb_assert (!this->is_async_p ());
SCOPE_EXIT { target_async (0); };
target_async (1);
- stop_all_threads ();
+ stop_all_threads ("remote connect in all-stop");
}
/* If all threads of an inferior were already stopped, we
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 2/9] gdb/linux-nat: remove check based on current_inferior in linux_handle_extended_wait
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
2022-01-18 4:09 ` [PATCH v2 1/9] gdb/infrun: add reason parameter to stop_all_threads Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 3/9] gdb: replace inferior::waiting_for_vfork_done with inferior::thread_waiting_for_vfork_done Simon Marchi
` (8 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches
The check removed by this patch, using current_inferior, looks wrong.
When debugging multiple inferiors with the Linux native target and
linux_handle_extended_wait is called, there's no guarantee about which
is the current inferior. The vfork-done event we receive could be for
any inferior. If the vfork-done event is for a non-current inferior, we
end up wrongfully ignoring it. As a result, the core never processes a
TARGET_WAITKIND_VFORK_DONE event, program_space::breakpoints_not_allowed
is never cleared, and breakpoints are never reinserted. However,
because the Linux native target decided to ignore the event, it resumed
the thread - while breakpoints out. And that's bad.
The proposed fix is to remove this check. Always report vfork-done
events and let infrun's logic decide if it should be ignored. We don't
save much cycles by filtering the event here.
Add a test that sets replicates the situation described above. See
comments in the test for more details.
Change-Id: Ibe33c1716c3602e847be6c2093120696f2286fbf
---
gdb/linux-nat.c | 17 +--
.../gdb.threads/vfork-multi-inferior-sleep.c | 25 ++++
.../gdb.threads/vfork-multi-inferior.c | 55 +++++++++
.../gdb.threads/vfork-multi-inferior.exp | 115 ++++++++++++++++++
4 files changed, 199 insertions(+), 13 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c
create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.c
create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.exp
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b926fb5eba9e..7de4da03a34f 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2096,20 +2096,11 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
if (event == PTRACE_EVENT_VFORK_DONE)
{
- if (current_inferior ()->waiting_for_vfork_done)
- {
- linux_nat_debug_printf
- ("Got expected PTRACE_EVENT_VFORK_DONE from LWP %ld: stopping",
- lp->ptid.lwp ());
-
- ourstatus->set_vfork_done ();
- return 0;
- }
-
linux_nat_debug_printf
- ("Got PTRACE_EVENT_VFORK_DONE from LWP %ld: ignoring", lp->ptid.lwp ());
-
- return 1;
+ ("Got expected PTRACE_EVENT_VFORK_DONE from LWP %ld",
+ lp->ptid.lwp ());
+ ourstatus->set_vfork_done ();
+ return 0;
}
internal_error (__FILE__, __LINE__,
diff --git a/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c b/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c
new file mode 100644
index 000000000000..a26ae07d53b7
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2022 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 <unistd.h>
+
+int
+main (void)
+{
+ sleep (30);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/vfork-multi-inferior.c b/gdb/testsuite/gdb.threads/vfork-multi-inferior.c
new file mode 100644
index 000000000000..1b7b00d3c755
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-multi-inferior.c
@@ -0,0 +1,55 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2022 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 <unistd.h>
+#include <sys/wait.h>
+#include <assert.h>
+
+static void
+should_break_here (void)
+{
+}
+
+int
+main (void)
+{
+ int i;
+
+ for (i = 0; i < NR_LOOPS; i++)
+ {
+ int pid = vfork ();
+
+ if (pid != 0)
+ {
+ /* Parent */
+ int stat;
+ int ret = waitpid (pid, &stat, 0);
+ assert (ret == pid);
+ assert (WIFEXITED (stat));
+ assert (WEXITSTATUS (stat) == 12);
+
+ should_break_here ();
+ }
+ else
+ {
+ /* Child */
+ _exit(12);
+ }
+ }
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/vfork-multi-inferior.exp b/gdb/testsuite/gdb.threads/vfork-multi-inferior.exp
new file mode 100644
index 000000000000..22a1476eaacc
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-multi-inferior.exp
@@ -0,0 +1,115 @@
+# Copyright 2020-2022 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 handling a vfork while another inferior is running. The bug that
+# prompted writing this test case was in the Linux native target. The target
+# assumed that the vfork-done event it received was for the current inferior
+# (an invalid assumption, the current inferior is the one randomly selected by
+# do_target_wait (at the time of writing). This caused the target to drop the
+# vfork-done event, because it was seen as unneeded and to restart the thread
+# as if nothing happened. This however resulted in the thread running with
+# breakpoints not inserted.
+#
+# To catch the bug, this test verifies that we can hit a breakpoint after a
+# vfork call, while a second inferior runs in the background.
+
+if [use_gdb_stub] {
+ unsupported "test uses multiple inferiors"
+ return
+}
+
+standard_testfile .c -sleep.c
+
+set srcfile_sleep $srcfile2
+set binfile_sleep ${binfile}-sleep
+
+# The reproducibility of the bug depends on which inferior randomly selects in
+# do_target_wait when consuming the vfork-done event. Since GDB doesn't call
+# srand(), we are likely to always see the same sequence of inferior selected by
+# do_target_wait, which can hide the bug if you are not "lucky". To work
+# around that, call vfork and hit the breakpoint in a loop, it makes it
+# somewhat likely that the wrong inferior will be selected eventually.
+set nr_loops 20
+
+# Compile the main program that calls vfork and hits a breakpoint.
+set opts [list debug additional_flags=-DNR_LOOPS=$nr_loops]
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable \
+ $opts] != "" } {
+ untested "failed to compile"
+ return -1
+}
+
+# Compile the secondary program, which just sleeps.
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile_sleep}" "${binfile_sleep}" executable \
+ {debug}] != "" } {
+ untested "failed to compile"
+ return -1
+}
+
+# We exercise two methods of getting a second inferior to execute while the
+# first one vforks. METHOD can be:
+#
+# - non-stop: start GDB with non-stop on and run the second inferior in
+# background.
+# - schedule-multiple: set "schedule-multiple on", this will make "continue"
+# resume both inferiors.
+proc do_test {method} {
+ save_vars { ::GDBFLAGS } {
+ if { $method == "non-stop" } {
+ append ::GDBFLAGS " -ex \"set non-stop on\""
+ }
+ clean_restart
+ }
+
+ # Start the second inferior in background.
+ gdb_test "add-inferior" "Added inferior 2.*"
+ gdb_test "inferior 2" "Switching to inferior 2 .*"
+ gdb_file_cmd ${::binfile_sleep}
+ if { $method == "non-stop" } {
+ gdb_test "run &" "Starting program: .*" "run inferior 2"
+ } else {
+ gdb_test "start" "Temporary breakpoint $::decimal, main .*" \
+ "start inferior 2"
+ }
+
+ # Start the first inferior.
+ gdb_test "inferior 1" "Switching to inferior 1 .*"
+ gdb_file_cmd ${::binfile}
+ gdb_test "break should_break_here" "Breakpoint $::decimal at .*"
+ gdb_test "start" "Thread 1.1 .* hit Temporary breakpoint.*" \
+ "start inferior 1"
+
+ # Only enable schedule-multiple this late, because of:
+ # https://sourceware.org/bugzilla/show_bug.cgi?id=28777
+ if { $method == "schedule-multiple" } {
+ gdb_test_no_output "set schedule-multiple on"
+ }
+
+
+ # Continue over vfork and until the breakpoint. The number of loops here
+ # matches the number of loops in the program. So if a breakpoint is missed
+ # at some point, a "continue" will wrongfully continue until the end of the
+ # program, which will fail the test.
+ for {set i 0} {$i < $::nr_loops} {incr i} {
+ with_test_prefix "i=$i" {
+ gdb_test "continue" \
+ "Thread 1.1 .* hit Breakpoint $::decimal, should_break_here.*"
+ }
+ }
+}
+
+foreach_with_prefix method {schedule-multiple non-stop} {
+ do_test $method
+}
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 3/9] gdb: replace inferior::waiting_for_vfork_done with inferior::thread_waiting_for_vfork_done
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
2022-01-18 4:09 ` [PATCH v2 1/9] gdb/infrun: add reason parameter to stop_all_threads Simon Marchi
2022-01-18 4:09 ` [PATCH v2 2/9] gdb/linux-nat: remove check based on current_inferior in linux_handle_extended_wait Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 4/9] gdb/infrun: add inferior parameters to stop_all_threads and restart_threads Simon Marchi
` (7 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches
The inferior::waiting_for_vfork_done flag indicates that some thread in
that inferior is waiting for a vfork-done event. Subsequent patches
will need to know which thread is waiting for that event.
I think there is a latent buglet in that waiting_for_vfork_done is
currently not reset on inferior exec or exit. I could imagine that if a
thread in the parent process calls exec or exit while another thread of
the parent process is waiting for its vfork child to exec or exit, we
could end up with inferior::waiting_for_vfork_done without a thread
actually waiting for a vfork-done event anymore. And since that flag is
checked in resume_1, things could misbehave there.
Since the new field points to a thread_info object, and those are
destroyed on exec or exit, it could be worse now since we could try to
access freed memory, if thread_waiting_for_vfork_done were to point to a
stale thread_info. To avoid this, clear the field in
infrun_inferior_exit and infrun_inferior_execd.
Change-Id: I31b847278613a49ba03fc4915f74d9ceb228fdce
---
gdb/inferior.h | 8 ++++----
gdb/infrun.c | 14 +++++++++-----
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/gdb/inferior.h b/gdb/inferior.h
index ec0fb6e8b16c..7b82703f1470 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -531,10 +531,10 @@ class inferior : public refcounted_object,
exits or execs. */
bool pending_detach = false;
- /* True if this inferior is a vfork parent waiting for a vfork child
- not under our control to be done with the shared memory region,
- either by exiting or execing. */
- bool waiting_for_vfork_done = false;
+ /* If non-nullptr, points to a thread that called vfork and is now waiting
+ for a vfork child not under our control to be done with the shared memory
+ region, either by exiting or execing. */
+ thread_info *thread_waiting_for_vfork_done = nullptr;
/* True if we're in the process of detaching from this inferior. */
bool detaching = false;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 46e14d720cce..40b4bdd73130 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -507,7 +507,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
insert breakpoints, so that we can debug it. A
subsequent child exec or exit is enough to know when does
the child stops using the parent's address space. */
- parent_inf->waiting_for_vfork_done = detach_fork;
+ parent_inf->thread_waiting_for_vfork_done
+ = detach_fork ? inferior_thread () : nullptr;
parent_inf->pspace->breakpoints_not_allowed = detach_fork;
}
}
@@ -639,7 +640,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \
child_inf->pending_detach = 0;
parent_inf->vfork_child = child_inf;
parent_inf->pending_detach = detach_fork;
- parent_inf->waiting_for_vfork_done = 0;
+ parent_inf->thread_waiting_for_vfork_done = nullptr;
}
else if (detach_fork)
{
@@ -1484,6 +1485,7 @@ static void
infrun_inferior_exit (struct inferior *inf)
{
inf->displaced_step_state.reset ();
+ inf->thread_waiting_for_vfork_done = nullptr;
}
static void
@@ -1502,6 +1504,8 @@ infrun_inferior_execd (inferior *inf)
one in progress at the time of the exec, it must have been the exec'ing
thread. */
clear_step_over_info ();
+
+ inf->thread_waiting_for_vfork_done = nullptr;
}
/* If ON, and the architecture supports it, GDB will use displaced
@@ -2254,7 +2258,7 @@ resume_1 (enum gdb_signal sig)
/* Depends on stepped_breakpoint. */
step = currently_stepping (tp);
- if (current_inferior ()->waiting_for_vfork_done)
+ if (current_inferior ()->thread_waiting_for_vfork_done != nullptr)
{
/* Don't try to single-step a vfork parent that is waiting for
the child to get out of the shared memory region (by exec'ing
@@ -2374,7 +2378,7 @@ resume_1 (enum gdb_signal sig)
&& use_displaced_stepping (tp)
&& !step_over_info_valid_p ()
&& sig == GDB_SIGNAL_0
- && !current_inferior ()->waiting_for_vfork_done)
+ && current_inferior ()->thread_waiting_for_vfork_done == nullptr)
{
displaced_step_prepare_status prepare_status
= displaced_step_prepare (tp);
@@ -5618,7 +5622,7 @@ handle_inferior_event (struct execution_control_state *ecs)
context_switch (ecs);
- current_inferior ()->waiting_for_vfork_done = 0;
+ current_inferior ()->thread_waiting_for_vfork_done = nullptr;
current_inferior ()->pspace->breakpoints_not_allowed = 0;
if (handle_stop_requested (ecs))
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 4/9] gdb/infrun: add inferior parameters to stop_all_threads and restart_threads
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (2 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 3/9] gdb: replace inferior::waiting_for_vfork_done with inferior::thread_waiting_for_vfork_done Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 5/9] gdb/infrun: add logging statement to do_target_resume Simon Marchi
` (6 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches
A following patch will want to stop all threads of a given inferior (as
opposed to all threads of all inferiors) while handling a vfork, and
restart them after. To help with this, add inferior parameters to
stop_all_threads and restart_threads. This is done as a separate patch
to make sure this doesn't cause regressions on its own, and to keep the
following patches more concise.
No visible changes are expected here, since all calls sites pass
nullptr, which should keep the existing behavior.
Change-Id: I4d9ba886ce842042075b4e346cfa64bbe2580dbf
---
gdb/infrun.c | 39 +++++++++++++++++++++++++++++++--------
gdb/infrun.h | 7 +++++--
2 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 40b4bdd73130..4d12c72f89fd 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3754,7 +3754,8 @@ struct wait_one_event
};
static bool handle_one (const wait_one_event &event);
-static void restart_threads (struct thread_info *event_thread);
+static void restart_threads (struct thread_info *event_thread,
+ inferior *inf = nullptr);
/* Prepare and stabilize the inferior for detaching it. E.g.,
detaching while a thread is displaced stepping is a recipe for
@@ -4919,7 +4920,7 @@ handle_one (const wait_one_event &event)
/* See infrun.h. */
void
-stop_all_threads (const char *reason)
+stop_all_threads (const char *reason, inferior *inf)
{
/* We may need multiple passes to discover all threads. */
int pass;
@@ -4927,22 +4928,29 @@ stop_all_threads (const char *reason)
gdb_assert (exists_non_stop_target ());
- INFRUN_SCOPED_DEBUG_START_END ("reason=%s", reason);
+ INFRUN_SCOPED_DEBUG_START_END ("reason=%s, inf=%d", reason,
+ inf != nullptr ? inf->num : -1);
scoped_restore_current_thread restore_thread;
- /* Enable thread events of all targets. */
+ /* Enable thread events on relevant targets. */
for (auto *target : all_non_exited_process_targets ())
{
+ if (inf != nullptr && inf->process_target () != target)
+ continue;
+
switch_to_target_no_thread (target);
target_thread_events (true);
}
SCOPE_EXIT
{
- /* Disable thread events of all targets. */
+ /* Disable thread events on relevant targets. */
for (auto *target : all_non_exited_process_targets ())
{
+ if (inf != nullptr && inf->process_target () != target)
+ continue;
+
switch_to_target_no_thread (target);
target_thread_events (false);
}
@@ -4967,6 +4975,9 @@ stop_all_threads (const char *reason)
for (auto *target : all_non_exited_process_targets ())
{
+ if (inf != nullptr && inf->process_target () != target)
+ continue;
+
switch_to_target_no_thread (target);
update_thread_list ();
}
@@ -4975,6 +4986,9 @@ stop_all_threads (const char *reason)
to tell the target to stop. */
for (thread_info *t : all_non_exited_threads ())
{
+ if (inf != nullptr && t->inf != inf)
+ continue;
+
/* For a single-target setting with an all-stop target,
we would not even arrive here. For a multi-target
setting, until GDB is able to handle a mixture of
@@ -5717,17 +5731,26 @@ handle_inferior_event (struct execution_control_state *ecs)
}
/* Restart threads back to what they were trying to do back when we
- paused them for an in-line step-over. The EVENT_THREAD thread is
- ignored. */
+ paused them (because of an in-line step-over or vfork, for example).
+ The EVENT_THREAD thread is ignored (not restarted).
+
+ If INF is non-nullptr, only resume threads from INF. */
static void
-restart_threads (struct thread_info *event_thread)
+restart_threads (struct thread_info *event_thread, inferior *inf)
{
+ INFRUN_SCOPED_DEBUG_START_END ("event_thread=%s, inf=%d",
+ event_thread->ptid.to_string ().c_str (),
+ inf != nullptr ? inf->num : -1);
+
/* In case the instruction just stepped spawned a new thread. */
update_thread_list ();
for (thread_info *tp : all_non_exited_threads ())
{
+ if (inf != nullptr && tp->inf != inf)
+ continue;
+
if (tp->inf->detaching)
{
infrun_debug_printf ("restart threads: [%s] inferior detaching",
diff --git a/gdb/infrun.h b/gdb/infrun.h
index be600fd8f7da..7e135ed420b7 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -142,8 +142,11 @@ extern void nullify_last_target_wait_ptid ();
/* Stop all threads. Only returns after everything is halted.
REASON is a string indicating the reason why we stop all threads, used in
- debug messages. */
-extern void stop_all_threads (const char *reason);
+ debug messages.
+
+ If INF is non-nullptr, stop all threads of that inferior. Otherwise, stop
+ all threads of all inferiors. */
+extern void stop_all_threads (const char *reason, inferior *inf = nullptr);
extern void prepare_for_detach (void);
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 5/9] gdb/infrun: add logging statement to do_target_resume
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (3 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 4/9] gdb/infrun: add inferior parameters to stop_all_threads and restart_threads Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) Simon Marchi
` (5 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches
This helped me, it shows which ptid we actually call target_resume with.
Change-Id: I2dfd771e83df8c25f39371a13e3e91dc7882b73d
---
gdb/infrun.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4d12c72f89fd..c976e3efbfb4 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2189,6 +2189,10 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
else
target_pass_signals (signal_pass);
+ infrun_debug_printf ("resume_ptid=%s, step=%d, sig=%s",
+ resume_ptid.to_string ().c_str (),
+ step, gdb_signal_to_symbol_string (sig));
+
target_resume (resume_ptid, step, sig);
if (target_can_async_p ())
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (4 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 5/9] gdb/infrun: add logging statement to do_target_resume Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-02-16 0:28 ` Lancelot SIX
2022-01-18 4:09 ` [PATCH v2 7/9] gdbserver: report correct status in thread stop race condition Simon Marchi
` (4 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
There is a problem with how GDB handles a vfork happening in a
multi-threaded program. This problem was reported to me by somebody not
using vfork directly, but using system(3) in a multi-threaded program,
which may be implemented using vfork.
This patch only deals about the follow-fork-mode=parent,
detach-on-fork=on case, because it would be too much to chew at once to
fix the bugs in the other cases as well (I tried).
The problem
-----------
When a program vforks, the parent thread is suspended by the kernel
until the child process exits or execs. Specifically, in a
multi-threaded program, only the thread that called vfork is suspended,
other threads keep running freely. This is documented in the vfork(2)
man page ("Caveats" section).
Let's suppose GDB is handling a vfork and the user's desire is to detach
from the child. Before detaching the child, GDB must remove the software
breakpoints inserted in the shared parent/child address space, in case
there's a breakpoint in the path the child is going to take before
exec'ing or exit'ing (unlikely, but possible). Otherwise the child could
hit a breakpoint instruction while running outside the control of GDB,
which would make it crash. GDB must also avoid re-inserting breakpoints
in the parent as long as it didn't receive the "vfork done" event (that
is, when the child has exited or execed): since the address space is
shared with the child, that would re-insert breakpoints in the child
process also. So what GDB does is:
1. Receive "vfork" event for the parent
2. Remove breakpoints from the (shared) address space and set
program_space::breakpoints_not_allowed to avoid re-inserting them
3. Detach from the child thread
4. Resume the parent
5. Wait for and receive "vfork done" event for the parent
6. Clean program_space::breakpoints_not_allowed and re-insert
breakpoints
7. Resume the parent
Resuming the parent at step 4 is necessary in order for the kernel to
report the "vfork done" event. The kernel won't report a ptrace event
for a thread that is ptrace-stopped. But the theory behind this is that
between steps 4 and 5, the parent won't actually do any progress even
though it is ptrace-resumed, because the kernel keeps it suspended,
waiting for the child to exec or exit. So it doesn't matter for that
thread if breakpoints are not inserted.
The problem is when the program is multi-threaded. In step 4, GDB
resumes all threads of the parent. The thread that did the vfork stays
suspended by the kernel, so that's fine. But other threads are running
freely while breakpoints are removed, which is a problem because they
could miss a breakpoint that they should have hit.
The problem is present with all-stop and non-stop targets. The only
difference is that with an all-stop targets, the other threads are
stopped by the target when it reports the vfork event and are resumed by
the target when GDB resumes the parent. With a non-stop target, the
other threads are simply never stopped.
The fix
-------
There many combinations of settings to consider (all-stop/non-stop,
target-non-stop on/off, follow-fork-mode parent/child, detach-on-fork
on/off, schedule-multiple on/off), but for this patch I restrict the
scope to follow-fork-mode=parent, detach-on-fork=on. That's the
"default" case, where we detach the child and keep debugging the
parent. I tried to fix them all, but it's just too much to do at once.
The code paths and behaviors for when we don't detach the child are
completely different.
The guiding principle for this patch is that all threads of the vforking
inferior should be stopped as long as breakpoints are removed. This is
similar to handling in-line step-overs, in a way.
For non-stop targets (the default on Linux native), this is what
happens:
- In follow_fork, we call stop_all_threads to stop all threads of the
inferior
- In follow_fork_inferior, we record the vfork parent thread in
inferior::thread_waiting_for_vfork_done
- Back in handle_inferior_event, we call keep_going, which resumes only
the event thread (this is already the case, with a non-stop target).
This is the thread that will be waiting for vfork-done.
- When we get the vfork-done event, we go in the (new) handle_vfork_done
function to restart the previously stopped threads.
In the same scenario, but with an all-stop target:
- In follow_fork, no need to stop all threads of the inferior, the
target has stopped all threads of all its inferiors before returning
the event.
- In follow_fork_inferior, we record the vfork parent thread in
inferior::thread_waiting_for_vfork_done.
- Back in handle_inferior_event, we also call keep_going. However, we
only want to resume the event thread here, not all inferior threads.
In internal_resume_ptid (called by resume_1), we therefore now check
whether one of the inferiors we are about to resume has
thread_waiting_for_vfork_done set. If so, we only resume that
thread. When resuming multiple inferiors, one vforking and one not
vforking, we could resume the vforking thread from the vforking
inferior plus all threads from the vforking inferior. However, the
target_resume interface today does not allow this.
- When we get the vfork-done event, the existing call to keep_going
naturally resumes all threads.
Testing-wise, add a test that tries to make the main thread hit a
breakpoint while a secondary thread calls vfork. Without the fix, the
main thread keeps going while breakpoints are removed, resulting in a
missed breakpoint and the program exiting.
Change-Id: I20eb78e17ca91f93c19c2b89a7e12c382ee814a1
---
gdb/infrun.c | 134 ++++++++++++++++--
.../gdb.threads/vfork-multi-thread.c | 74 ++++++++++
.../gdb.threads/vfork-multi-thread.exp | 96 +++++++++++++
3 files changed, 296 insertions(+), 8 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.c
create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c976e3efbfb4..75252be22038 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -95,6 +95,11 @@ static void resume (gdb_signal sig);
static void wait_for_inferior (inferior *inf);
+static void restart_threads (struct thread_info *event_thread,
+ inferior *inf = nullptr);
+
+static bool start_step_over (void);
+
/* Asynchronous signal handler registered as event loop source for
when we have pending events ready to be passed to the core. */
static struct async_event_handler *infrun_async_inferior_event_token;
@@ -431,6 +436,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
inferior *parent_inf = current_inferior ();
inferior *child_inf = nullptr;
+ gdb_assert (parent_inf->thread_waiting_for_vfork_done == nullptr);
+
if (!follow_child)
{
/* Detach new forked process? */
@@ -640,7 +647,6 @@ holding the child stopped. Try \"set detach-on-fork\" or \
child_inf->pending_detach = 0;
parent_inf->vfork_child = child_inf;
parent_inf->pending_detach = detach_fork;
- parent_inf->thread_waiting_for_vfork_done = nullptr;
}
else if (detach_fork)
{
@@ -773,6 +779,12 @@ follow_fork ()
parent = inferior_ptid;
child = tp->pending_follow.child_ptid ();
+ /* If handling a vfork, stop all the inferior's threads, they will be
+ restarted when the vfork shared region is complete. */
+ if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
+ && target_is_non_stop_p ())
+ stop_all_threads ("handling vfork", tp->inf);
+
process_stratum_target *parent_targ = tp->inf->process_target ();
/* Set up inferior(s) as specified by the caller, and tell the
target to do whatever is necessary to follow either parent
@@ -1034,6 +1046,53 @@ handle_vfork_child_exec_or_exit (int exec)
}
}
+/* Handle TARGET_WAITKIND_VFORK_DONE. */
+
+static void
+handle_vfork_done (thread_info *event_thread)
+{
+ /* We only care about this event if inferior::thread_waiting_for_vfork_done is
+ set, that is if we are waiting for a vfork child not under our control
+ (because we detached it) to exec or exit.
+
+ If an inferior has vforked and we are debugging the child, we don't use
+ the vfork-done event to get notified about the end of the shared address
+ space window). We rely instead on the child's exec or exit event, and the
+ inferior::vfork_{parent,child} fields are used instead. See
+ handle_vfork_child_exec_or_exit for that. */
+ if (event_thread->inf->thread_waiting_for_vfork_done == nullptr)
+ {
+ infrun_debug_printf ("not waiting for a vfork-done event");
+ return;
+ }
+
+ INFRUN_SCOPED_DEBUG_ENTER_EXIT;
+
+ /* We stopped all threads (other than the vforking thread) of the inferior in
+ follow_fork and kept them stopped until now. It should therefore not be
+ possible for another thread to have reported a vfork during that window.
+ If THREAD_WAITING_FOR_VFORK_DONE is set, it has to be the same thread whose
+ vfork-done we are handling right now. */
+ gdb_assert (event_thread->inf->thread_waiting_for_vfork_done == event_thread);
+
+ event_thread->inf->thread_waiting_for_vfork_done = nullptr;
+ event_thread->inf->pspace->breakpoints_not_allowed = 0;
+
+ /* On non-stop targets, we stopped all the inferior's threads in follow_fork,
+ resume them now. On all-stop targets, everything that needs to be resumed
+ will be when we resume the event thread. */
+ if (target_is_non_stop_p ())
+ {
+ /* restart_threads and start_step_over may change the current thread, make
+ sure we leave the event thread as the current thread. */
+ scoped_restore_current_thread restore_thread;
+
+ insert_breakpoints ();
+ restart_threads (event_thread, event_thread->inf);
+ start_step_over ();
+ }
+}
+
/* Enum strings for "set|show follow-exec-mode". */
static const char follow_exec_mode_new[] = "new";
@@ -1908,6 +1967,16 @@ start_step_over (void)
continue;
}
+ if (tp->inf->thread_waiting_for_vfork_done)
+ {
+ /* When we stop all threads, handling a vfork, any thread in the step
+ over chain remains there. A user could also try to continue a
+ thread stopped at a breakpoint while another thread is waiting for
+ a vfork-done event. In any case, we don't want to start a step
+ over right now. */
+ continue;
+ }
+
/* Remove thread from the THREADS_TO_STEP chain. If anything goes wrong
while we try to prepare the displaced step, we don't add it back to
the global step over chain. This is to avoid a thread staying in the
@@ -2143,8 +2212,41 @@ internal_resume_ptid (int user_step)
return a wildcard ptid. */
if (target_is_non_stop_p ())
return inferior_ptid;
- else
- return user_visible_resume_ptid (user_step);
+
+ /* The rest of the function assumes non-stop==off and
+ target-non-stop==off.
+
+ If a thread is waiting for a vfork-done event, it means breakpoints are out
+ for this inferior (well, program space in fact). We don't want to resume
+ any thread other than the one waiting for vfork done, otherwise these other
+ threads could miss breakpoints. So if a thread in the resumption set is
+ waiting for a vfork-done event, resume only that thread.
+
+ The resumption set width depends on whether schedule-multiple is on or off.
+
+ Note that if the target_resume interface was more flexible, we could be
+ smarter here when schedule-multiple is on. For example, imagine 3
+ inferiors with 2 threads each (1.1, 1.2, 2.1, 2.2, 3.1 and 3.2). Threads
+ 2.1 and 3.2 are both waiting for a vfork-done event. Then we could ask the
+ target(s) to resume:
+
+ - All threads of inferior 1
+ - Thread 2.1
+ - Thread 3.2
+
+ Since we don't have that flexibility (we can only pass one ptid), just
+ resume the first thread waiting for a vfork-done event we find (e.g. thread
+ 2.1). */
+ if (sched_multi)
+ {
+ for (inferior *inf : all_non_exited_inferiors ())
+ if (inf->thread_waiting_for_vfork_done != nullptr)
+ return inf->thread_waiting_for_vfork_done->ptid;
+ }
+ else if (current_inferior ()->thread_waiting_for_vfork_done != nullptr)
+ return current_inferior ()->thread_waiting_for_vfork_done->ptid;
+
+ return user_visible_resume_ptid (user_step);
}
/* Wrapper for target_resume, that handles infrun-specific
@@ -3258,6 +3360,19 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
continue;
}
+ /* If a thread of that inferior is waiting for a vfork-done
+ (for a detached vfork child to exec or exit), breakpoints are
+ removed. We must not resume any thread of that inferior, other
+ than the one waiting for the vfork-done. */
+ if (tp->inf->thread_waiting_for_vfork_done != nullptr
+ && tp != tp->inf->thread_waiting_for_vfork_done)
+ {
+ infrun_debug_printf ("[%s] another thread of this inferior is "
+ "waiting for vfork-done",
+ tp->ptid.to_string ().c_str ());
+ continue;
+ }
+
infrun_debug_printf ("resuming %s",
tp->ptid.to_string ().c_str ());
@@ -3268,7 +3383,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
error (_("Command aborted."));
}
}
- else if (!cur_thr->resumed () && !thread_is_in_step_over_chain (cur_thr))
+ else if (!cur_thr->resumed ()
+ && !thread_is_in_step_over_chain (cur_thr)
+ /* In non-stop, forbid resume a thread if some other thread of
+ that inferior is waiting for a vfork-done event (this means
+ breakpoints are out for this inferior). */
+ && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done))
{
/* The thread wasn't started, and isn't queued, run it now. */
reset_ecs (ecs, cur_thr);
@@ -3758,8 +3878,6 @@ struct wait_one_event
};
static bool handle_one (const wait_one_event &event);
-static void restart_threads (struct thread_info *event_thread,
- inferior *inf = nullptr);
/* Prepare and stabilize the inferior for detaching it. E.g.,
detaching while a thread is displaced stepping is a recipe for
@@ -5640,8 +5758,8 @@ handle_inferior_event (struct execution_control_state *ecs)
context_switch (ecs);
- current_inferior ()->thread_waiting_for_vfork_done = nullptr;
- current_inferior ()->pspace->breakpoints_not_allowed = 0;
+ handle_vfork_done (ecs->event_thread);
+ gdb_assert (inferior_thread () == ecs->event_thread);
if (handle_stop_requested (ecs))
return;
diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.c b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
new file mode 100644
index 000000000000..cd3dfcc1e653
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
@@ -0,0 +1,74 @@
+#include <assert.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+// Start with:
+//
+// ./gdb -nx -q --data-directory=data-directory repro -ex "tb break_here_first" -ex r -ex "b should_break_here"
+//
+// Then do "continue".
+//
+// The main thread will likely cross should_break_here while we are handling
+// the vfork and breakpoints are removed, therefore missing the breakpoint.
+
+static volatile int release_vfork = 0;
+static volatile int release_main = 0;
+
+static void *vforker(void *arg)
+{
+ while (!release_vfork);
+
+ pid_t pid = vfork ();
+ if (pid == 0) {
+ /* A vfork child is not supposed to mess with the state of the program,
+ but it is helpful for the purpose of this test. */
+ release_main = 1;
+ _exit(7);
+ }
+
+ int stat;
+ int ret = waitpid (pid, &stat, 0);
+ assert (ret == pid);
+ assert (WIFEXITED (stat));
+ assert (WEXITSTATUS (stat) == 7);
+
+ return NULL;
+}
+
+static void should_break_here(void) {}
+
+int main()
+{
+
+ pthread_t thread;
+ int ret = pthread_create(&thread, NULL, vforker, NULL);
+ assert(ret == 0);
+
+ /* We break here first, while the thread is stuck on `!release_fork`. */
+ release_vfork = 1;
+
+ /* We set a breakpoint on should_break_here.
+
+ We then set "release_fork" from the debugger and continue. The main
+ thread hangs on `!release_main` while the non-main thread vforks. During
+ the window of time where the two processes have a shared address space
+ (after vfork, before _exit), GDB removes the breakpoints from the address
+ space. During that window, only the vfork-ing thread (the non-main
+ thread) is frozen by the kernel. The main thread is free to execute. The
+ child process sets `release_main`, releasing the main thread. A buggy GDB
+ would let the main thread execute during that window, leading to the
+ breakpoint on should_break_here being missed. A fixed GDB does not resume
+ the threads of the vforking process other than the vforking thread. When
+ the vfork child exits, the fixed GDB resumes the main thread, after
+ breakpoints are reinserted, so the breakpoint is not missed. */
+
+ while (!release_main);
+
+ should_break_here();
+
+ pthread_join (thread, NULL);
+
+ return 6;
+}
diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.exp b/gdb/testsuite/gdb.threads/vfork-multi-thread.exp
new file mode 100644
index 000000000000..d405411be012
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.exp
@@ -0,0 +1,96 @@
+# Copyright 2022 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 that a multi-threaded program doing a vfork doesn't miss breakpoints.
+#
+# When a program vforks, its address space is shared with the parent. When we
+# detach a vfork child, we must keep breakpoints out of that shared address space
+# until the child either exits or execs, so that the child does not hit a
+# breakpoint while out of GDB's control. During that time, threads from
+# the parent must be held stopped, otherwise they could miss breakpoints.
+#
+# The thread that did the vfork is suspended by the kernel, so it's not a
+# concern. The other threads need to be manually stopped by GDB and resumed
+# once the vfork critical region is done.
+#
+# This test spawns one thread that calls vfork. Meanwhile, the main thread
+# crosses a breakpoint. A buggy GDB would let the main thread run while
+# breakpoints are removed, so the main thread would miss the breakpoint and run
+# until exit.
+
+standard_testfile
+
+if { [build_executable "failed to prepare" ${testfile} ${srcfile} {debug pthreads}] } {
+ return
+}
+
+set any "\[^\r\n\]*"
+
+# A bunch of util procedures to continue an inferior to an expected point.
+
+proc continue_to_parent_breakpoint {} {
+ gdb_test "continue" \
+ "hit Breakpoint .* should_break_here .*" \
+ "continue parent to breakpoint"
+}
+
+proc continue_to_parent_end {} {
+ gdb_test "continue" "Inferior 1.*exited with code 06.*" \
+ "continue parent to end"
+}
+
+# Run the test with the given GDB settings.
+
+proc do_test { target-non-stop non-stop follow-fork-mode detach-on-fork schedule-multiple } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
+ append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
+ clean_restart ${::binfile}
+ }
+
+ gdb_test_no_output "set follow-fork-mode ${follow-fork-mode}"
+ gdb_test_no_output "set detach-on-fork ${detach-on-fork}"
+ gdb_test_no_output "set schedule-multiple ${schedule-multiple}"
+
+ # The message about thread 2 of inferior 1 exiting happens at a somewhat
+ # unpredictable moment, it's simpler to silence it than to try to match it.
+ gdb_test_no_output "set print thread-events off"
+
+ if { ![runto_main] } {
+ return
+ }
+
+ # The main thread is expected to hit this breakpoint.
+ gdb_test "break should_break_here" "Breakpoint $::decimal at .*"
+
+ continue_to_parent_breakpoint
+ continue_to_parent_end
+}
+
+# We only test with follow-fork-mode=parent and detach-on-fork=on at the
+# moment, but the loops below are written to make it easy to add other values
+# on these axes in the future.
+
+foreach_with_prefix target-non-stop {auto on off} {
+ foreach_with_prefix non-stop {off on} {
+ foreach_with_prefix follow-fork-mode {parent} {
+ foreach_with_prefix detach-on-fork {on} {
+ foreach_with_prefix schedule-multiple {off on} {
+ do_test ${target-non-stop} ${non-stop} ${follow-fork-mode} ${detach-on-fork} ${schedule-multiple}
+ }
+ }
+ }
+ }
+}
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
2022-01-18 4:09 ` [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) Simon Marchi
@ 2022-02-16 0:28 ` Lancelot SIX
2022-02-16 13:35 ` Simon Marchi
0 siblings, 1 reply; 16+ messages in thread
From: Lancelot SIX @ 2022-02-16 0:28 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches, Simon Marchi
Hi,
I have a question (in the test), and few styling related remarks I have
added below in the text.
On Mon, Jan 17, 2022 at 11:09:34PM -0500, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> There is a problem with how GDB handles a vfork happening in a
> multi-threaded program. This problem was reported to me by somebody not
> using vfork directly, but using system(3) in a multi-threaded program,
> which may be implemented using vfork.
>
> This patch only deals about the follow-fork-mode=parent,
> detach-on-fork=on case, because it would be too much to chew at once to
> fix the bugs in the other cases as well (I tried).
>
> The problem
> -----------
>
> When a program vforks, the parent thread is suspended by the kernel
> until the child process exits or execs. Specifically, in a
> multi-threaded program, only the thread that called vfork is suspended,
> other threads keep running freely. This is documented in the vfork(2)
> man page ("Caveats" section).
>
> Let's suppose GDB is handling a vfork and the user's desire is to detach
> from the child. Before detaching the child, GDB must remove the software
> breakpoints inserted in the shared parent/child address space, in case
> there's a breakpoint in the path the child is going to take before
> exec'ing or exit'ing (unlikely, but possible). Otherwise the child could
> hit a breakpoint instruction while running outside the control of GDB,
> which would make it crash. GDB must also avoid re-inserting breakpoints
> in the parent as long as it didn't receive the "vfork done" event (that
> is, when the child has exited or execed): since the address space is
> shared with the child, that would re-insert breakpoints in the child
> process also. So what GDB does is:
>
> 1. Receive "vfork" event for the parent
> 2. Remove breakpoints from the (shared) address space and set
> program_space::breakpoints_not_allowed to avoid re-inserting them
> 3. Detach from the child thread
> 4. Resume the parent
> 5. Wait for and receive "vfork done" event for the parent
> 6. Clean program_space::breakpoints_not_allowed and re-insert
> breakpoints
> 7. Resume the parent
>
> Resuming the parent at step 4 is necessary in order for the kernel to
> report the "vfork done" event. The kernel won't report a ptrace event
> for a thread that is ptrace-stopped. But the theory behind this is that
> between steps 4 and 5, the parent won't actually do any progress even
> though it is ptrace-resumed, because the kernel keeps it suspended,
> waiting for the child to exec or exit. So it doesn't matter for that
> thread if breakpoints are not inserted.
>
> The problem is when the program is multi-threaded. In step 4, GDB
> resumes all threads of the parent. The thread that did the vfork stays
> suspended by the kernel, so that's fine. But other threads are running
> freely while breakpoints are removed, which is a problem because they
> could miss a breakpoint that they should have hit.
>
> The problem is present with all-stop and non-stop targets. The only
> difference is that with an all-stop targets, the other threads are
> stopped by the target when it reports the vfork event and are resumed by
> the target when GDB resumes the parent. With a non-stop target, the
> other threads are simply never stopped.
>
> The fix
> -------
>
> There many combinations of settings to consider (all-stop/non-stop,
> target-non-stop on/off, follow-fork-mode parent/child, detach-on-fork
> on/off, schedule-multiple on/off), but for this patch I restrict the
> scope to follow-fork-mode=parent, detach-on-fork=on. That's the
> "default" case, where we detach the child and keep debugging the
> parent. I tried to fix them all, but it's just too much to do at once.
> The code paths and behaviors for when we don't detach the child are
> completely different.
>
> The guiding principle for this patch is that all threads of the vforking
> inferior should be stopped as long as breakpoints are removed. This is
> similar to handling in-line step-overs, in a way.
>
> For non-stop targets (the default on Linux native), this is what
> happens:
>
> - In follow_fork, we call stop_all_threads to stop all threads of the
> inferior
> - In follow_fork_inferior, we record the vfork parent thread in
> inferior::thread_waiting_for_vfork_done
> - Back in handle_inferior_event, we call keep_going, which resumes only
> the event thread (this is already the case, with a non-stop target).
> This is the thread that will be waiting for vfork-done.
> - When we get the vfork-done event, we go in the (new) handle_vfork_done
> function to restart the previously stopped threads.
>
> In the same scenario, but with an all-stop target:
>
> - In follow_fork, no need to stop all threads of the inferior, the
> target has stopped all threads of all its inferiors before returning
> the event.
> - In follow_fork_inferior, we record the vfork parent thread in
> inferior::thread_waiting_for_vfork_done.
> - Back in handle_inferior_event, we also call keep_going. However, we
> only want to resume the event thread here, not all inferior threads.
> In internal_resume_ptid (called by resume_1), we therefore now check
> whether one of the inferiors we are about to resume has
> thread_waiting_for_vfork_done set. If so, we only resume that
> thread. When resuming multiple inferiors, one vforking and one not
> vforking, we could resume the vforking thread from the vforking
> inferior plus all threads from the vforking inferior. However, the
^
e
Did you forget a "non-" here?
> target_resume interface today does not allow this.
> - When we get the vfork-done event, the existing call to keep_going
> naturally resumes all threads.
>
> Testing-wise, add a test that tries to make the main thread hit a
> breakpoint while a secondary thread calls vfork. Without the fix, the
> main thread keeps going while breakpoints are removed, resulting in a
> missed breakpoint and the program exiting.
>
> Change-Id: I20eb78e17ca91f93c19c2b89a7e12c382ee814a1
> ---
> gdb/infrun.c | 134 ++++++++++++++++--
> .../gdb.threads/vfork-multi-thread.c | 74 ++++++++++
> .../gdb.threads/vfork-multi-thread.exp | 96 +++++++++++++
> 3 files changed, 296 insertions(+), 8 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.exp
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c976e3efbfb4..75252be22038 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -95,6 +95,11 @@ static void resume (gdb_signal sig);
>
> static void wait_for_inferior (inferior *inf);
>
> +static void restart_threads (struct thread_info *event_thread,
> + inferior *inf = nullptr);
> +
> +static bool start_step_over (void);
> +
> /* Asynchronous signal handler registered as event loop source for
> when we have pending events ready to be passed to the core. */
> static struct async_event_handler *infrun_async_inferior_event_token;
> @@ -431,6 +436,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \
> inferior *parent_inf = current_inferior ();
> inferior *child_inf = nullptr;
>
> + gdb_assert (parent_inf->thread_waiting_for_vfork_done == nullptr);
> +
> if (!follow_child)
> {
> /* Detach new forked process? */
> @@ -640,7 +647,6 @@ holding the child stopped. Try \"set detach-on-fork\" or \
> child_inf->pending_detach = 0;
> parent_inf->vfork_child = child_inf;
> parent_inf->pending_detach = detach_fork;
> - parent_inf->thread_waiting_for_vfork_done = nullptr;
> }
> else if (detach_fork)
> {
> @@ -773,6 +779,12 @@ follow_fork ()
> parent = inferior_ptid;
> child = tp->pending_follow.child_ptid ();
>
> + /* If handling a vfork, stop all the inferior's threads, they will be
> + restarted when the vfork shared region is complete. */
> + if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
> + && target_is_non_stop_p ())
> + stop_all_threads ("handling vfork", tp->inf);
> +
> process_stratum_target *parent_targ = tp->inf->process_target ();
> /* Set up inferior(s) as specified by the caller, and tell the
> target to do whatever is necessary to follow either parent
> @@ -1034,6 +1046,53 @@ handle_vfork_child_exec_or_exit (int exec)
> }
> }
>
> +/* Handle TARGET_WAITKIND_VFORK_DONE. */
> +
> +static void
> +handle_vfork_done (thread_info *event_thread)
> +{
> + /* We only care about this event if inferior::thread_waiting_for_vfork_done is
> + set, that is if we are waiting for a vfork child not under our control
> + (because we detached it) to exec or exit.
> +
> + If an inferior has vforked and we are debugging the child, we don't use
> + the vfork-done event to get notified about the end of the shared address
> + space window). We rely instead on the child's exec or exit event, and the
^
This closing parens has no corresponding opening one.
> + inferior::vfork_{parent,child} fields are used instead. See
> + handle_vfork_child_exec_or_exit for that. */
> + if (event_thread->inf->thread_waiting_for_vfork_done == nullptr)
> + {
> + infrun_debug_printf ("not waiting for a vfork-done event");
> + return;
> + }
> +
> + INFRUN_SCOPED_DEBUG_ENTER_EXIT;
> +
> + /* We stopped all threads (other than the vforking thread) of the inferior in
> + follow_fork and kept them stopped until now. It should therefore not be
> + possible for another thread to have reported a vfork during that window.
> + If THREAD_WAITING_FOR_VFORK_DONE is set, it has to be the same thread whose
> + vfork-done we are handling right now. */
> + gdb_assert (event_thread->inf->thread_waiting_for_vfork_done == event_thread);
> +
> + event_thread->inf->thread_waiting_for_vfork_done = nullptr;
> + event_thread->inf->pspace->breakpoints_not_allowed = 0;
> +
> + /* On non-stop targets, we stopped all the inferior's threads in follow_fork,
> + resume them now. On all-stop targets, everything that needs to be resumed
> + will be when we resume the event thread. */
> + if (target_is_non_stop_p ())
> + {
> + /* restart_threads and start_step_over may change the current thread, make
> + sure we leave the event thread as the current thread. */
> + scoped_restore_current_thread restore_thread;
> +
> + insert_breakpoints ();
> + restart_threads (event_thread, event_thread->inf);
> + start_step_over ();
> + }
> +}
> +
> /* Enum strings for "set|show follow-exec-mode". */
>
> static const char follow_exec_mode_new[] = "new";
> @@ -1908,6 +1967,16 @@ start_step_over (void)
> continue;
> }
>
> + if (tp->inf->thread_waiting_for_vfork_done)
Should be tp->inf->thread_waiting_for_vfork_done != nullptr
> + {
> + /* When we stop all threads, handling a vfork, any thread in the step
> + over chain remains there. A user could also try to continue a
> + thread stopped at a breakpoint while another thread is waiting for
> + a vfork-done event. In any case, we don't want to start a step
> + over right now. */
> + continue;
> + }
> +
> /* Remove thread from the THREADS_TO_STEP chain. If anything goes wrong
> while we try to prepare the displaced step, we don't add it back to
> the global step over chain. This is to avoid a thread staying in the
> @@ -2143,8 +2212,41 @@ internal_resume_ptid (int user_step)
> return a wildcard ptid. */
> if (target_is_non_stop_p ())
> return inferior_ptid;
> - else
> - return user_visible_resume_ptid (user_step);
> +
> + /* The rest of the function assumes non-stop==off and
> + target-non-stop==off.
> +
> + If a thread is waiting for a vfork-done event, it means breakpoints are out
> + for this inferior (well, program space in fact). We don't want to resume
> + any thread other than the one waiting for vfork done, otherwise these other
> + threads could miss breakpoints. So if a thread in the resumption set is
> + waiting for a vfork-done event, resume only that thread.
> +
> + The resumption set width depends on whether schedule-multiple is on or off.
> +
> + Note that if the target_resume interface was more flexible, we could be
> + smarter here when schedule-multiple is on. For example, imagine 3
> + inferiors with 2 threads each (1.1, 1.2, 2.1, 2.2, 3.1 and 3.2). Threads
> + 2.1 and 3.2 are both waiting for a vfork-done event. Then we could ask the
> + target(s) to resume:
> +
> + - All threads of inferior 1
> + - Thread 2.1
> + - Thread 3.2
> +
> + Since we don't have that flexibility (we can only pass one ptid), just
> + resume the first thread waiting for a vfork-done event we find (e.g. thread
> + 2.1). */
> + if (sched_multi)
> + {
> + for (inferior *inf : all_non_exited_inferiors ())
> + if (inf->thread_waiting_for_vfork_done != nullptr)
> + return inf->thread_waiting_for_vfork_done->ptid;
> + }
> + else if (current_inferior ()->thread_waiting_for_vfork_done != nullptr)
> + return current_inferior ()->thread_waiting_for_vfork_done->ptid;
> +
> + return user_visible_resume_ptid (user_step);
> }
>
> /* Wrapper for target_resume, that handles infrun-specific
> @@ -3258,6 +3360,19 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
> continue;
> }
>
> + /* If a thread of that inferior is waiting for a vfork-done
> + (for a detached vfork child to exec or exit), breakpoints are
> + removed. We must not resume any thread of that inferior, other
> + than the one waiting for the vfork-done. */
> + if (tp->inf->thread_waiting_for_vfork_done != nullptr
> + && tp != tp->inf->thread_waiting_for_vfork_done)
> + {
> + infrun_debug_printf ("[%s] another thread of this inferior is "
> + "waiting for vfork-done",
> + tp->ptid.to_string ().c_str ());
> + continue;
> + }
> +
> infrun_debug_printf ("resuming %s",
> tp->ptid.to_string ().c_str ());
>
> @@ -3268,7 +3383,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
> error (_("Command aborted."));
> }
> }
> - else if (!cur_thr->resumed () && !thread_is_in_step_over_chain (cur_thr))
> + else if (!cur_thr->resumed ()
> + && !thread_is_in_step_over_chain (cur_thr)
> + /* In non-stop, forbid resume a thread if some other thread of
> + that inferior is waiting for a vfork-done event (this means
> + breakpoints are out for this inferior). */
> + && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done))
Should be cur_thr->inf->thread_waiting_for_vfork_done != nullptr
> {
> /* The thread wasn't started, and isn't queued, run it now. */
> reset_ecs (ecs, cur_thr);
> @@ -3758,8 +3878,6 @@ struct wait_one_event
> };
>
> static bool handle_one (const wait_one_event &event);
> -static void restart_threads (struct thread_info *event_thread,
> - inferior *inf = nullptr);
>
> /* Prepare and stabilize the inferior for detaching it. E.g.,
> detaching while a thread is displaced stepping is a recipe for
> @@ -5640,8 +5758,8 @@ handle_inferior_event (struct execution_control_state *ecs)
>
> context_switch (ecs);
>
> - current_inferior ()->thread_waiting_for_vfork_done = nullptr;
> - current_inferior ()->pspace->breakpoints_not_allowed = 0;
> + handle_vfork_done (ecs->event_thread);
> + gdb_assert (inferior_thread () == ecs->event_thread);
>
> if (handle_stop_requested (ecs))
> return;
> diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.c b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
> new file mode 100644
> index 000000000000..cd3dfcc1e653
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
> @@ -0,0 +1,74 @@
> +#include <assert.h>
> +#include <pthread.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/wait.h>
> +
> +// Start with:
> +//
> +// ./gdb -nx -q --data-directory=data-directory repro -ex "tb break_here_first" -ex r -ex "b should_break_here"
> +//
> +// Then do "continue".
> +//
> +// The main thread will likely cross should_break_here while we are handling
> +// the vfork and breakpoints are removed, therefore missing the breakpoint.
> +
> +static volatile int release_vfork = 0;
> +static volatile int release_main = 0;
> +
> +static void *vforker(void *arg)
shouldn’t vforker shoult be at column 0, even in test code?
> +{
> + while (!release_vfork);
> +
> + pid_t pid = vfork ();
> + if (pid == 0) {
> + /* A vfork child is not supposed to mess with the state of the program,
> + but it is helpful for the purpose of this test. */
> + release_main = 1;
> + _exit(7);
> + }
> +
> + int stat;
> + int ret = waitpid (pid, &stat, 0);
> + assert (ret == pid);
> + assert (WIFEXITED (stat));
> + assert (WEXITSTATUS (stat) == 7);
> +
> + return NULL;
> +}
> +
> +static void should_break_here(void) {}
> +
> +int main()
Same, main at column 0.
> +{
> +
> + pthread_t thread;
> + int ret = pthread_create(&thread, NULL, vforker, NULL);
> + assert(ret == 0);
space before paren.
> +
> + /* We break here first, while the thread is stuck on `!release_fork`. */
> + release_vfork = 1;
> +
> + /* We set a breakpoint on should_break_here.
> +
> + We then set "release_fork" from the debugger and continue. The main
> + thread hangs on `!release_main` while the non-main thread vforks. During
> + the window of time where the two processes have a shared address space
> + (after vfork, before _exit), GDB removes the breakpoints from the address
> + space. During that window, only the vfork-ing thread (the non-main
> + thread) is frozen by the kernel. The main thread is free to execute. The
> + child process sets `release_main`, releasing the main thread. A buggy GDB
> + would let the main thread execute during that window, leading to the
> + breakpoint on should_break_here being missed. A fixed GDB does not resume
> + the threads of the vforking process other than the vforking thread. When
> + the vfork child exits, the fixed GDB resumes the main thread, after
> + breakpoints are reinserted, so the breakpoint is not missed. */
> +
> + while (!release_main);
> +
> + should_break_here();
Space before paren.
> +
> + pthread_join (thread, NULL);
> +
> + return 6;
> +}
> diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.exp b/gdb/testsuite/gdb.threads/vfork-multi-thread.exp
> new file mode 100644
> index 000000000000..d405411be012
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.exp
> @@ -0,0 +1,96 @@
> +# Copyright 2022 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 that a multi-threaded program doing a vfork doesn't miss breakpoints.
> +#
> +# When a program vforks, its address space is shared with the parent. When we
> +# detach a vfork child, we must keep breakpoints out of that shared address space
> +# until the child either exits or execs, so that the child does not hit a
> +# breakpoint while out of GDB's control. During that time, threads from
> +# the parent must be held stopped, otherwise they could miss breakpoints.
> +#
> +# The thread that did the vfork is suspended by the kernel, so it's not a
> +# concern. The other threads need to be manually stopped by GDB and resumed
> +# once the vfork critical region is done.
> +#
> +# This test spawns one thread that calls vfork. Meanwhile, the main thread
> +# crosses a breakpoint. A buggy GDB would let the main thread run while
> +# breakpoints are removed, so the main thread would miss the breakpoint and run
> +# until exit.
b> +
> +standard_testfile
> +
> +if { [build_executable "failed to prepare" ${testfile} ${srcfile} {debug pthreads}] } {
> + return
> +}
> +
> +set any "\[^\r\n\]*"
> +
> +# A bunch of util procedures to continue an inferior to an expected point.
> +
> +proc continue_to_parent_breakpoint {} {
> + gdb_test "continue" \
> + "hit Breakpoint .* should_break_here .*" \
> + "continue parent to breakpoint"
> +}
> +
> +proc continue_to_parent_end {} {
> + gdb_test "continue" "Inferior 1.*exited with code 06.*" \
> + "continue parent to end"
> +}
> +
> +# Run the test with the given GDB settings.
> +
> +proc do_test { target-non-stop non-stop follow-fork-mode detach-on-fork schedule-multiple } {
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
> + append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
> + clean_restart ${::binfile}
> + }
> +
> + gdb_test_no_output "set follow-fork-mode ${follow-fork-mode}"
> + gdb_test_no_output "set detach-on-fork ${detach-on-fork}"
> + gdb_test_no_output "set schedule-multiple ${schedule-multiple}"
OOC, why do you use GDBFLAGS to set 2 settings and gdb_test_no_output
for 3 settings?
> +
> + # The message about thread 2 of inferior 1 exiting happens at a somewhat
> + # unpredictable moment, it's simpler to silence it than to try to match it.
> + gdb_test_no_output "set print thread-events off"
> +
> + if { ![runto_main] } {
> + return
> + }
> +
> + # The main thread is expected to hit this breakpoint.
> + gdb_test "break should_break_here" "Breakpoint $::decimal at .*"
> +
> + continue_to_parent_breakpoint
> + continue_to_parent_end
> +}
> +
> +# We only test with follow-fork-mode=parent and detach-on-fork=on at the
> +# moment, but the loops below are written to make it easy to add other values
> +# on these axes in the future.
> +
> +foreach_with_prefix target-non-stop {auto on off} {
> + foreach_with_prefix non-stop {off on} {
> + foreach_with_prefix follow-fork-mode {parent} {
> + foreach_with_prefix detach-on-fork {on} {
> + foreach_with_prefix schedule-multiple {off on} {
> + do_test ${target-non-stop} ${non-stop} ${follow-fork-mode} ${detach-on-fork} ${schedule-multiple}
> + }
> + }
> + }
> + }
> +}
> --
> 2.34.1
>
Best,
Lancelot.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on)
2022-02-16 0:28 ` Lancelot SIX
@ 2022-02-16 13:35 ` Simon Marchi
0 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-02-16 13:35 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches, Simon Marchi
>> - In follow_fork, no need to stop all threads of the inferior, the
>> target has stopped all threads of all its inferiors before returning
>> the event.
>> - In follow_fork_inferior, we record the vfork parent thread in
>> inferior::thread_waiting_for_vfork_done.
>> - Back in handle_inferior_event, we also call keep_going. However, we
>> only want to resume the event thread here, not all inferior threads.
>> In internal_resume_ptid (called by resume_1), we therefore now check
>> whether one of the inferiors we are about to resume has
>> thread_waiting_for_vfork_done set. If so, we only resume that
>> thread. When resuming multiple inferiors, one vforking and one not
>> vforking, we could resume the vforking thread from the vforking
>> inferior plus all threads from the vforking inferior. However, the
> ^
> e
> Did you forget a "non-" here?
Yes thanks. The last vforking -> non-vforking.
>> @@ -1034,6 +1046,53 @@ handle_vfork_child_exec_or_exit (int exec)
>> }
>> }
>>
>> +/* Handle TARGET_WAITKIND_VFORK_DONE. */
>> +
>> +static void
>> +handle_vfork_done (thread_info *event_thread)
>> +{
>> + /* We only care about this event if inferior::thread_waiting_for_vfork_done is
>> + set, that is if we are waiting for a vfork child not under our control
>> + (because we detached it) to exec or exit.
>> +
>> + If an inferior has vforked and we are debugging the child, we don't use
>> + the vfork-done event to get notified about the end of the shared address
>> + space window). We rely instead on the child's exec or exit event, and the
> ^
>
> This closing parens has no corresponding opening one.
Removed.
>> @@ -1908,6 +1967,16 @@ start_step_over (void)
>> continue;
>> }
>>
>> + if (tp->inf->thread_waiting_for_vfork_done)
>
> Should be tp->inf->thread_waiting_for_vfork_done != nullptr
Fixed.
>> @@ -3268,7 +3383,12 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal)
>> error (_("Command aborted."));
>> }
>> }
>> - else if (!cur_thr->resumed () && !thread_is_in_step_over_chain (cur_thr))
>> + else if (!cur_thr->resumed ()
>> + && !thread_is_in_step_over_chain (cur_thr)
>> + /* In non-stop, forbid resume a thread if some other thread of
I fixed resume -> resuming.
>> + that inferior is waiting for a vfork-done event (this means
>> + breakpoints are out for this inferior). */
>> + && !(non_stop && cur_thr->inf->thread_waiting_for_vfork_done))
>
> Should be cur_thr->inf->thread_waiting_for_vfork_done != nullptr
Fixed.
>> diff --git a/gdb/testsuite/gdb.threads/vfork-multi-thread.c b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
>> new file mode 100644
>> index 000000000000..cd3dfcc1e653
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.threads/vfork-multi-thread.c
>> @@ -0,0 +1,74 @@
>> +#include <assert.h>
>> +#include <pthread.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <sys/wait.h>
>> +
>> +// Start with:
>> +//
>> +// ./gdb -nx -q --data-directory=data-directory repro -ex "tb break_here_first" -ex r -ex "b should_break_here"
>> +//
>> +// Then do "continue".
>> +//
>> +// The main thread will likely cross should_break_here while we are handling
>> +// the vfork and breakpoints are removed, therefore missing the breakpoint.
>> +
>> +static volatile int release_vfork = 0;
>> +static volatile int release_main = 0;
>> +
>> +static void *vforker(void *arg)
>
> shouldn’t vforker shoult be at column 0, even in test code?
Looks like I completely forgot to clean up this file to make it clean
for upstream. It's missing a copyright header and the formatting is
off. I'll fix all I can find, including what you pointed out below.
>> +# Run the test with the given GDB settings.
>> +
>> +proc do_test { target-non-stop non-stop follow-fork-mode detach-on-fork schedule-multiple } {
>> + save_vars { ::GDBFLAGS } {
>> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
>> + append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
>> + clean_restart ${::binfile}
>> + }
>> +
>> + gdb_test_no_output "set follow-fork-mode ${follow-fork-mode}"
>> + gdb_test_no_output "set detach-on-fork ${detach-on-fork}"
>> + gdb_test_no_output "set schedule-multiple ${schedule-multiple}"
>
> OOC, why do you use GDBFLAGS to set 2 settings and gdb_test_no_output
> for 3 settings?
Because these settings have an effect during the clean_restart, when
using some board files. The native-extended-gdbserver board, in
particular. This board defines "starting GDB" as:
- starting GDB
- starting GDBserver with --multi
- connecting GDB to GDBserver with extended-remote
After clean_restart, the state is that GDB is connected to GDBserver but
not debugging anything, ready to attach or run a program. And the
non-stop and maint target-non-stop settings must be in effect before the
remote connection is set up (for example GDB sends the QNonStop packet
during the initial connection), it would be too late to set them after
the clean_restart.
Thanks for the comments. I saved the changes locally. Since it's all
formatting and not functional changes, I won't send a new version right
away, as I'm sure there will be more comments about the functionality
eventually.
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 7/9] gdbserver: report correct status in thread stop race condition
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (5 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 6/9] gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-01-18 4:09 ` [PATCH v2 8/9] gdb/remote: remove_new_fork_children don't access target_waitstatus::child_ptid if kind == TARGET_WAITKIND_THREAD_EXITED Simon Marchi
` (3 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
The test introduced by the following patch would sometimes fail in this
configuration:
FAIL: gdb.threads/next-fork-other-thread.exp: fork_func=vfork: target-non-stop=on: non-stop=off: displaced-stepping=auto: i=14: next to for loop
The test has multiple threads constantly forking or vforking while the
main thread keep doing "next"s.
(After writing the commit message, I realized this also fixes a similar
failure in gdb.threads/forking-threads-plus-breakpoint.exp with the
native-gdbserver and native-extended-gdbserver boards.)
As stop_all_threads is called, because the main thread finished its
"next", it inevitably happens at some point that we ask the remote
target to stop a thread and wait() reports that this thread stopped with
a fork or vfork event, instead of the SIGSTOP we sent to try to stop it.
While running this test, I attached to GDBserver and stopped at
linux-low.cc:3626. We can see that the status pulled from the kernel
for 2742805 is indeed a vfork event:
(gdb) p/x w
$3 = 0x2057f
(gdb) p WIFSTOPPED(w)
$4 = true
(gdb) p WSTOPSIG(w)
$5 = 5
(gdb) p/x (w >> 8) & (PTRACE_EVENT_VFORK << 8)
$6 = 0x200
However, the statement at line 3626 overrides that:
ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
OURSTATUS becomes "stopped by a SIGTRAP". The information about the
fork or vfork is lost.
It's then all downhill from there, stop_all_threads eventually asks for
a thread list update. That thread list includes the child of that
forgotten fork or vfork, the remote target goes "oh cool, a new process,
let's attach to it!", when in fact that vfork child's destiny was to be
detached.
My reverse-engineered understanding of the code around there is that the
if/else between lines 3562 and 3583 (in the original code) makes sure
OURSTATUS is always initialized (not "ignore"). Either the details are
already in event_child->waitstatus (in the case of fork/vfork, for
example), in which case we just copy event_child->waitstatus to
ourstatus. Or, if the event is a plain "stopped by a signal" or a
syscall event, OURSTATUS is set to "stopped", but without a signal
number. Lines 3601 to 3629 (in the original code) serve to fill in that
last bit of information.
The problem is that when `w` holds the vfork status, the code wrongfully
takes this branch, because WSTOPSIG(w) returns SIGTRAP:
else if (current_thread->last_resume_kind == resume_stop
&& WSTOPSIG (w) != SIGSTOP)
The intent of this branch is, for example, when we sent SIGSTOP to try
to stop a thread, but wait() reports that it stopped with another signal
(that it must have received from somewhere else simultaneously), say
SIGWINCH. In that case, we want to report the SIGWINCH. But in our
fork/vfork case, we don't want to take this branch, as the thread didn't
really stop because it received a signal. For the non "stopped by a
signal" and non "syscall signal" cases, we would ideally skip over all
that snippet that fills in the signal or syscall number.
The fix I propose is to move this snipppet of the else branch of the
if/else above. In addition to moving the code, the last two "else if"
branches:
else if (current_thread->last_resume_kind == resume_stop
&& WSTOPSIG (w) != SIGSTOP)
{
/* A thread that has been requested to stop by GDB with vCont;t,
but, it stopped for other reasons. */
ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
}
else if (ourstatus->kind () == TARGET_WAITKIND_STOPPED)
ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
are changed into a single else:
else
ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
This is the default path we take if:
- W is not a syscall status
- W does not represent a SIGSTOP that have sent to stop the thread and
therefore want to suppress it
Change-Id: If2dc1f0537a549c293f7fa3c53efd00e3e194e79
---
gdbserver/linux-low.cc | 60 ++++++++++++++++++++----------------------
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d7c7336ff1f5..b86dff16f079 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -3559,6 +3559,9 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
unstop_all_lwps (1, event_child);
}
+ /* At this point, we haven't set OURSTATUS. This is where we do it. */
+ gdb_assert (ourstatus->kind () == TARGET_WAITKIND_IGNORE);
+
if (event_child->waitstatus.kind () != TARGET_WAITKIND_IGNORE)
{
/* If the reported event is an exit, fork, vfork or exec, let
@@ -3578,8 +3581,31 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
}
else
{
- /* The actual stop signal is overwritten below. */
- ourstatus->set_stopped (GDB_SIGNAL_0);
+ /* The LWP stopped due to a plain signal or a syscall signal. Either way,
+ event_chid->waitstatus wasn't filled in with the details, so look at
+ the wait status W. */
+ if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
+ {
+ int syscall_number;
+
+ get_syscall_trapinfo (event_child, &syscall_number);
+ if (event_child->syscall_state == TARGET_WAITKIND_SYSCALL_ENTRY)
+ ourstatus->set_syscall_entry (syscall_number);
+ else if (event_child->syscall_state == TARGET_WAITKIND_SYSCALL_RETURN)
+ ourstatus->set_syscall_return (syscall_number);
+ else
+ gdb_assert_not_reached ("unexpected syscall state");
+ }
+ else if (current_thread->last_resume_kind == resume_stop
+ && WSTOPSIG (w) == SIGSTOP)
+ {
+ /* A thread that has been requested to stop by GDB with vCont;t,
+ and it stopped cleanly, so report as SIG0. The use of
+ SIGSTOP is an implementation detail. */
+ ourstatus->set_stopped (GDB_SIGNAL_0);
+ }
+ else
+ ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
}
/* Now that we've selected our final event LWP, un-adjust its PC if
@@ -3598,36 +3624,6 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
}
}
- if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
- {
- int syscall_number;
-
- get_syscall_trapinfo (event_child, &syscall_number);
- if (event_child->syscall_state == TARGET_WAITKIND_SYSCALL_ENTRY)
- ourstatus->set_syscall_entry (syscall_number);
- else if (event_child->syscall_state == TARGET_WAITKIND_SYSCALL_RETURN)
- ourstatus->set_syscall_return (syscall_number);
- else
- gdb_assert_not_reached ("unexpected syscall state");
- }
- else if (current_thread->last_resume_kind == resume_stop
- && WSTOPSIG (w) == SIGSTOP)
- {
- /* A thread that has been requested to stop by GDB with vCont;t,
- and it stopped cleanly, so report as SIG0. The use of
- SIGSTOP is an implementation detail. */
- ourstatus->set_stopped (GDB_SIGNAL_0);
- }
- else if (current_thread->last_resume_kind == resume_stop
- && WSTOPSIG (w) != SIGSTOP)
- {
- /* A thread that has been requested to stop by GDB with vCont;t,
- but, it stopped for other reasons. */
- ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
- }
- else if (ourstatus->kind () == TARGET_WAITKIND_STOPPED)
- ourstatus->set_stopped (gdb_signal_from_host (WSTOPSIG (w)));
-
gdb_assert (step_over_bkpt == null_ptid);
if (debug_threads)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v2 8/9] gdb/remote: remove_new_fork_children don't access target_waitstatus::child_ptid if kind == TARGET_WAITKIND_THREAD_EXITED
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (6 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 7/9] gdbserver: report correct status in thread stop race condition Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-03-31 19:25 ` Pedro Alves
2022-01-18 4:09 ` [PATCH v2 9/9] gdb: resume ongoing step after handling fork or vfork Simon Marchi
` (2 subsequent siblings)
10 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches
Following the previous patch, running
gdb.threads/forking-threads-plus-breakpoints.exp continuously eventually
gives me an internal error.
gdb/target/waitstatus.h:372: internal-error: child_ptid: Assertion `m_kind == TARGET_WAITKIND_FORKED || m_kind == TARGET_WAITKIND_VFORKED' failed.^M
FAIL: gdb.threads/forking-threads-plus-breakpoint.exp: cond_bp_target=0: detach_on_fork=on: displaced=off: inferior 1 exited (GDB internal error)
The backtrace is:
0x55925b679c85 internal_error(char const*, int, char const*, ...)
/home/simark/src/binutils-gdb/gdbsupport/errors.cc:55
0x559258deadd2 target_waitstatus::child_ptid() const
/home/simark/src/binutils-gdb/gdb/target/waitstatus.h:372
0x55925a7cbac9 remote_target::remove_new_fork_children(threads_listing_context*)
/home/simark/src/binutils-gdb/gdb/remote.c:7311
0x55925a79dfdb remote_target::update_thread_list()
/home/simark/src/binutils-gdb/gdb/remote.c:3981
0x55925ad79b83 target_update_thread_list()
/home/simark/src/binutils-gdb/gdb/target.c:3793
0x55925addbb15 update_thread_list()
/home/simark/src/binutils-gdb/gdb/thread.c:2031
0x559259d64838 stop_all_threads(char const*, inferior*)
/home/simark/src/binutils-gdb/gdb/infrun.c:5104
0x559259d88b45 keep_going_pass_signal
/home/simark/src/binutils-gdb/gdb/infrun.c:8215
0x559259d8951b keep_going
/home/simark/src/binutils-gdb/gdb/infrun.c:8251
0x559259d78835 process_event_stop_test
/home/simark/src/binutils-gdb/gdb/infrun.c:6858
0x559259d750e9 handle_signal_stop
/home/simark/src/binutils-gdb/gdb/infrun.c:6580
0x559259d6c07b handle_inferior_event
/home/simark/src/binutils-gdb/gdb/infrun.c:5832
0x559259d57db8 fetch_inferior_event()
/home/simark/src/binutils-gdb/gdb/infrun.c:4222
Indeed, the code accesses target_waitstatus::child_ptid when the kind
is TARGET_WAITKIND_THREAD_EXITED, which is not right. A
TARGET_WAITKIND_THREAD_EXITED event does not have a child_ptid value
associated, it has an exit status, which we are not interested in. The
intent is to remove from the thread list the thread that has exited.
Its ptid is found in the stop reply event, get it from there.
Change-Id: Icb298cbb80b8779fdf0c660dde9a5314d5591535
---
gdb/remote.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 8a5cc1108007..a41b174b48c0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7306,9 +7306,10 @@ remote_target::remove_new_fork_children (threads_listing_context *context)
remote_notif_get_pending_events (notif);
for (auto &event : get_remote_state ()->stop_reply_queue)
if (event->ws.kind () == TARGET_WAITKIND_FORKED
- || event->ws.kind () == TARGET_WAITKIND_VFORKED
- || event->ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
+ || event->ws.kind () == TARGET_WAITKIND_VFORKED)
context->remove_thread (event->ws.child_ptid ());
+ else if (event->ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
+ context->remove_thread (event->ptid);
}
/* Check whether any event pending in the vStopped queue would prevent a
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 8/9] gdb/remote: remove_new_fork_children don't access target_waitstatus::child_ptid if kind == TARGET_WAITKIND_THREAD_EXITED
2022-01-18 4:09 ` [PATCH v2 8/9] gdb/remote: remove_new_fork_children don't access target_waitstatus::child_ptid if kind == TARGET_WAITKIND_THREAD_EXITED Simon Marchi
@ 2022-03-31 19:25 ` Pedro Alves
0 siblings, 0 replies; 16+ messages in thread
From: Pedro Alves @ 2022-03-31 19:25 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
I thought I was done with this, but then I noticed there was a v2...
> Indeed, the code accesses target_waitstatus::child_ptid when the kind
> is TARGET_WAITKIND_THREAD_EXITED, which is not right. A
> TARGET_WAITKIND_THREAD_EXITED event does not have a child_ptid value
> associated, it has an exit status, which we are not interested in. The
> intent is to remove from the thread list the thread that has exited.
> Its ptid is found in the stop reply event, get it from there.
Hmm, indeed. I think I'll need to tweak this in the step-over-exit work, though.
Meanwhile, this is OK.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 9/9] gdb: resume ongoing step after handling fork or vfork
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (7 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 8/9] gdb/remote: remove_new_fork_children don't access target_waitstatus::child_ptid if kind == TARGET_WAITKIND_THREAD_EXITED Simon Marchi
@ 2022-01-18 4:09 ` Simon Marchi
2022-03-31 19:28 ` Pedro Alves
2022-03-23 13:02 ` [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
2022-04-05 2:13 ` Simon Marchi
10 siblings, 1 reply; 16+ messages in thread
From: Simon Marchi @ 2022-01-18 4:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
New in v2:
- updated the check in handle_inferior_event from
if (parent->inf->thread_waiting_for_vfork_done != nullptr
|| !switch_back_to_stepped_thread (ecs))
to
if ((!follow_child
&& detach_fork
&& parent->inf->thread_waiting_for_vfork_done != nullptr)
|| !switch_back_to_stepped_thread (ecs))
The `parent` pointer is stale when not following the parent, so that
broke some follow-fork-mode=child tests. I changed this expression last
minute before sending and did not re-test :(.
The test introduced by this patch would fail in this configuration, with
the native-gdbserver or native-extended-gdbserver boards:
FAIL: gdb.threads/next-fork-other-thread.exp: fork_func=fork: target-non-stop=auto: non-stop=off: displaced-stepping=auto: i=2: next to for loop
The problem is that the step operation is forgotten when handling the
fork/vfork. With "debug infrun" and "debug remote", it looks like this
(some lines omitted for brevity). We do the next:
[infrun] proceed: enter
[infrun] proceed: addr=0xffffffffffffffff, signal=GDB_SIGNAL_DEFAULT
[infrun] resume_1: step=1, signal=GDB_SIGNAL_0, trap_expected=0, current thread [4154304.4154304.0] at 0x5555555553bf
[infrun] do_target_resume: resume_ptid=4154304.0.0, step=1, sig=GDB_SIGNAL_0
[remote] Sending packet: $vCont;r5555555553bf,5555555553c4:p3f63c0.3f63c0;c:p3f63c0.-1#cd
[infrun] proceed: exit
We then handle a fork event:
[infrun] fetch_inferior_event: enter
[remote] wait: enter
[remote] Packet received: T05fork:p3f63ee.3f63ee;06:0100000000000000;07:b08e59f6ff7f0000;10:bf60e8f7ff7f0000;thread:p3f63c0.3f63c6;core:17;
[remote] wait: exit
[infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
[infrun] print_target_wait_results: 4154304.4154310.0 [Thread 4154304.4154310],
[infrun] print_target_wait_results: status->kind = FORKED, child_ptid = 4154350.4154350.0
[infrun] handle_inferior_event: status->kind = FORKED, child_ptid = 4154350.4154350.0
[remote] Sending packet: $D;3f63ee#4b
[infrun] resume_1: step=0, signal=GDB_SIGNAL_0, trap_expected=0, current thread [4154304.4154310.0] at 0x7ffff7e860bf
[infrun] do_target_resume: resume_ptid=4154304.0.0, step=0, sig=GDB_SIGNAL_0
[remote] Sending packet: $vCont;c:p3f63c0.-1#73
[infrun] fetch_inferior_event: exit
In the first snippet, we resume the stepping thread with the range-stepping (r)
vCont command. But after handling the fork (detaching the fork child), we
resumed the whole process freely. The stepping thread, which was paused by
GDBserver while reporting the fork event, was therefore resumed freely, instead
of confined to the addresses of the stepped line. Note that since this
is a "next", it could be that we have entered a function, installed a
step-resume breakpoint, and it's ok to continue freely the stepping
thread, but that's not the case here. The two snippets shown above were
next to each other in the logs.
For the fork case, we can resume stepping right after handling the
event.
However, for the vfork case, where we are waiting for the
external child process to exec or exit, we only resume the thread that
called vfork, and keep the others stopped (see patch "gdb: fix handling of
vfork by multi-threaded program" prior in this series). So we can't
resume the stepping thread right now. Instead, do it after handling the
vfork-done event.
Change-Id: I92539c970397ce880110e039fe92b87480f816bd
---
gdb/infrun.c | 23 +++-
.../gdb.threads/next-fork-other-thread.c | 86 +++++++++++++
.../gdb.threads/next-fork-other-thread.exp | 116 ++++++++++++++++++
3 files changed, 221 insertions(+), 4 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.c
create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 75252be22038..f9bc4807d954 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5744,7 +5744,18 @@ handle_inferior_event (struct execution_control_state *ecs)
ecs->ptid = inferior_ptid;
if (should_resume)
- keep_going (ecs);
+ {
+ /* Never call switch_back_to_stepped_thread if we are waiting for
+ vfork-done (waiting for an external vfork child to exec or
+ exit). We will resume only the vforking thread for the purpose
+ of collecting the vfork-done event, and we will restart any
+ step once the critical shared address space window is done. */
+ if ((!follow_child
+ && detach_fork
+ && parent->inf->thread_waiting_for_vfork_done != nullptr)
+ || !switch_back_to_stepped_thread (ecs))
+ keep_going (ecs);
+ }
else
stop_waiting (ecs);
return;
@@ -5764,9 +5775,13 @@ handle_inferior_event (struct execution_control_state *ecs)
if (handle_stop_requested (ecs))
return;
- /* This also takes care of reinserting breakpoints in the
- previously locked inferior. */
- keep_going (ecs);
+ if (!switch_back_to_stepped_thread (ecs))
+ {
+ gdb_assert (inferior_thread () == ecs->event_thread);
+ /* This also takes care of reinserting breakpoints in the
+ previously locked inferior. */
+ keep_going (ecs);
+ }
return;
case TARGET_WAITKIND_EXECD:
diff --git a/gdb/testsuite/gdb.threads/next-fork-other-thread.c b/gdb/testsuite/gdb.threads/next-fork-other-thread.c
new file mode 100644
index 000000000000..6679f92c70c5
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/next-fork-other-thread.c
@@ -0,0 +1,86 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2022 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 <pthread.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <errno.h>
+#include <assert.h>
+#include <limits.h>
+
+/* Number of threads doing forks. */
+#define N_FORKERS 4
+
+static void *
+forker (void *arg)
+{
+ for (;;)
+ {
+ pid_t pid = FORK_FUNC ();
+
+ if (pid == 0)
+ _exit(11);
+
+ assert (pid > 0);
+
+ /* Wait for children to exit. */
+ int ret;
+ int stat;
+ do {
+ ret = waitpid (pid, &stat, 0);
+ } while (ret == EINTR);
+
+ assert (ret == pid);
+ assert (WIFEXITED (stat));
+ assert (WEXITSTATUS (stat) == 11);
+
+ usleep (40 * 1000);
+ }
+
+ return NULL;
+}
+
+static void
+sleep_a_bit (void)
+{
+ usleep (1000 * 50);
+}
+
+int
+main (void)
+{
+ alarm (60);
+
+ pthread_t thread[N_FORKERS];
+ for (int i = 0; i < N_FORKERS; ++i)
+ {
+ int ret = pthread_create (&thread[i], NULL, forker, NULL);
+ assert (ret == 0);
+ }
+
+ for (int i = 0; i < INT_MAX; ++i) /* for loop */
+ {
+ sleep_a_bit (); /* break here */
+ sleep_a_bit (); /* other line */
+ }
+
+ for (int i = 0; i < N_FORKERS; ++i)
+ pthread_join (thread[i], NULL);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/next-fork-other-thread.exp b/gdb/testsuite/gdb.threads/next-fork-other-thread.exp
new file mode 100644
index 000000000000..5960bb9ba100
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/next-fork-other-thread.exp
@@ -0,0 +1,116 @@
+# Copyright 2022 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 doing a "next" on a thread during which forks or vforks happen in other
+# threads.
+
+standard_testfile
+
+# Line where to stop the main thread.
+set break_here_line [gdb_get_line_number "break here"]
+
+# Build executables, one for each fork flavor.
+foreach_with_prefix fork_func {fork vfork} {
+ set opts [list debug pthreads additional_flags=-DFORK_FUNC=${fork_func}]
+ if { [build_executable "failed to prepare" \
+ ${testfile}-${fork_func} ${srcfile} $opts] } {
+ return
+ }
+}
+
+# If testing against GDBserver, consume all it its output.
+
+proc drain_gdbserver_output { } {
+ if { [info exists ::server_spawn_id] } {
+ gdb_test_multiple "" "" {
+ -i "$::server_spawn_id"
+ -timeout 0
+ -re ".+" {
+ exp_continue
+ }
+ }
+ }
+}
+
+# Run the test with the given parameters:
+#
+# - FORK_FUNC: fork flavor, "fork" or "vfork".
+# - TARGET-NON-STOP: "maintenance set target-non-stop" value, "auto", "on" or
+# "off".
+# - NON-STOP: "set non-stop" value, "on" or "off".
+# - DISPLACED-STEPPING: "set displaced-stepping" value, "auto", "on" or "off".
+
+proc do_test { fork_func target-non-stop non-stop displaced-stepping } {
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
+ append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\""
+ clean_restart ${::binfile}-${fork_func}
+ }
+
+ gdb_test_no_output "set displaced-stepping ${displaced-stepping}"
+
+ if { ![runto_main] } {
+ return
+ }
+
+ # The "Detached after (v)fork" messages get in the way in non-stop, disable
+ # them.
+ gdb_test_no_output "set print inferior-events off"
+
+ # Advance the next-ing thread to the point where we'll execute the nexts.
+ # Leave the breakpoint in: it will force GDB to step over it while next-ing,
+ # which exercises some additional code paths.
+ gdb_test "break $::break_here_line" "Breakpoint $::decimal at $::hex.*"
+ gdb_test "continue" "hit Breakpoint $::decimal, main.*"
+
+ # Next an arbitrary number of times over the lines of the loop.
+ #
+ # It is useful to bump this number to a larger value (e.g. 200) to stress
+ # test more, but it makes the test case run for considerably longer. If
+ # you increase the number of loops, you might want to adjust the alarm
+ # time in the .c file accordingly.
+ for { set i 0 } { $i < 20 } { incr i } {
+ # If testing against GDBserver, the forking threads cause a lot of
+ # "Detaching from process XYZ" messages to appear. If we don't consume
+ # that output, GDBserver eventually blocks on a full stderr. Drain it
+ # once every loop. It may not be needed for 20 iterations, but it's
+ # needed if you increase to 200 iterations.
+ drain_gdbserver_output
+
+ with_test_prefix "i=$i" {
+ if { [gdb_test "next" "other line.*" "next to other line"] != 0 } {
+ return
+ }
+
+ if { [gdb_test "next" "for loop.*" "next to for loop"] != 0 } {
+ return
+ }
+
+ if { [gdb_test "next" "break here.*" "next to break here"] != 0} {
+ return
+ }
+ }
+ }
+}
+
+foreach_with_prefix fork_func {fork vfork} {
+ foreach_with_prefix target-non-stop {auto on off} {
+ foreach_with_prefix non-stop {off on} {
+ foreach_with_prefix displaced-stepping {auto on off} {
+ do_test ${fork_func} ${target-non-stop} ${non-stop} ${displaced-stepping}
+ }
+ }
+ }
+}
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 9/9] gdb: resume ongoing step after handling fork or vfork
2022-01-18 4:09 ` [PATCH v2 9/9] gdb: resume ongoing step after handling fork or vfork Simon Marchi
@ 2022-03-31 19:28 ` Pedro Alves
0 siblings, 0 replies; 16+ messages in thread
From: Pedro Alves @ 2022-03-31 19:28 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 2022-01-18 04:09, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> New in v2:
>
> - updated the check in handle_inferior_event from
>
> if (parent->inf->thread_waiting_for_vfork_done != nullptr
> || !switch_back_to_stepped_thread (ecs))
>
> to
>
> if ((!follow_child
> && detach_fork
> && parent->inf->thread_waiting_for_vfork_done != nullptr)
> || !switch_back_to_stepped_thread (ecs))
>
> The `parent` pointer is stale when not following the parent, so that
> broke some follow-fork-mode=child tests. I changed this expression last
> minute before sending and did not re-test :(.
>
OK with the nits I pointed out in v1 addressed.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (8 preceding siblings ...)
2022-01-18 4:09 ` [PATCH v2 9/9] gdb: resume ongoing step after handling fork or vfork Simon Marchi
@ 2022-03-23 13:02 ` Simon Marchi
2022-04-05 2:13 ` Simon Marchi
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-03-23 13:02 UTC (permalink / raw)
To: gdb-patches
Ping!
On 2022-01-17 23:09, Simon Marchi wrote:
> This is v2 of:
>
> https://sourceware.org/pipermail/gdb-patches/2022-January/185204.html
>
> The two changes are:
>
> - New patch 8: patch 7 fixes a bug gdbserver-side, which is good, but
> it then uncovers a small bug in gdb. Patch 8 fixes it.
>
> - Patch 8/8 in v1 (now 9/9) had a bug that broke some tests that detach
> the fork parent.
>
> Simon Marchi (9):
> gdb/infrun: add reason parameter to stop_all_threads
> gdb/linux-nat: remove check based on current_inferior in
> linux_handle_extended_wait
> gdb: replace inferior::waiting_for_vfork_done with
> inferior::thread_waiting_for_vfork_done
> gdb/infrun: add inferior parameters to stop_all_threads and
> restart_threads
> gdb/infrun: add logging statement to do_target_resume
> gdb: fix handling of vfork by multi-threaded program
> (follow-fork-mode=parent, detach-on-fork=on)
> gdbserver: report correct status in thread stop race condition
> gdb/remote: remove_new_fork_children don't access
> target_waitstatus::child_ptid if kind ==
> TARGET_WAITKIND_THREAD_EXITED
> gdb: resume ongoing step after handling fork or vfork
>
> gdb/infcmd.c | 2 +-
> gdb/inferior.h | 8 +-
> gdb/infrun.c | 212 ++++++++++++++++--
> gdb/infrun.h | 14 +-
> gdb/linux-nat.c | 17 +-
> gdb/remote.c | 7 +-
> .../gdb.threads/next-fork-other-thread.c | 86 +++++++
> .../gdb.threads/next-fork-other-thread.exp | 116 ++++++++++
> .../gdb.threads/vfork-multi-inferior-sleep.c | 25 +++
> .../gdb.threads/vfork-multi-inferior.c | 55 +++++
> .../gdb.threads/vfork-multi-inferior.exp | 115 ++++++++++
> .../gdb.threads/vfork-multi-thread.c | 74 ++++++
> .../gdb.threads/vfork-multi-thread.exp | 96 ++++++++
> gdbserver/linux-low.cc | 60 +++--
> 14 files changed, 806 insertions(+), 81 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.c
> create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.exp
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.exp
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.exp
>
>
> base-commit: e2c0149e8b0ce12accb8f4b02cbbeff3e91e4737
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs
2022-01-18 4:09 [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
` (9 preceding siblings ...)
2022-03-23 13:02 ` [PATCH v2 0/9] Some fixes for handling vfork by multi-threaded programs Simon Marchi
@ 2022-04-05 2:13 ` Simon Marchi
10 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2022-04-05 2:13 UTC (permalink / raw)
To: gdb-patches
On 2022-01-17 23:09, Simon Marchi wrote:
> This is v2 of:
>
> https://sourceware.org/pipermail/gdb-patches/2022-January/185204.html
>
> The two changes are:
>
> - New patch 8: patch 7 fixes a bug gdbserver-side, which is good, but
> it then uncovers a small bug in gdb. Patch 8 fixes it.
>
> - Patch 8/8 in v1 (now 9/9) had a bug that broke some tests that detach
> the fork parent.
>
> Simon Marchi (9):
> gdb/infrun: add reason parameter to stop_all_threads
> gdb/linux-nat: remove check based on current_inferior in
> linux_handle_extended_wait
> gdb: replace inferior::waiting_for_vfork_done with
> inferior::thread_waiting_for_vfork_done
> gdb/infrun: add inferior parameters to stop_all_threads and
> restart_threads
> gdb/infrun: add logging statement to do_target_resume
> gdb: fix handling of vfork by multi-threaded program
> (follow-fork-mode=parent, detach-on-fork=on)
> gdbserver: report correct status in thread stop race condition
> gdb/remote: remove_new_fork_children don't access
> target_waitstatus::child_ptid if kind ==
> TARGET_WAITKIND_THREAD_EXITED
> gdb: resume ongoing step after handling fork or vfork
>
> gdb/infcmd.c | 2 +-
> gdb/inferior.h | 8 +-
> gdb/infrun.c | 212 ++++++++++++++++--
> gdb/infrun.h | 14 +-
> gdb/linux-nat.c | 17 +-
> gdb/remote.c | 7 +-
> .../gdb.threads/next-fork-other-thread.c | 86 +++++++
> .../gdb.threads/next-fork-other-thread.exp | 116 ++++++++++
> .../gdb.threads/vfork-multi-inferior-sleep.c | 25 +++
> .../gdb.threads/vfork-multi-inferior.c | 55 +++++
> .../gdb.threads/vfork-multi-inferior.exp | 115 ++++++++++
> .../gdb.threads/vfork-multi-thread.c | 74 ++++++
> .../gdb.threads/vfork-multi-thread.exp | 96 ++++++++
> gdbserver/linux-low.cc | 60 +++--
> 14 files changed, 806 insertions(+), 81 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.c
> create mode 100644 gdb/testsuite/gdb.threads/next-fork-other-thread.exp
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior-sleep.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-inferior.exp
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.c
> create mode 100644 gdb/testsuite/gdb.threads/vfork-multi-thread.exp
>
>
> base-commit: e2c0149e8b0ce12accb8f4b02cbbeff3e91e4737
I believe I addressed all of Pedro's comments, so I went ahead and pushed this.
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread