* [PATCH 1/2] gdbserver: hide fork child threads from GDB
@ 2021-10-29 20:33 Simon Marchi
2021-10-29 20:33 ` [PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Simon Marchi @ 2021-10-29 20:33 UTC (permalink / raw)
To: gdb-patches
This patch aims at fixing a bug where an inferior is unexpectedly
created when a fork happens at the same time as another event, and that
other event is reported to GDB first (and the fork event stays pending
in GDBserver). This happens for example when we step a thread and
another thread forks at the same time. The bug looks like (if I
reproduce the included test by hand):
(gdb) show detach-on-fork
Whether gdb will detach the child of a fork is on.
(gdb) show follow-fork-mode
Debugger response to a program call of fork or vfork is "parent".
(gdb) si
[New inferior 2]
Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
Reading symbols from target:/home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread...
[New Thread 965190.965190]
[Switching to Thread 965190.965190]
Remote 'g' packet reply is too long (expected 560 bytes, got 816 bytes): ... <long series of bytes>
The sequence of events leading to the problem is:
- We are using the all-stop user-visible mode as well as the
synchronous / all-stop variant of the remote protocol
- We have two threads, thread A that we single-step and thread B that
calls fork at the same time
- GDBserver's linux_process_target::wait pulls the "single step
complete SIGTRAP" and the "fork" events from the kernel. It
arbitrarily choses one event to report, it happens to be the
single-step SIGTRAP. The fork stays pending in the thread_info.
- GDBserver send that SIGTRAP as a stop reply to GDB
- While in stop_all_threads, GDB calls update_thread_list, which ends
up querying the remote thread list using qXfer:threads:read.
- In the reply, GDBserver includes the fork child created as a result
of thread B's fork.
- GDB-side, the remote target sees the new PID, calls
remote_notice_new_inferior, which ends up unexpectedly creating a new
inferior, and things go downhill from there.
The problem here is that as long as GDB did not process the fork event,
it should pretend the fork child does not exist. Ultimately, this event
will be reported, we'll go through follow_fork, and that process will be
detached.
The remote target (GDB-side), has some code to remove from the reported
thread list the threads that are the result of forks not processed by
GDB yet. But that only works for fork events that have made their way
to the remote target (GDB-side), but haven't been consumed by the core
yet, so are still lingering as pending stop replies in the remote target
(see remove_new_fork_children in remote.c). But in our case, the fork
event hasn't made its way to the GDB-side remote target. We need to
implement the same kind of logic GDBserver-side: if there exists a
thread / inferior that is the result of a fork event GDBserver hasn't
reported yet, it should exclude that thread / inferior from the reported
thread list.
This was actually discussed a while ago, but not implemented AFAIK:
https://pi.simark.ca/gdb-patches/1ad9f5a8-d00e-9a26-b0c9-3f4066af5142@redhat.com/#t
https://sourceware.org/pipermail/gdb-patches/2016-June/133906.html
Implementation details-wise, the fix for this is all in GDBserver. The
Linux layer of GDBserver already tracks unreported fork parent / child
relationships using the lwp_info::fork_relative, in order to avoid
wildcard actions resuming fork childs unknown to GDB. Move this
information to the common thread_info structure, so that it can be used
in handle_qxfer_threads_worker to filter out unreported fork child
threads. Plus, that makes it usable for other OSes that use fork if
need be.
I split the field in two (fork_parent / fork_child), I think it's
clearer this way, easier to follow which thread is the parent and which
is the child, and helps ensure things are consistent. That simplifies
things a bit in linux_set_resume_request. Currently, when
lwp->fork_relative is set, we have to deduce whether this is a parent or
child using the pending event. With separate fork_parent and fork_child
fields, it becomes more obvious. If the thread has a fork parent, then
it means it's a fork child, and vice-versa.
Testing-wise, the test replicates pretty-much the sequence of events
shown above. The setup of the test makes it such that the main thread
is about to fork. We stepi the other thread, so that the step completes
very quickly, in a single event. Meanwhile, the main thread is resumed,
so very likely has time to call fork. This means that the bug may not
reproduce every time (if the main thread does not have time to call
fork), but it will reproduce more often than not. The test fails
without the fix applied on the native-gdbserver and
native-extended-gdbserver boards.
At some point I suspected that which thread called fork and which thread
did the step influenced the order in which the events were reported, and
therefore the reproducibility of the bug. So I made the test try both
combinations: main thread forks while other thread steps, and vice
versa. I'm not sure this is still necessary, but I left it there
anyway. It doesn't hurt to test a few more combinations.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28288
Change-Id: I2158d5732fc7d7ca06b0eb01f88cf27bf527b990
---
.../gdb.threads/pending-fork-event.c | 82 ++++++++++++++++++
.../gdb.threads/pending-fork-event.exp | 86 +++++++++++++++++++
gdbserver/gdbthread.h | 9 ++
gdbserver/linux-low.cc | 36 ++++----
gdbserver/linux-low.h | 6 --
gdbserver/server.cc | 6 ++
6 files changed, 202 insertions(+), 23 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
new file mode 100644
index 000000000000..a39ca75a49ac
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
@@ -0,0 +1,82 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 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 <assert.h>
+
+static volatile int release_forking_thread = 0;
+static int x;
+static pthread_barrier_t barrier;
+
+static void
+break_here (void)
+{
+ x++;
+}
+
+static void
+do_fork (void)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (!release_forking_thread);
+
+ if (FORK_FUNCTION () == 0)
+ _exit (0);
+
+}
+
+static void *
+thread_func (void *p)
+{
+#if defined(MAIN_THREAD_FORKS)
+ break_here ();
+#elif defined(OTHER_THREAD_FORKS)
+ do_fork ();
+#else
+# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
+#endif
+}
+
+
+int
+main (void)
+{
+ pthread_t thread;
+ int ret;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ alarm (30);
+ ret = pthread_create (&thread, NULL, thread_func, NULL);
+ assert (ret == 0);
+
+ pthread_barrier_wait (&barrier);
+
+#if defined(MAIN_THREAD_FORKS)
+ do_fork ();
+#elif defined(OTHER_THREAD_FORKS)
+ break_here ();
+#else
+# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
+#endif
+
+ pthread_join (thread, NULL);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
new file mode 100644
index 000000000000..6574b9abf5b7
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -0,0 +1,86 @@
+# Copyright (C) 2021 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 we handle well, in all-stop, a fork happening in a thread B while
+# doing a step-like command in a thread A.
+#
+# The scenario we want to test for is:
+#
+# - the step of thread A completes, we handle the event and stop all threads
+# as a result
+# - while stopping all threads, thread B reports a fork event which stays
+# pending
+#
+# A buggy GDB / GDBserver combo would notice the thread of the child process
+# of the (still unprocessed) fork event, and erroneously create a new inferior
+# for it. Once fixed, the child process' thread is hidden by whoever holds the
+# pending fork event.
+
+standard_testfile
+
+proc do_test { target-non-stop who_forks fork_function } {
+
+ set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
+
+ # WHO_FORKS says which of the main or other thread calls (v)fork. The
+ # thread that does not call (v)fork is the one who tries to step.
+ if { $who_forks == "main" } {
+ lappend opts "additional_flags=-DMAIN_THREAD_FORKS"
+ set this_binfile ${::binfile}-main-${fork_function}
+ } elseif { $who_forks == "other" } {
+ lappend opts "additional_flags=-DOTHER_THREAD_FORKS"
+ set this_binfile ${::binfile}-other-${fork_function}
+ } else {
+ error "invalid who_forks value: $who_forks"
+ }
+
+ if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
+ return
+ }
+
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
+ clean_restart $this_binfile
+ }
+
+ if {![runto_main]} {
+ fail "could not run to main"
+ return
+ }
+
+ # Run until breakpoint in the second thread.
+ gdb_test "break break_here" "Breakpoint $::decimal.*"
+ gdb_continue_to_breakpoint "thread started"
+
+ # Delete the breakpoint so the thread doesn't do a step-over.
+ delete_breakpoints
+
+ # Let the forking thread make progress during the step.
+ gdb_test "p release_forking_thread = 1" " = 1"
+
+ # stepi the non-forking thread.
+ gdb_test "stepi"
+
+ # Make sure there's still a single inferior.
+ gdb_test "info inferior" {\* 1 [^\r\n]+}
+}
+
+foreach_with_prefix target-non-stop { auto on off } {
+ foreach_with_prefix who_forks { main other } {
+ foreach_with_prefix fork_function { fork vfork } {
+ do_test ${target-non-stop} $who_forks $fork_function
+ }
+ }
+}
diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 7c293b1f89d2..9f9f69eb1ddd 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -80,6 +80,15 @@ struct thread_info
/* Branch trace target information for this thread. */
struct btrace_target_info *btrace = nullptr;
+
+ /* A pointer to this thread's fork child or fork parent. Valid only while
+ the parent fork or vfork event is not reported to GDB.
+
+ Used to avoid wildcard vCont actions resuming a (v)fork child before GDB is
+ notified about the parent's (v)fork event. Also used to avoid including the
+ (v)fork child in thread list packet responses (e.g. qfThreadInfo). */
+ thread_info *fork_child = nullptr;
+ thread_info *fork_parent = nullptr;
};
extern std::list<thread_info *> all_threads;
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 34ede238d19f..ff001475a9f0 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -564,10 +564,10 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
event_lwp->status_pending_p = 1;
event_lwp->status_pending = wstat;
- /* Link the threads until the parent event is passed on to
- higher layers. */
- event_lwp->fork_relative = child_lwp;
- child_lwp->fork_relative = event_lwp;
+ /* Link the threads until the parent's event is passed on to
+ GDB. */
+ event_lwp->thread->fork_child = child_lwp->thread;
+ child_lwp->thread->fork_parent = event_lwp->thread;
/* If the parent thread is doing step-over with single-step
breakpoints, the list of single-step breakpoints are cloned
@@ -3594,8 +3594,10 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED
|| event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED)
{
- event_child->fork_relative->fork_relative = NULL;
- event_child->fork_relative = NULL;
+ gdb_assert (event_child->thread->fork_child != nullptr);
+ gdb_assert (event_child->thread->fork_child->fork_parent != nullptr);
+ event_child->thread->fork_child->fork_parent = nullptr;
+ event_child->thread->fork_child = nullptr;
}
*ourstatus = event_child->waitstatus;
@@ -4375,19 +4377,19 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n)
/* Don't let wildcard resumes resume fork children that GDB
does not yet know are new fork children. */
- if (lwp->fork_relative != NULL)
+ if (lwp->thread->fork_parent != nullptr)
{
- struct lwp_info *rel = lwp->fork_relative;
+ lwp_info *parent = get_thread_lwp (lwp->thread->fork_parent);
+ target_waitkind kind = parent->waitstatus.kind ();
- if (rel->status_pending_p
- && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED
- || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED))
- {
- if (debug_threads)
- debug_printf ("not resuming LWP %ld: has queued stop reply\n",
- lwpid_of (thread));
- continue;
- }
+ gdb_assert (parent->status_pending_p);
+ gdb_assert (kind == TARGET_WAITKIND_FORKED
+ || kind == TARGET_WAITKIND_VFORKED);
+
+ if (debug_threads)
+ debug_printf ("not resuming LWP %ld: is a fork child not know to GDB\n",
+ lwpid_of (thread));
+ continue;
}
/* If the thread has a pending event that has already been
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 05067ffc6e6f..46aff0a29524 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -756,12 +756,6 @@ struct lwp_info
information or exit status until it can be reported to GDB. */
struct target_waitstatus waitstatus;
- /* A pointer to the fork child/parent relative. Valid only while
- the parent fork event is not reported to higher layers. Used to
- avoid wildcard vCont actions resuming a fork child before GDB is
- notified about the parent's fork event. */
- struct lwp_info *fork_relative = nullptr;
-
/* When stopped is set, this is where the lwp last stopped, with
decr_pc_after_break already accounted for. If the LWP is
running, this is the address at which the lwp was resumed. */
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c58f7e01d8da..9890d6cc9798 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1656,6 +1656,12 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer)
gdb_byte *handle;
bool handle_status = target_thread_handle (ptid, &handle, &handle_len);
+ /* If this is a fork or vfork child (has a fork parent), GDB does not yet
+ know about this process, and must not know about it until it gets the
+ corresponding (v)fork event. Exclude this thread from the list. */
+ if (thread->fork_parent != nullptr)
+ return;
+
write_ptid (ptid_s, ptid);
buffer_xml_printf (buffer, "<thread id=\"%s\"", ptid_s);
--
2.33.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent
2021-10-29 20:33 [PATCH 1/2] gdbserver: hide fork child threads from GDB Simon Marchi
@ 2021-10-29 20:33 ` Simon Marchi
2021-11-12 20:54 ` [PATCH 1/2] gdbserver: hide fork child threads from GDB (ping) Simon Marchi
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2 siblings, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2021-10-29 20:33 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
While working with pending fork events, I wondered what would happen if
the user detached an inferior while a thread of that inferior had a
pending fork event. What happens with the fork child, which is
ptrace-attached by the GDB process (or by GDBserver), but not known to
the core? Sure enough, neither the core of GDB or the target detach the
child process, so GDB (or GDBserver) just stays ptrace-attached to the
process. The result is that the fork child process is stuck, while you
would expect it to be detached and run.
When debugging on Linux, there are multiple layers in which the fork
event can be pendings:
- in GDBserver, where a fork child is saved in the
thread_info::fork_child field
- in the Linux native target, in the lwp_info::status field
- in the core of GDB, in thread_info's pending wait status
I suppose that a pending fork event could also be in the remote target's
stop reply queue, but I didn't find a case where would could detach
while having pending events there.
This patch makes each actor listed above (GDBserver, the Linux native
target, core GDB) take care of detaching any pending fork child it is
responsible for when an inferior is detached.
- In GDBserver, that is done in the generic handle_detach function.
Since a process_info already exists for the child, we can simply call
detach_inferior.
- In the Linux native target, when detacing a single thread, we check
its status to see if it's a fork or vfork, and ptrace-detach the
child before ptrace-detaching the thread itself. Move the
ptrace-detach code to a new function (detach_one_pid) to avoid code
duplication.
- In the core of GDB, check for a thread with a fork pending waitstatus
in target_detach. For this, we need a way to ask the target stack to
detach a given pid that is not bound to an inferior. I started by
trying to shove that in the existing `detach` method (by making it
take a pid instead of an inferior), but that was message. I ended up
adding a new `detach_pid` method.
Update the gdb.threads/pending-fork-event.exp to try to detach the
process while a thread has a pending fork event. In order to verify
that the fork child process is correctly detached and resumes execution
outside of GDB's control, make that process create a file in the test
output directory, and make the test wait $timeout seconds for that file
to appear (it happens instantly if everything goes well).
The test catches a bug in linux-nat.c, also reported as PR 28512
("waitstatus.h:300: internal-error: gdb_signal target_waitstatus::sig()
const: Assertion `m_kind == TARGET_WAITKIND_STOPPED || m_kind ==
TARGET_WAITKIND_SIGNALLED' failed.). When detaching a thread with a
pending event, get_detach_signal unconditionally fetches the signal
stored in the waitstatus (`tp->pending_waitstatus ().sig ()`). However,
that is only valid if the pending event is of type
TARGET_WAITKIND_STOPPED, and this is now enforced using assertions (iit
would also be valid for TARGET_WAITKIND_SIGNALLED, but that would mean
the thread does not exist anymore, so we wouldn't be detaching it). Add
a condition in get_detach_signal to access the signal number only if the
wait status is of kind TARGET_WAITKIND_STOPPED, and use GDB_SIGNAL_0
instead (since the thread was not stopped with a signal to begin with).
Change-Id: I6d811a56f520e3cb92d5ea563ad38976f92e93dd
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28512
---
gdb/linux-nat.c | 113 ++++++++++++------
gdb/linux-nat.h | 1 +
gdb/remote.c | 9 ++
gdb/target-delegates.c | 24 ++++
gdb/target.c | 37 ++++--
gdb/target.h | 10 ++
.../pending-fork-event-touch-file.c | 26 ++++
.../gdb.threads/pending-fork-event.c | 6 +-
.../gdb.threads/pending-fork-event.exp | 76 +++++++++++-
gdbserver/server.cc | 29 +++++
10 files changed, 276 insertions(+), 55 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index cada889c5348..fa86e25e2609 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1226,6 +1226,45 @@ linux_nat_target::attach (const char *args, int from_tty)
target_async (1);
}
+/* Ptrace-detach the thread with pid PID. */
+
+static void
+detach_one_pid (int pid, int signo)
+{
+ if (ptrace (PTRACE_DETACH, pid, 0, signo) < 0)
+ {
+ int save_errno = errno;
+
+ /* We know the thread exists, so ESRCH must mean the lwp is
+ zombie. This can happen if one of the already-detached
+ threads exits the whole thread group. In that case we're
+ still attached, and must reap the lwp. */
+ if (save_errno == ESRCH)
+ {
+ int ret, status;
+
+ ret = my_waitpid (pid, &status, __WALL);
+ if (ret == -1)
+ {
+ warning (_("Couldn't reap LWP %d while detaching: %s"),
+ pid, safe_strerror (errno));
+ }
+ else if (!WIFEXITED (status) && !WIFSIGNALED (status))
+ {
+ warning (_("Reaping LWP %d while detaching "
+ "returned unexpected status 0x%x"),
+ pid, status);
+ }
+ }
+ else
+ error (_("Can't detach %d: %s"),
+ pid, safe_strerror (save_errno));
+ }
+ else
+ linux_nat_debug_printf ("PTRACE_DETACH (%d, %s, 0) (OK)",
+ pid, strsignal (signo));
+}
+
/* Get pending signal of THREAD as a host signal number, for detaching
purposes. This is the signal the thread last stopped for, which we
need to deliver to the thread when detaching, otherwise, it'd be
@@ -1268,7 +1307,16 @@ get_detach_signal (struct lwp_info *lp)
if (target_is_non_stop_p () && !tp->executing ())
{
if (tp->has_pending_waitstatus ())
- signo = tp->pending_waitstatus ().sig ();
+ {
+ /* If the thread has a pending event, and it was stopped with a
+ signal, use that signal to resume it. If it has a pending
+ event of another kind, it was not stopped with a signal, so
+ resume it without a signal. */
+ if (tp->pending_waitstatus ().kind () == TARGET_WAITKIND_STOPPED)
+ signo = tp->pending_waitstatus ().sig ();
+ else
+ signo = GDB_SIGNAL_0;
+ }
else
signo = tp->stop_signal ();
}
@@ -1320,6 +1368,24 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
+ /* If the lwp/thread we are about to detach has a pending fork event,
+ there is a process GDB is attached to that the core of GDB doesn't know
+ about. Detach from it. */
+ if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
+ {
+ int event = linux_ptrace_get_extended_event (lp->status);
+
+ if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
+ {
+ unsigned long child_pid;
+ int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
+ if (ret == 0)
+ detach_one_pid (child_pid, 0);
+ else
+ perror_warning_with_name (_("Failed to detach fork child"));
+ }
+ }
+
if (lp->status != 0)
linux_nat_debug_printf ("Pending %s for %s on detach.",
strsignal (WSTOPSIG (lp->status)),
@@ -1356,42 +1422,7 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
throw;
}
- if (ptrace (PTRACE_DETACH, lwpid, 0, signo) < 0)
- {
- int save_errno = errno;
-
- /* We know the thread exists, so ESRCH must mean the lwp is
- zombie. This can happen if one of the already-detached
- threads exits the whole thread group. In that case we're
- still attached, and must reap the lwp. */
- if (save_errno == ESRCH)
- {
- int ret, status;
-
- ret = my_waitpid (lwpid, &status, __WALL);
- if (ret == -1)
- {
- warning (_("Couldn't reap LWP %d while detaching: %s"),
- lwpid, safe_strerror (errno));
- }
- else if (!WIFEXITED (status) && !WIFSIGNALED (status))
- {
- warning (_("Reaping LWP %d while detaching "
- "returned unexpected status 0x%x"),
- lwpid, status);
- }
- }
- else
- {
- error (_("Can't detach %s: %s"),
- target_pid_to_str (lp->ptid).c_str (),
- safe_strerror (save_errno));
- }
- }
- else
- linux_nat_debug_printf ("PTRACE_DETACH (%s, %s, 0) (OK)",
- target_pid_to_str (lp->ptid).c_str (),
- strsignal (signo));
+ detach_one_pid (lwpid, signo);
delete_lwp (lp->ptid);
}
@@ -1407,6 +1438,14 @@ detach_callback (struct lwp_info *lp)
return 0;
}
+/* Implementation of target_ops::detach_pid. */
+
+void
+linux_nat_target::detach_pid (int pid)
+{
+ detach_one_pid (pid, 0);
+}
+
void
linux_nat_target::detach (inferior *inf, int from_tty)
{
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 95e26b7ee460..2ba5f09761a9 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -43,6 +43,7 @@ class linux_nat_target : public inf_ptrace_target
void attach (const char *, int) override;
void detach (inferior *, int) override;
+ void detach_pid (int pid) override;
void resume (ptid_t, int, enum gdb_signal) override;
diff --git a/gdb/remote.c b/gdb/remote.c
index 0fb427535960..8bc5bc7a1524 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -424,6 +424,7 @@ class remote_target : public process_stratum_target
void close () override;
void detach (inferior *, int) override;
+ void detach_pid (int pid) override;
void disconnect (const char *, int) override;
void commit_resumed () override;
@@ -5910,6 +5911,14 @@ remote_target::detach (inferior *inf, int from_tty)
remote_detach_1 (inf, from_tty);
}
+/* Implementation of target_ops::detach_pid. */
+
+void
+remote_target::detach_pid (int pid)
+{
+ remote_detach_pid (pid);
+}
+
void
extended_remote_target::detach (inferior *inf, int from_tty)
{
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index fb9c78a5f793..2ed214ae400d 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -12,6 +12,7 @@ struct dummy_target : public target_ops
void post_attach (int arg0) override;
void detach (inferior *arg0, int arg1) override;
+ void detach_pid (int arg0) override;
void disconnect (const char *arg0, int arg1) override;
void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
void commit_resumed () override;
@@ -187,6 +188,7 @@ struct debug_target : public target_ops
void post_attach (int arg0) override;
void detach (inferior *arg0, int arg1) override;
+ void detach_pid (int arg0) override;
void disconnect (const char *arg0, int arg1) override;
void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
void commit_resumed () override;
@@ -398,6 +400,28 @@ debug_target::detach (inferior *arg0, int arg1)
fputs_unfiltered (")\n", gdb_stdlog);
}
+void
+target_ops::detach_pid (int arg0)
+{
+ this->beneath ()->detach_pid (arg0);
+}
+
+void
+dummy_target::detach_pid (int arg0)
+{
+ default_detach_pid (this, arg0);
+}
+
+void
+debug_target::detach_pid (int arg0)
+{
+ fprintf_unfiltered (gdb_stdlog, "-> %s->detach_pid (...)\n", this->beneath ()->shortname ());
+ this->beneath ()->detach_pid (arg0);
+ fprintf_unfiltered (gdb_stdlog, "<- %s->detach_pid (", this->beneath ()->shortname ());
+ target_debug_print_int (arg0);
+ fputs_unfiltered (")\n", gdb_stdlog);
+}
+
void
target_ops::disconnect (const char *arg0, int arg1)
{
diff --git a/gdb/target.c b/gdb/target.c
index 6219393987e8..5e88d59b782f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1150,6 +1150,12 @@ default_execution_direction (struct target_ops *self)
to_execution_direction must be implemented for reverse async");
}
+static void
+default_detach_pid (target_ops *self, int pid)
+{
+ gdb_assert_not_reached ("Target does not implement the detach_pid method.");
+}
+
/* See target.h. */
void
@@ -2548,11 +2554,6 @@ target_preopen (int from_tty)
void
target_detach (inferior *inf, int from_tty)
{
- /* After we have detached, we will clear the register cache for this inferior
- by calling registers_changed_ptid. We must save the pid_ptid before
- detaching, as the target detach method will clear inf->pid. */
- ptid_t save_pid_ptid = ptid_t (inf->pid);
-
/* As long as some to_detach implementations rely on the current_inferior
(either directly, or indirectly, like through target_gdbarch or by
reading memory), INF needs to be the current inferior. When that
@@ -2560,17 +2561,35 @@ target_detach (inferior *inf, int from_tty)
assertion. */
gdb_assert (inf == current_inferior ());
+ process_stratum_target *proc_target = inf->process_target ();
+
+ /* Check for a pending fork event stored in the thread_info structure,
+ detach from the fork child. */
+ for (thread_info *thread : inf->non_exited_threads ())
+ {
+ if (!thread->has_pending_waitstatus ())
+ continue;
+
+ const target_waitstatus &ws = thread->pending_waitstatus ();
+
+ if (ws.kind () == TARGET_WAITKIND_FORKED
+ || ws.kind () == TARGET_WAITKIND_VFORKED)
+ inf->top_target ()->detach_pid (ws.child_ptid ().pid ());
+ }
+
+ /* After we have detached, we will clear the register cache for this inferior
+ by calling registers_changed_ptid. We must save the pid_ptid before
+ detaching, as the target detach method will clear inf->pid. */
+ ptid_t save_pid_ptid = ptid_t (inf->pid);
+
prepare_for_detach ();
/* Hold a strong reference because detaching may unpush the
target. */
- auto proc_target_ref = target_ops_ref::new_reference (inf->process_target ());
+ auto proc_target_ref = target_ops_ref::new_reference (proc_target);
current_inferior ()->top_target ()->detach (inf, from_tty);
- process_stratum_target *proc_target
- = as_process_stratum_target (proc_target_ref.get ());
-
registers_changed_ptid (proc_target, save_pid_ptid);
/* We have to ensure we have no frame cache left. Normally,
diff --git a/gdb/target.h b/gdb/target.h
index 4dc17fd4806d..e68ddc471e97 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -479,6 +479,16 @@ struct target_ops
virtual void detach (inferior *, int)
TARGET_DEFAULT_IGNORE ();
+ /* Detach from process with pid PID.
+
+ The difference between the `detach` method and this one is that `detach`
+ is used to detach an inferior, whereas `detach_pid` is used to detach
+ a process not bound to an inferior. For example, when detaching from
+ an inferior that has a pending fork event, `detach_pid` is used to
+ detach the fork child. */
+ virtual void detach_pid (int pid)
+ TARGET_DEFAULT_FUNC (default_detach_pid);
+
virtual void disconnect (const char *, int)
TARGET_DEFAULT_NORETURN (tcomplain ());
virtual void resume (ptid_t,
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
new file mode 100644
index 000000000000..5536381847b1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+
+int
+main (void)
+{
+ FILE *f = fopen (TOUCH_FILE_PATH, "w");
+ fclose (f);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
index a39ca75a49ac..bab4a99e5786 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.c
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
@@ -32,18 +32,18 @@ break_here (void)
static void
do_fork (void)
{
- pthread_barrier_wait (&barrier);
-
while (!release_forking_thread);
if (FORK_FUNCTION () == 0)
- _exit (0);
+ execl (TOUCH_FILE_BIN, TOUCH_FILE_BIN, NULL);
}
static void *
thread_func (void *p)
{
+ pthread_barrier_wait (&barrier);
+
#if defined(MAIN_THREAD_FORKS)
break_here ();
#elif defined(OTHER_THREAD_FORKS)
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
index 6574b9abf5b7..cbe55c2f5e7c 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.exp
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -28,11 +28,32 @@
# for it. Once fixed, the child process' thread is hidden by whoever holds the
# pending fork event.
-standard_testfile
+standard_testfile .c -touch-file.c
-proc do_test { target-non-stop who_forks fork_function } {
+set touch_file_bin $binfile-touch-file
+set touch_file_path [standard_output_file some-file]
- set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
+set opts [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]
+if { [gdb_compile "$srcdir/$subdir/$srcfile2" $touch_file_bin executable $opts] != "" } {
+ return 0
+}
+
+# Run until execution is stopped and a thread has a pending fork event.
+#
+# WHO_FORKS tells which of the thread program threads does the fork ("main" or
+# "other").
+#
+# FORK_FUNCTION tells which function to call to fork ("fork" or "vfork").
+#
+# Return 1 on success, 0 otherwise.
+
+proc setup { target-non-stop who_forks fork_function } {
+ set this_binfile ${::binfile}-${fork_function}
+
+ set opts [list \
+ debug \
+ "additional_flags=-DFORK_FUNCTION=$fork_function" \
+ "additional_flags=-DTOUCH_FILE_BIN=\"$::touch_file_bin\""]
# WHO_FORKS says which of the main or other thread calls (v)fork. The
# thread that does not call (v)fork is the one who tries to step.
@@ -47,7 +68,7 @@ proc do_test { target-non-stop who_forks fork_function } {
}
if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
- return
+ return 0
}
save_vars { ::GDBFLAGS } {
@@ -57,7 +78,7 @@ proc do_test { target-non-stop who_forks fork_function } {
if {![runto_main]} {
fail "could not run to main"
- return
+ return 0
}
# Run until breakpoint in the second thread.
@@ -75,12 +96,55 @@ proc do_test { target-non-stop who_forks fork_function } {
# Make sure there's still a single inferior.
gdb_test "info inferior" {\* 1 [^\r\n]+}
+
+ return 1
+}
+
+# Return 1 if file ::TOUCH_FILE_PATH exists, else 0.
+
+proc file_exists {} {
+ return [file exists $::touch_file_path]
+}
+
+# Wait up to ::TIMEOUT seconds for file ::TOUCH_FILE_PATH to exist. Return
+# 1 if it does exist, 0 otherwise.
+
+proc file_exists_with_timeout {} {
+ for {set i 0} {$i < $::timeout} {incr i} {
+ if { [file_exists] } {
+ return 1
+ }
+
+ sleep 1
+ }
+
+ return 0
+}
+
+# Run until there exists (at least, likely exists) a pending fork event.
+# Detach the fork parent. Verify that the fork child (which does not appear in
+# GDB, since the fork event is pending) is detached as well.
+
+proc_with_prefix do_test_detach { target-non-stop who_forks fork_function } {
+ file delete $::touch_file_path
+ gdb_assert { ![file_exists] } "file does not exist before test"
+
+ if { ![setup ${target-non-stop} $who_forks $fork_function] } {
+ return
+ }
+
+ gdb_test "detach"
+
+ # After being detached, the fork child creates file ::TOUCH_FILE_PATH.
+ # Seeing this file tells us the fork child was detached and executed
+ # successfully.
+ gdb_assert { [file_exists_with_timeout] } "file exists after detach"
}
foreach_with_prefix target-non-stop { auto on off } {
foreach_with_prefix who_forks { main other } {
foreach_with_prefix fork_function { fork vfork } {
- do_test ${target-non-stop} $who_forks $fork_function
+ do_test_detach ${target-non-stop} $who_forks $fork_function
}
}
}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 9890d6cc9798..f34da89d5a6e 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
/* We'll need this after PROCESS has been destroyed. */
int pid = process->pid;
+ /* If this process has an unreported fork child, that child is not known to
+ GDB, so detach it as well.
+
+ Here, we specifically don't want to use "safe iteration", as detaching
+ another process might delete the next thread in the iteration, which is
+ the one saved by the safe iterator. We will never delete the currently
+ iterated on thread, so standard iteration should be safe. */
+ for (thread_info *thread : all_threads)
+ {
+ /* Only threads that are of the process we are detaching. */
+ if (thread->id.pid () != pid)
+ continue;
+
+ /* Only threads that have a pending fork event. */
+ if (thread->fork_child == nullptr)
+ continue;
+
+ process_info *fork_child_process
+ = find_process_pid (thread->fork_child->id.pid ());
+ gdb_assert (fork_child_process != nullptr);
+
+ int fork_child_pid = fork_child_process->pid;
+
+ if (detach_inferior (fork_child_process) != 0)
+ warning (_("Failed to detach fork child %s, child of %s"),
+ target_pid_to_str (ptid_t (fork_child_pid)).c_str (),
+ target_pid_to_str (thread->id).c_str ());
+ }
+
if (detach_inferior (process) != 0)
write_enn (own_buf);
else
--
2.33.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] gdbserver: hide fork child threads from GDB (ping)
2021-10-29 20:33 [PATCH 1/2] gdbserver: hide fork child threads from GDB Simon Marchi
2021-10-29 20:33 ` [PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
@ 2021-11-12 20:54 ` Simon Marchi
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2 siblings, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2021-11-12 20:54 UTC (permalink / raw)
To: gdb-patches
Ping.
On 2021-10-29 4:33 p.m., Simon Marchi wrote:
> This patch aims at fixing a bug where an inferior is unexpectedly
> created when a fork happens at the same time as another event, and that
> other event is reported to GDB first (and the fork event stays pending
> in GDBserver). This happens for example when we step a thread and
> another thread forks at the same time. The bug looks like (if I
> reproduce the included test by hand):
>
> (gdb) show detach-on-fork
> Whether gdb will detach the child of a fork is on.
> (gdb) show follow-fork-mode
> Debugger response to a program call of fork or vfork is "parent".
> (gdb) si
> [New inferior 2]
> Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
> Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
> Reading symbols from target:/home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread...
> [New Thread 965190.965190]
> [Switching to Thread 965190.965190]
> Remote 'g' packet reply is too long (expected 560 bytes, got 816 bytes): ... <long series of bytes>
>
> The sequence of events leading to the problem is:
>
> - We are using the all-stop user-visible mode as well as the
> synchronous / all-stop variant of the remote protocol
> - We have two threads, thread A that we single-step and thread B that
> calls fork at the same time
> - GDBserver's linux_process_target::wait pulls the "single step
> complete SIGTRAP" and the "fork" events from the kernel. It
> arbitrarily choses one event to report, it happens to be the
> single-step SIGTRAP. The fork stays pending in the thread_info.
> - GDBserver send that SIGTRAP as a stop reply to GDB
> - While in stop_all_threads, GDB calls update_thread_list, which ends
> up querying the remote thread list using qXfer:threads:read.
> - In the reply, GDBserver includes the fork child created as a result
> of thread B's fork.
> - GDB-side, the remote target sees the new PID, calls
> remote_notice_new_inferior, which ends up unexpectedly creating a new
> inferior, and things go downhill from there.
>
> The problem here is that as long as GDB did not process the fork event,
> it should pretend the fork child does not exist. Ultimately, this event
> will be reported, we'll go through follow_fork, and that process will be
> detached.
>
> The remote target (GDB-side), has some code to remove from the reported
> thread list the threads that are the result of forks not processed by
> GDB yet. But that only works for fork events that have made their way
> to the remote target (GDB-side), but haven't been consumed by the core
> yet, so are still lingering as pending stop replies in the remote target
> (see remove_new_fork_children in remote.c). But in our case, the fork
> event hasn't made its way to the GDB-side remote target. We need to
> implement the same kind of logic GDBserver-side: if there exists a
> thread / inferior that is the result of a fork event GDBserver hasn't
> reported yet, it should exclude that thread / inferior from the reported
> thread list.
>
> This was actually discussed a while ago, but not implemented AFAIK:
>
> https://pi.simark.ca/gdb-patches/1ad9f5a8-d00e-9a26-b0c9-3f4066af5142@redhat.com/#t
> https://sourceware.org/pipermail/gdb-patches/2016-June/133906.html
>
> Implementation details-wise, the fix for this is all in GDBserver. The
> Linux layer of GDBserver already tracks unreported fork parent / child
> relationships using the lwp_info::fork_relative, in order to avoid
> wildcard actions resuming fork childs unknown to GDB. Move this
> information to the common thread_info structure, so that it can be used
> in handle_qxfer_threads_worker to filter out unreported fork child
> threads. Plus, that makes it usable for other OSes that use fork if
> need be.
>
> I split the field in two (fork_parent / fork_child), I think it's
> clearer this way, easier to follow which thread is the parent and which
> is the child, and helps ensure things are consistent. That simplifies
> things a bit in linux_set_resume_request. Currently, when
> lwp->fork_relative is set, we have to deduce whether this is a parent or
> child using the pending event. With separate fork_parent and fork_child
> fields, it becomes more obvious. If the thread has a fork parent, then
> it means it's a fork child, and vice-versa.
>
> Testing-wise, the test replicates pretty-much the sequence of events
> shown above. The setup of the test makes it such that the main thread
> is about to fork. We stepi the other thread, so that the step completes
> very quickly, in a single event. Meanwhile, the main thread is resumed,
> so very likely has time to call fork. This means that the bug may not
> reproduce every time (if the main thread does not have time to call
> fork), but it will reproduce more often than not. The test fails
> without the fix applied on the native-gdbserver and
> native-extended-gdbserver boards.
>
> At some point I suspected that which thread called fork and which thread
> did the step influenced the order in which the events were reported, and
> therefore the reproducibility of the bug. So I made the test try both
> combinations: main thread forks while other thread steps, and vice
> versa. I'm not sure this is still necessary, but I left it there
> anyway. It doesn't hurt to test a few more combinations.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28288
> Change-Id: I2158d5732fc7d7ca06b0eb01f88cf27bf527b990
> ---
> .../gdb.threads/pending-fork-event.c | 82 ++++++++++++++++++
> .../gdb.threads/pending-fork-event.exp | 86 +++++++++++++++++++
> gdbserver/gdbthread.h | 9 ++
> gdbserver/linux-low.cc | 36 ++++----
> gdbserver/linux-low.h | 6 --
> gdbserver/server.cc | 6 ++
> 6 files changed, 202 insertions(+), 23 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c
> create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp
>
> diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
> new file mode 100644
> index 000000000000..a39ca75a49ac
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
> @@ -0,0 +1,82 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2021 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 <assert.h>
> +
> +static volatile int release_forking_thread = 0;
> +static int x;
> +static pthread_barrier_t barrier;
> +
> +static void
> +break_here (void)
> +{
> + x++;
> +}
> +
> +static void
> +do_fork (void)
> +{
> + pthread_barrier_wait (&barrier);
> +
> + while (!release_forking_thread);
> +
> + if (FORK_FUNCTION () == 0)
> + _exit (0);
> +
> +}
> +
> +static void *
> +thread_func (void *p)
> +{
> +#if defined(MAIN_THREAD_FORKS)
> + break_here ();
> +#elif defined(OTHER_THREAD_FORKS)
> + do_fork ();
> +#else
> +# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
> +#endif
> +}
> +
> +
> +int
> +main (void)
> +{
> + pthread_t thread;
> + int ret;
> +
> + pthread_barrier_init (&barrier, NULL, 2);
> +
> + alarm (30);
> + ret = pthread_create (&thread, NULL, thread_func, NULL);
> + assert (ret == 0);
> +
> + pthread_barrier_wait (&barrier);
> +
> +#if defined(MAIN_THREAD_FORKS)
> + do_fork ();
> +#elif defined(OTHER_THREAD_FORKS)
> + break_here ();
> +#else
> +# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
> +#endif
> +
> + pthread_join (thread, NULL);
> +
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
> new file mode 100644
> index 000000000000..6574b9abf5b7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
> @@ -0,0 +1,86 @@
> +# Copyright (C) 2021 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 we handle well, in all-stop, a fork happening in a thread B while
> +# doing a step-like command in a thread A.
> +#
> +# The scenario we want to test for is:
> +#
> +# - the step of thread A completes, we handle the event and stop all threads
> +# as a result
> +# - while stopping all threads, thread B reports a fork event which stays
> +# pending
> +#
> +# A buggy GDB / GDBserver combo would notice the thread of the child process
> +# of the (still unprocessed) fork event, and erroneously create a new inferior
> +# for it. Once fixed, the child process' thread is hidden by whoever holds the
> +# pending fork event.
> +
> +standard_testfile
> +
> +proc do_test { target-non-stop who_forks fork_function } {
> +
> + set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
> +
> + # WHO_FORKS says which of the main or other thread calls (v)fork. The
> + # thread that does not call (v)fork is the one who tries to step.
> + if { $who_forks == "main" } {
> + lappend opts "additional_flags=-DMAIN_THREAD_FORKS"
> + set this_binfile ${::binfile}-main-${fork_function}
> + } elseif { $who_forks == "other" } {
> + lappend opts "additional_flags=-DOTHER_THREAD_FORKS"
> + set this_binfile ${::binfile}-other-${fork_function}
> + } else {
> + error "invalid who_forks value: $who_forks"
> + }
> +
> + if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
> + return
> + }
> +
> + save_vars { ::GDBFLAGS } {
> + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
> + clean_restart $this_binfile
> + }
> +
> + if {![runto_main]} {
> + fail "could not run to main"
> + return
> + }
> +
> + # Run until breakpoint in the second thread.
> + gdb_test "break break_here" "Breakpoint $::decimal.*"
> + gdb_continue_to_breakpoint "thread started"
> +
> + # Delete the breakpoint so the thread doesn't do a step-over.
> + delete_breakpoints
> +
> + # Let the forking thread make progress during the step.
> + gdb_test "p release_forking_thread = 1" " = 1"
> +
> + # stepi the non-forking thread.
> + gdb_test "stepi"
> +
> + # Make sure there's still a single inferior.
> + gdb_test "info inferior" {\* 1 [^\r\n]+}
> +}
> +
> +foreach_with_prefix target-non-stop { auto on off } {
> + foreach_with_prefix who_forks { main other } {
> + foreach_with_prefix fork_function { fork vfork } {
> + do_test ${target-non-stop} $who_forks $fork_function
> + }
> + }
> +}
> diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
> index 7c293b1f89d2..9f9f69eb1ddd 100644
> --- a/gdbserver/gdbthread.h
> +++ b/gdbserver/gdbthread.h
> @@ -80,6 +80,15 @@ struct thread_info
>
> /* Branch trace target information for this thread. */
> struct btrace_target_info *btrace = nullptr;
> +
> + /* A pointer to this thread's fork child or fork parent. Valid only while
> + the parent fork or vfork event is not reported to GDB.
> +
> + Used to avoid wildcard vCont actions resuming a (v)fork child before GDB is
> + notified about the parent's (v)fork event. Also used to avoid including the
> + (v)fork child in thread list packet responses (e.g. qfThreadInfo). */
> + thread_info *fork_child = nullptr;
> + thread_info *fork_parent = nullptr;
> };
>
> extern std::list<thread_info *> all_threads;
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 34ede238d19f..ff001475a9f0 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -564,10 +564,10 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
> event_lwp->status_pending_p = 1;
> event_lwp->status_pending = wstat;
>
> - /* Link the threads until the parent event is passed on to
> - higher layers. */
> - event_lwp->fork_relative = child_lwp;
> - child_lwp->fork_relative = event_lwp;
> + /* Link the threads until the parent's event is passed on to
> + GDB. */
> + event_lwp->thread->fork_child = child_lwp->thread;
> + child_lwp->thread->fork_parent = event_lwp->thread;
>
> /* If the parent thread is doing step-over with single-step
> breakpoints, the list of single-step breakpoints are cloned
> @@ -3594,8 +3594,10 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
> if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED
> || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED)
> {
> - event_child->fork_relative->fork_relative = NULL;
> - event_child->fork_relative = NULL;
> + gdb_assert (event_child->thread->fork_child != nullptr);
> + gdb_assert (event_child->thread->fork_child->fork_parent != nullptr);
> + event_child->thread->fork_child->fork_parent = nullptr;
> + event_child->thread->fork_child = nullptr;
> }
>
> *ourstatus = event_child->waitstatus;
> @@ -4375,19 +4377,19 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n)
>
> /* Don't let wildcard resumes resume fork children that GDB
> does not yet know are new fork children. */
> - if (lwp->fork_relative != NULL)
> + if (lwp->thread->fork_parent != nullptr)
> {
> - struct lwp_info *rel = lwp->fork_relative;
> + lwp_info *parent = get_thread_lwp (lwp->thread->fork_parent);
> + target_waitkind kind = parent->waitstatus.kind ();
>
> - if (rel->status_pending_p
> - && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED
> - || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED))
> - {
> - if (debug_threads)
> - debug_printf ("not resuming LWP %ld: has queued stop reply\n",
> - lwpid_of (thread));
> - continue;
> - }
> + gdb_assert (parent->status_pending_p);
> + gdb_assert (kind == TARGET_WAITKIND_FORKED
> + || kind == TARGET_WAITKIND_VFORKED);
> +
> + if (debug_threads)
> + debug_printf ("not resuming LWP %ld: is a fork child not know to GDB\n",
> + lwpid_of (thread));
> + continue;
> }
>
> /* If the thread has a pending event that has already been
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 05067ffc6e6f..46aff0a29524 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -756,12 +756,6 @@ struct lwp_info
> information or exit status until it can be reported to GDB. */
> struct target_waitstatus waitstatus;
>
> - /* A pointer to the fork child/parent relative. Valid only while
> - the parent fork event is not reported to higher layers. Used to
> - avoid wildcard vCont actions resuming a fork child before GDB is
> - notified about the parent's fork event. */
> - struct lwp_info *fork_relative = nullptr;
> -
> /* When stopped is set, this is where the lwp last stopped, with
> decr_pc_after_break already accounted for. If the LWP is
> running, this is the address at which the lwp was resumed. */
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index c58f7e01d8da..9890d6cc9798 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1656,6 +1656,12 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer)
> gdb_byte *handle;
> bool handle_status = target_thread_handle (ptid, &handle, &handle_len);
>
> + /* If this is a fork or vfork child (has a fork parent), GDB does not yet
> + know about this process, and must not know about it until it gets the
> + corresponding (v)fork event. Exclude this thread from the list. */
> + if (thread->fork_parent != nullptr)
> + return;
> +
> write_ptid (ptid_s, ptid);
>
> buffer_xml_printf (buffer, "<thread id=\"%s\"", ptid_s);
> --
> 2.33.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 0/3] Fix handling of pending fork events
2021-10-29 20:33 [PATCH 1/2] gdbserver: hide fork child threads from GDB Simon Marchi
2021-10-29 20:33 ` [PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
2021-11-12 20:54 ` [PATCH 1/2] gdbserver: hide fork child threads from GDB (ping) Simon Marchi
@ 2021-11-24 20:04 ` Simon Marchi
2021-11-24 20:04 ` [PATCH 1/3] gdbserver: hide fork child threads from GDB Simon Marchi
` (4 more replies)
2 siblings, 5 replies; 15+ messages in thread
From: Simon Marchi @ 2021-11-24 20:04 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
This is v2 of:
https://sourceware.org/pipermail/gdb-patches/2021-October/182922.html
Patch 2/3 is new, it factors out some code in the function, to keep the
following patch simple.
Changes in patch 3/3 (patch 2/2 in v1) are:
- The test did fail on some machines (not my development machine
apparently). It is because I only handled pending statuses in
lwp_info::status, in the linux-nat target. There may also be some
pending statuses in lwp_info::waitstatus, handle them too.
- Rather than adding a target method to allow the core to detach
pending fork children, make the targets (linux-nat and remote) peek
in thread_info::pending_waitstatus to see if there is one there.
Simon Marchi (3):
gdbserver: hide fork child threads from GDB
gdb/linux-nat: factor ptrace-detach code to new detach_one_pid
function
gdb, gdbserver: detach fork child when detaching from fork parent
gdb/linux-nat.c | 124 ++++++++++-----
gdb/remote.c | 14 ++
.../pending-fork-event-touch-file.c | 26 +++
.../gdb.threads/pending-fork-event.c | 82 ++++++++++
.../gdb.threads/pending-fork-event.exp | 150 ++++++++++++++++++
gdbserver/gdbthread.h | 9 ++
gdbserver/linux-low.cc | 36 +++--
gdbserver/linux-low.h | 6 -
gdbserver/server.cc | 35 ++++
9 files changed, 422 insertions(+), 60 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] gdbserver: hide fork child threads from GDB
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
@ 2021-11-24 20:04 ` Simon Marchi
2021-11-26 22:51 ` Pedro Alves
2021-11-24 20:04 ` [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2021-11-24 20:04 UTC (permalink / raw)
To: gdb-patches
From: Simon Marchi <simon.marchi@polymtl.ca>
This patch aims at fixing a bug where an inferior is unexpectedly
created when a fork happens at the same time as another event, and that
other event is reported to GDB first (and the fork event stays pending
in GDBserver). This happens for example when we step a thread and
another thread forks at the same time. The bug looks like (if I
reproduce the included test by hand):
(gdb) show detach-on-fork
Whether gdb will detach the child of a fork is on.
(gdb) show follow-fork-mode
Debugger response to a program call of fork or vfork is "parent".
(gdb) si
[New inferior 2]
Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
Reading /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread from remote target...
Reading symbols from target:/home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-while-fork-in-other-thread/step-while-fork-in-other-thread...
[New Thread 965190.965190]
[Switching to Thread 965190.965190]
Remote 'g' packet reply is too long (expected 560 bytes, got 816 bytes): ... <long series of bytes>
The sequence of events leading to the problem is:
- We are using the all-stop user-visible mode as well as the
synchronous / all-stop variant of the remote protocol
- We have two threads, thread A that we single-step and thread B that
calls fork at the same time
- GDBserver's linux_process_target::wait pulls the "single step
complete SIGTRAP" and the "fork" events from the kernel. It
arbitrarily choses one event to report, it happens to be the
single-step SIGTRAP. The fork stays pending in the thread_info.
- GDBserver send that SIGTRAP as a stop reply to GDB
- While in stop_all_threads, GDB calls update_thread_list, which ends
up querying the remote thread list using qXfer:threads:read.
- In the reply, GDBserver includes the fork child created as a result
of thread B's fork.
- GDB-side, the remote target sees the new PID, calls
remote_notice_new_inferior, which ends up unexpectedly creating a new
inferior, and things go downhill from there.
The problem here is that as long as GDB did not process the fork event,
it should pretend the fork child does not exist. Ultimately, this event
will be reported, we'll go through follow_fork, and that process will be
detached.
The remote target (GDB-side), has some code to remove from the reported
thread list the threads that are the result of forks not processed by
GDB yet. But that only works for fork events that have made their way
to the remote target (GDB-side), but haven't been consumed by the core
yet, so are still lingering as pending stop replies in the remote target
(see remove_new_fork_children in remote.c). But in our case, the fork
event hasn't made its way to the GDB-side remote target. We need to
implement the same kind of logic GDBserver-side: if there exists a
thread / inferior that is the result of a fork event GDBserver hasn't
reported yet, it should exclude that thread / inferior from the reported
thread list.
This was actually discussed a while ago, but not implemented AFAIK:
https://pi.simark.ca/gdb-patches/1ad9f5a8-d00e-9a26-b0c9-3f4066af5142@redhat.com/#t
https://sourceware.org/pipermail/gdb-patches/2016-June/133906.html
Implementation details-wise, the fix for this is all in GDBserver. The
Linux layer of GDBserver already tracks unreported fork parent / child
relationships using the lwp_info::fork_relative, in order to avoid
wildcard actions resuming fork childs unknown to GDB. Move this
information to the common thread_info structure, so that it can be used
in handle_qxfer_threads_worker to filter out unreported fork child
threads. Plus, that makes it usable for other OSes that use fork if
need be.
I split the field in two (fork_parent / fork_child), I think it's
clearer this way, easier to follow which thread is the parent and which
is the child, and helps ensure things are consistent. That simplifies
things a bit in linux_set_resume_request. Currently, when
lwp->fork_relative is set, we have to deduce whether this is a parent or
child using the pending event. With separate fork_parent and fork_child
fields, it becomes more obvious. If the thread has a fork parent, then
it means it's a fork child, and vice-versa.
Testing-wise, the test replicates pretty-much the sequence of events
shown above. The setup of the test makes it such that the main thread
is about to fork. We stepi the other thread, so that the step completes
very quickly, in a single event. Meanwhile, the main thread is resumed,
so very likely has time to call fork. This means that the bug may not
reproduce every time (if the main thread does not have time to call
fork), but it will reproduce more often than not. The test fails
without the fix applied on the native-gdbserver and
native-extended-gdbserver boards.
At some point I suspected that which thread called fork and which thread
did the step influenced the order in which the events were reported, and
therefore the reproducibility of the bug. So I made the test try both
combinations: main thread forks while other thread steps, and vice
versa. I'm not sure this is still necessary, but I left it there
anyway. It doesn't hurt to test a few more combinations.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28288
Change-Id: I2158d5732fc7d7ca06b0eb01f88cf27bf527b990
---
.../gdb.threads/pending-fork-event.c | 82 ++++++++++++++++++
.../gdb.threads/pending-fork-event.exp | 86 +++++++++++++++++++
gdbserver/gdbthread.h | 9 ++
gdbserver/linux-low.cc | 36 ++++----
gdbserver/linux-low.h | 6 --
gdbserver/server.cc | 6 ++
6 files changed, 202 insertions(+), 23 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
new file mode 100644
index 000000000000..a39ca75a49ac
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
@@ -0,0 +1,82 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 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 <assert.h>
+
+static volatile int release_forking_thread = 0;
+static int x;
+static pthread_barrier_t barrier;
+
+static void
+break_here (void)
+{
+ x++;
+}
+
+static void
+do_fork (void)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (!release_forking_thread);
+
+ if (FORK_FUNCTION () == 0)
+ _exit (0);
+
+}
+
+static void *
+thread_func (void *p)
+{
+#if defined(MAIN_THREAD_FORKS)
+ break_here ();
+#elif defined(OTHER_THREAD_FORKS)
+ do_fork ();
+#else
+# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
+#endif
+}
+
+
+int
+main (void)
+{
+ pthread_t thread;
+ int ret;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ alarm (30);
+ ret = pthread_create (&thread, NULL, thread_func, NULL);
+ assert (ret == 0);
+
+ pthread_barrier_wait (&barrier);
+
+#if defined(MAIN_THREAD_FORKS)
+ do_fork ();
+#elif defined(OTHER_THREAD_FORKS)
+ break_here ();
+#else
+# error "MAIN_THREAD_FORKS or OTHER_THREAD_FORKS must be defined"
+#endif
+
+ pthread_join (thread, NULL);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
new file mode 100644
index 000000000000..6574b9abf5b7
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -0,0 +1,86 @@
+# Copyright (C) 2021 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 we handle well, in all-stop, a fork happening in a thread B while
+# doing a step-like command in a thread A.
+#
+# The scenario we want to test for is:
+#
+# - the step of thread A completes, we handle the event and stop all threads
+# as a result
+# - while stopping all threads, thread B reports a fork event which stays
+# pending
+#
+# A buggy GDB / GDBserver combo would notice the thread of the child process
+# of the (still unprocessed) fork event, and erroneously create a new inferior
+# for it. Once fixed, the child process' thread is hidden by whoever holds the
+# pending fork event.
+
+standard_testfile
+
+proc do_test { target-non-stop who_forks fork_function } {
+
+ set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
+
+ # WHO_FORKS says which of the main or other thread calls (v)fork. The
+ # thread that does not call (v)fork is the one who tries to step.
+ if { $who_forks == "main" } {
+ lappend opts "additional_flags=-DMAIN_THREAD_FORKS"
+ set this_binfile ${::binfile}-main-${fork_function}
+ } elseif { $who_forks == "other" } {
+ lappend opts "additional_flags=-DOTHER_THREAD_FORKS"
+ set this_binfile ${::binfile}-other-${fork_function}
+ } else {
+ error "invalid who_forks value: $who_forks"
+ }
+
+ if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
+ return
+ }
+
+ save_vars { ::GDBFLAGS } {
+ append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
+ clean_restart $this_binfile
+ }
+
+ if {![runto_main]} {
+ fail "could not run to main"
+ return
+ }
+
+ # Run until breakpoint in the second thread.
+ gdb_test "break break_here" "Breakpoint $::decimal.*"
+ gdb_continue_to_breakpoint "thread started"
+
+ # Delete the breakpoint so the thread doesn't do a step-over.
+ delete_breakpoints
+
+ # Let the forking thread make progress during the step.
+ gdb_test "p release_forking_thread = 1" " = 1"
+
+ # stepi the non-forking thread.
+ gdb_test "stepi"
+
+ # Make sure there's still a single inferior.
+ gdb_test "info inferior" {\* 1 [^\r\n]+}
+}
+
+foreach_with_prefix target-non-stop { auto on off } {
+ foreach_with_prefix who_forks { main other } {
+ foreach_with_prefix fork_function { fork vfork } {
+ do_test ${target-non-stop} $who_forks $fork_function
+ }
+ }
+}
diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h
index 7c293b1f89d2..9f9f69eb1ddd 100644
--- a/gdbserver/gdbthread.h
+++ b/gdbserver/gdbthread.h
@@ -80,6 +80,15 @@ struct thread_info
/* Branch trace target information for this thread. */
struct btrace_target_info *btrace = nullptr;
+
+ /* A pointer to this thread's fork child or fork parent. Valid only while
+ the parent fork or vfork event is not reported to GDB.
+
+ Used to avoid wildcard vCont actions resuming a (v)fork child before GDB is
+ notified about the parent's (v)fork event. Also used to avoid including the
+ (v)fork child in thread list packet responses (e.g. qfThreadInfo). */
+ thread_info *fork_child = nullptr;
+ thread_info *fork_parent = nullptr;
};
extern std::list<thread_info *> all_threads;
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d214aff7051e..7e87bdd8643c 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -564,10 +564,10 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
event_lwp->status_pending_p = 1;
event_lwp->status_pending = wstat;
- /* Link the threads until the parent event is passed on to
- higher layers. */
- event_lwp->fork_relative = child_lwp;
- child_lwp->fork_relative = event_lwp;
+ /* Link the threads until the parent's event is passed on to
+ GDB. */
+ event_lwp->thread->fork_child = child_lwp->thread;
+ child_lwp->thread->fork_parent = event_lwp->thread;
/* If the parent thread is doing step-over with single-step
breakpoints, the list of single-step breakpoints are cloned
@@ -3590,8 +3590,10 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED
|| event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED)
{
- event_child->fork_relative->fork_relative = NULL;
- event_child->fork_relative = NULL;
+ gdb_assert (event_child->thread->fork_child != nullptr);
+ gdb_assert (event_child->thread->fork_child->fork_parent != nullptr);
+ event_child->thread->fork_child->fork_parent = nullptr;
+ event_child->thread->fork_child = nullptr;
}
*ourstatus = event_child->waitstatus;
@@ -4371,19 +4373,19 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n)
/* Don't let wildcard resumes resume fork children that GDB
does not yet know are new fork children. */
- if (lwp->fork_relative != NULL)
+ if (lwp->thread->fork_parent != nullptr)
{
- struct lwp_info *rel = lwp->fork_relative;
+ lwp_info *parent = get_thread_lwp (lwp->thread->fork_parent);
+ target_waitkind kind = parent->waitstatus.kind ();
- if (rel->status_pending_p
- && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED
- || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED))
- {
- if (debug_threads)
- debug_printf ("not resuming LWP %ld: has queued stop reply\n",
- lwpid_of (thread));
- continue;
- }
+ gdb_assert (parent->status_pending_p);
+ gdb_assert (kind == TARGET_WAITKIND_FORKED
+ || kind == TARGET_WAITKIND_VFORKED);
+
+ if (debug_threads)
+ debug_printf ("not resuming LWP %ld: is a fork child not know to GDB\n",
+ lwpid_of (thread));
+ continue;
}
/* If the thread has a pending event that has already been
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 05067ffc6e6f..46aff0a29524 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -756,12 +756,6 @@ struct lwp_info
information or exit status until it can be reported to GDB. */
struct target_waitstatus waitstatus;
- /* A pointer to the fork child/parent relative. Valid only while
- the parent fork event is not reported to higher layers. Used to
- avoid wildcard vCont actions resuming a fork child before GDB is
- notified about the parent's fork event. */
- struct lwp_info *fork_relative = nullptr;
-
/* When stopped is set, this is where the lwp last stopped, with
decr_pc_after_break already accounted for. If the LWP is
running, this is the address at which the lwp was resumed. */
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b1d4c92b3d97..7963f2faf267 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1656,6 +1656,12 @@ handle_qxfer_threads_worker (thread_info *thread, struct buffer *buffer)
gdb_byte *handle;
bool handle_status = target_thread_handle (ptid, &handle, &handle_len);
+ /* If this is a fork or vfork child (has a fork parent), GDB does not yet
+ know about this process, and must not know about it until it gets the
+ corresponding (v)fork event. Exclude this thread from the list. */
+ if (thread->fork_parent != nullptr)
+ return;
+
write_ptid (ptid_s, ptid);
buffer_xml_printf (buffer, "<thread id=\"%s\"", ptid_s);
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2021-11-24 20:04 ` [PATCH 1/3] gdbserver: hide fork child threads from GDB Simon Marchi
@ 2021-11-24 20:04 ` Simon Marchi
2021-11-26 22:51 ` Pedro Alves
2021-11-24 20:04 ` [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2021-11-24 20:04 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
The following patch will add some code paths that need to ptrace-detach
a given PID. Factor out the code that does this and put it in its own
function, so that it can be re-used.
Change-Id: Ie65ca0d89893b41aea0a23d9fc6ffbed042a9705
---
gdb/linux-nat.c | 76 ++++++++++++++++++++++++++-----------------------
1 file changed, 40 insertions(+), 36 deletions(-)
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f8f728481ea7..85ae68d8a6fa 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1234,6 +1234,45 @@ linux_nat_target::attach (const char *args, int from_tty)
target_async (1);
}
+/* Ptrace-detach the thread with pid PID. */
+
+static void
+detach_one_pid (int pid, int signo)
+{
+ if (ptrace (PTRACE_DETACH, pid, 0, signo) < 0)
+ {
+ int save_errno = errno;
+
+ /* We know the thread exists, so ESRCH must mean the lwp is
+ zombie. This can happen if one of the already-detached
+ threads exits the whole thread group. In that case we're
+ still attached, and must reap the lwp. */
+ if (save_errno == ESRCH)
+ {
+ int ret, status;
+
+ ret = my_waitpid (pid, &status, __WALL);
+ if (ret == -1)
+ {
+ warning (_("Couldn't reap LWP %d while detaching: %s"),
+ pid, safe_strerror (errno));
+ }
+ else if (!WIFEXITED (status) && !WIFSIGNALED (status))
+ {
+ warning (_("Reaping LWP %d while detaching "
+ "returned unexpected status 0x%x"),
+ pid, status);
+ }
+ }
+ else
+ error (_("Can't detach %d: %s"),
+ pid, safe_strerror (save_errno));
+ }
+ else
+ linux_nat_debug_printf ("PTRACE_DETACH (%d, %s, 0) (OK)",
+ pid, strsignal (signo));
+}
+
/* Get pending signal of THREAD as a host signal number, for detaching
purposes. This is the signal the thread last stopped for, which we
need to deliver to the thread when detaching, otherwise, it'd be
@@ -1364,42 +1403,7 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
throw;
}
- if (ptrace (PTRACE_DETACH, lwpid, 0, signo) < 0)
- {
- int save_errno = errno;
-
- /* We know the thread exists, so ESRCH must mean the lwp is
- zombie. This can happen if one of the already-detached
- threads exits the whole thread group. In that case we're
- still attached, and must reap the lwp. */
- if (save_errno == ESRCH)
- {
- int ret, status;
-
- ret = my_waitpid (lwpid, &status, __WALL);
- if (ret == -1)
- {
- warning (_("Couldn't reap LWP %d while detaching: %s"),
- lwpid, safe_strerror (errno));
- }
- else if (!WIFEXITED (status) && !WIFSIGNALED (status))
- {
- warning (_("Reaping LWP %d while detaching "
- "returned unexpected status 0x%x"),
- lwpid, status);
- }
- }
- else
- {
- error (_("Can't detach %s: %s"),
- target_pid_to_str (lp->ptid).c_str (),
- safe_strerror (save_errno));
- }
- }
- else
- linux_nat_debug_printf ("PTRACE_DETACH (%s, %s, 0) (OK)",
- target_pid_to_str (lp->ptid).c_str (),
- strsignal (signo));
+ detach_one_pid (lwpid, signo);
delete_lwp (lp->ptid);
}
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2021-11-24 20:04 ` [PATCH 1/3] gdbserver: hide fork child threads from GDB Simon Marchi
2021-11-24 20:04 ` [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
@ 2021-11-24 20:04 ` Simon Marchi
2021-11-26 22:54 ` Pedro Alves
2021-11-24 20:56 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2021-11-26 22:50 ` Pedro Alves
4 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2021-11-24 20:04 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
While working with pending fork events, I wondered what would happen if
the user detached an inferior while a thread of that inferior had a
pending fork event. What happens with the fork child, which is
ptrace-attached by the GDB process (or by GDBserver), but not known to
the core? Sure enough, neither the core of GDB or the target detach the
child process, so GDB (or GDBserver) just stays ptrace-attached to the
process. The result is that the fork child process is stuck, while you
would expect it to be detached and run.
Make GDBserver detach of fork children it knows about. That is done in
the generic handle_detach function. Since a process_info already exists
for the child, we can simply call detach_inferior on it.
GDB-side, make the linux-nat and remote targets detach of fork children
known because of pending fork events. These pending fork events can be
stored in thread_info::pending_waitstatus, if the core has consumed the
event but then saved it for later. They can also be stored, for the
linux-nat target, in lwp_info::status and lwp_info::waitstatus. For the
remote target, I suppose that a pending fork event could maybe be in the
stop reply queue, but I could generate a case where would could detach
while having pending events there, so I didn't handle that case in this
patch.
Update the gdb.threads/pending-fork-event.exp to try to detach the
process while a thread has a pending fork event. In order to verify
that the fork child process is correctly detached and resumes execution
outside of GDB's control, make that process create a file in the test
output directory, and make the test wait $timeout seconds for that file
to appear (it happens instantly if everything goes well).
The test catches a bug in linux-nat.c, also reported as PR 28512
("waitstatus.h:300: internal-error: gdb_signal target_waitstatus::sig()
const: Assertion `m_kind == TARGET_WAITKIND_STOPPED || m_kind ==
TARGET_WAITKIND_SIGNALLED' failed.). When detaching a thread with a
pending event, get_detach_signal unconditionally fetches the signal
stored in the waitstatus (`tp->pending_waitstatus ().sig ()`). However,
that is only valid if the pending event is of type
TARGET_WAITKIND_STOPPED, and this is now enforced using assertions (iit
would also be valid for TARGET_WAITKIND_SIGNALLED, but that would mean
the thread does not exist anymore, so we wouldn't be detaching it). Add
a condition in get_detach_signal to access the signal number only if the
wait status is of kind TARGET_WAITKIND_STOPPED, and use GDB_SIGNAL_0
instead (since the thread was not stopped with a signal to begin with).
There is a known limitation: we don't remove breakpoints from the
children before detaching it. So the children, could hit a trap
instruction after being detached and crash. I know this is wrong, and
it should be fixed, but I would like to handle that later. The current
patch doesn't fix everything, but it's a step in the right direction.
Change-Id: I6d811a56f520e3cb92d5ea563ad38976f92e93dd
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28512
---
gdb/linux-nat.c | 48 +++++++++++-
gdb/remote.c | 14 ++++
.../pending-fork-event-touch-file.c | 26 +++++++
.../gdb.threads/pending-fork-event.c | 6 +-
.../gdb.threads/pending-fork-event.exp | 76 +++++++++++++++++--
gdbserver/server.cc | 29 +++++++
6 files changed, 189 insertions(+), 10 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 85ae68d8a6fa..b9e09e4fc0ce 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1315,7 +1315,16 @@ get_detach_signal (struct lwp_info *lp)
if (target_is_non_stop_p () && !tp->executing ())
{
if (tp->has_pending_waitstatus ())
- signo = tp->pending_waitstatus ().sig ();
+ {
+ /* If the thread has a pending event, and it was stopped with a
+ signal, use that signal to resume it. If it has a pending
+ event of another kind, it was not stopped with a signal, so
+ resume it without a signal. */
+ if (tp->pending_waitstatus ().kind () == TARGET_WAITKIND_STOPPED)
+ signo = tp->pending_waitstatus ().sig ();
+ else
+ signo = GDB_SIGNAL_0;
+ }
else
signo = tp->stop_signal ();
}
@@ -1367,6 +1376,43 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
+ /* If the lwp/thread we are about to detach has a pending fork event,
+ there is a process GDB is attached to that the core of GDB doesn't know
+ about. Detach from it. */
+
+ /* Check in lwp_info::status. */
+ if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
+ {
+ int event = linux_ptrace_get_extended_event (lp->status);
+
+ if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
+ {
+ unsigned long child_pid;
+ int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
+ if (ret == 0)
+ detach_one_pid (child_pid, 0);
+ else
+ perror_warning_with_name (_("Failed to detach fork child"));
+ }
+ }
+
+ /* Check in lwp_info::waitstatus. */
+ if (lp->waitstatus.kind () == TARGET_WAITKIND_VFORKED
+ || lp->waitstatus.kind () == TARGET_WAITKIND_FORKED)
+ detach_one_pid (lp->waitstatus.child_ptid ().pid (), 0);
+
+
+ /* Check in thread_info::pending_waitstatus. */
+ thread_info *tp = find_thread_ptid (linux_target, lp->ptid);
+ if (tp->has_pending_waitstatus ())
+ {
+ const target_waitstatus &ws = tp->pending_waitstatus ();
+
+ if (ws.kind () == TARGET_WAITKIND_VFORKED
+ || ws.kind () == TARGET_WAITKIND_FORKED)
+ detach_one_pid (ws.child_ptid ().pid (), 0);
+ }
+
if (lp->status != 0)
linux_nat_debug_printf ("Pending %s for %s on detach.",
strsignal (WSTOPSIG (lp->status)),
diff --git a/gdb/remote.c b/gdb/remote.c
index 724386e09164..677d54ccd5c9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5876,6 +5876,20 @@ remote_target::remote_detach_1 (inferior *inf, int from_tty)
if (from_tty && !rs->extended && number_of_live_inferiors (this) == 1)
puts_filtered (_("Ending remote debugging.\n"));
+ /* See if any thread of the inferior we are detaching has a pending fork
+ status. In that case, we must detach from the child resulting from
+ that fork. */
+ for (thread_info *thread : inf->non_exited_threads ())
+ {
+ if (!thread->has_pending_waitstatus ())
+ continue;
+
+ const target_waitstatus &ws = thread->pending_waitstatus ();
+ if (ws.kind () == TARGET_WAITKIND_FORKED
+ || ws.kind () == TARGET_WAITKIND_VFORKED)
+ remote_detach_pid (ws.child_ptid ().pid ());
+ }
+
thread_info *tp = find_thread_ptid (this, inferior_ptid);
/* Check to see if we are detaching a fork parent. Note that if we
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
new file mode 100644
index 000000000000..5536381847b1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+
+int
+main (void)
+{
+ FILE *f = fopen (TOUCH_FILE_PATH, "w");
+ fclose (f);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
index a39ca75a49ac..bab4a99e5786 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.c
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
@@ -32,18 +32,18 @@ break_here (void)
static void
do_fork (void)
{
- pthread_barrier_wait (&barrier);
-
while (!release_forking_thread);
if (FORK_FUNCTION () == 0)
- _exit (0);
+ execl (TOUCH_FILE_BIN, TOUCH_FILE_BIN, NULL);
}
static void *
thread_func (void *p)
{
+ pthread_barrier_wait (&barrier);
+
#if defined(MAIN_THREAD_FORKS)
break_here ();
#elif defined(OTHER_THREAD_FORKS)
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
index 6574b9abf5b7..cbe55c2f5e7c 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.exp
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -28,11 +28,32 @@
# for it. Once fixed, the child process' thread is hidden by whoever holds the
# pending fork event.
-standard_testfile
+standard_testfile .c -touch-file.c
-proc do_test { target-non-stop who_forks fork_function } {
+set touch_file_bin $binfile-touch-file
+set touch_file_path [standard_output_file some-file]
- set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
+set opts [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]
+if { [gdb_compile "$srcdir/$subdir/$srcfile2" $touch_file_bin executable $opts] != "" } {
+ return 0
+}
+
+# Run until execution is stopped and a thread has a pending fork event.
+#
+# WHO_FORKS tells which of the thread program threads does the fork ("main" or
+# "other").
+#
+# FORK_FUNCTION tells which function to call to fork ("fork" or "vfork").
+#
+# Return 1 on success, 0 otherwise.
+
+proc setup { target-non-stop who_forks fork_function } {
+ set this_binfile ${::binfile}-${fork_function}
+
+ set opts [list \
+ debug \
+ "additional_flags=-DFORK_FUNCTION=$fork_function" \
+ "additional_flags=-DTOUCH_FILE_BIN=\"$::touch_file_bin\""]
# WHO_FORKS says which of the main or other thread calls (v)fork. The
# thread that does not call (v)fork is the one who tries to step.
@@ -47,7 +68,7 @@ proc do_test { target-non-stop who_forks fork_function } {
}
if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
- return
+ return 0
}
save_vars { ::GDBFLAGS } {
@@ -57,7 +78,7 @@ proc do_test { target-non-stop who_forks fork_function } {
if {![runto_main]} {
fail "could not run to main"
- return
+ return 0
}
# Run until breakpoint in the second thread.
@@ -75,12 +96,55 @@ proc do_test { target-non-stop who_forks fork_function } {
# Make sure there's still a single inferior.
gdb_test "info inferior" {\* 1 [^\r\n]+}
+
+ return 1
+}
+
+# Return 1 if file ::TOUCH_FILE_PATH exists, else 0.
+
+proc file_exists {} {
+ return [file exists $::touch_file_path]
+}
+
+# Wait up to ::TIMEOUT seconds for file ::TOUCH_FILE_PATH to exist. Return
+# 1 if it does exist, 0 otherwise.
+
+proc file_exists_with_timeout {} {
+ for {set i 0} {$i < $::timeout} {incr i} {
+ if { [file_exists] } {
+ return 1
+ }
+
+ sleep 1
+ }
+
+ return 0
+}
+
+# Run until there exists (at least, likely exists) a pending fork event.
+# Detach the fork parent. Verify that the fork child (which does not appear in
+# GDB, since the fork event is pending) is detached as well.
+
+proc_with_prefix do_test_detach { target-non-stop who_forks fork_function } {
+ file delete $::touch_file_path
+ gdb_assert { ![file_exists] } "file does not exist before test"
+
+ if { ![setup ${target-non-stop} $who_forks $fork_function] } {
+ return
+ }
+
+ gdb_test "detach"
+
+ # After being detached, the fork child creates file ::TOUCH_FILE_PATH.
+ # Seeing this file tells us the fork child was detached and executed
+ # successfully.
+ gdb_assert { [file_exists_with_timeout] } "file exists after detach"
}
foreach_with_prefix target-non-stop { auto on off } {
foreach_with_prefix who_forks { main other } {
foreach_with_prefix fork_function { fork vfork } {
- do_test ${target-non-stop} $who_forks $fork_function
+ do_test_detach ${target-non-stop} $who_forks $fork_function
}
}
}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 7963f2faf267..2f3d5341758f 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
/* We'll need this after PROCESS has been destroyed. */
int pid = process->pid;
+ /* If this process has an unreported fork child, that child is not known to
+ GDB, so detach it as well.
+
+ Here, we specifically don't want to use "safe iteration", as detaching
+ another process might delete the next thread in the iteration, which is
+ the one saved by the safe iterator. We will never delete the currently
+ iterated on thread, so standard iteration should be safe. */
+ for (thread_info *thread : all_threads)
+ {
+ /* Only threads that are of the process we are detaching. */
+ if (thread->id.pid () != pid)
+ continue;
+
+ /* Only threads that have a pending fork event. */
+ if (thread->fork_child == nullptr)
+ continue;
+
+ process_info *fork_child_process
+ = find_process_pid (thread->fork_child->id.pid ());
+ gdb_assert (fork_child_process != nullptr);
+
+ int fork_child_pid = fork_child_process->pid;
+
+ if (detach_inferior (fork_child_process) != 0)
+ warning (_("Failed to detach fork child %s, child of %s"),
+ target_pid_to_str (ptid_t (fork_child_pid)).c_str (),
+ target_pid_to_str (thread->id).c_str ());
+ }
+
if (detach_inferior (process) != 0)
write_enn (own_buf);
else
--
2.26.2
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Fix handling of pending fork events
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
` (2 preceding siblings ...)
2021-11-24 20:04 ` [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
@ 2021-11-24 20:56 ` Simon Marchi
2021-11-26 22:50 ` Pedro Alves
4 siblings, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2021-11-24 20:56 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Aaaaaand I forgot to use v2 in the subject :(.
On 2021-11-24 3:04 p.m., Simon Marchi via Gdb-patches wrote:
> This is v2 of:
>
> https://sourceware.org/pipermail/gdb-patches/2021-October/182922.html
>
> Patch 2/3 is new, it factors out some code in the function, to keep the
> following patch simple.
>
> Changes in patch 3/3 (patch 2/2 in v1) are:
>
> - The test did fail on some machines (not my development machine
> apparently). It is because I only handled pending statuses in
> lwp_info::status, in the linux-nat target. There may also be some
> pending statuses in lwp_info::waitstatus, handle them too.
> - Rather than adding a target method to allow the core to detach
> pending fork children, make the targets (linux-nat and remote) peek
> in thread_info::pending_waitstatus to see if there is one there.
>
> Simon Marchi (3):
> gdbserver: hide fork child threads from GDB
> gdb/linux-nat: factor ptrace-detach code to new detach_one_pid
> function
> gdb, gdbserver: detach fork child when detaching from fork parent
>
> gdb/linux-nat.c | 124 ++++++++++-----
> gdb/remote.c | 14 ++
> .../pending-fork-event-touch-file.c | 26 +++
> .../gdb.threads/pending-fork-event.c | 82 ++++++++++
> .../gdb.threads/pending-fork-event.exp | 150 ++++++++++++++++++
> gdbserver/gdbthread.h | 9 ++
> gdbserver/linux-low.cc | 36 +++--
> gdbserver/linux-low.h | 6 -
> gdbserver/server.cc | 35 ++++
> 9 files changed, 422 insertions(+), 60 deletions(-)
> create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
> create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.c
> create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Fix handling of pending fork events
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
` (3 preceding siblings ...)
2021-11-24 20:56 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
@ 2021-11-26 22:50 ` Pedro Alves
4 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2021-11-26 22:50 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote:
> This is v2 of:
>
> https://sourceware.org/pipermail/gdb-patches/2021-October/182922.html
>
> Patch 2/3 is new, it factors out some code in the function, to keep the
> following patch simple.
>
> Changes in patch 3/3 (patch 2/2 in v1) are:
>
> - The test did fail on some machines (not my development machine
> apparently). It is because I only handled pending statuses in
> lwp_info::status, in the linux-nat target. There may also be some
> pending statuses in lwp_info::waitstatus, handle them too.
> - Rather than adding a target method to allow the core to detach
> pending fork children, make the targets (linux-nat and remote) peek
> in thread_info::pending_waitstatus to see if there is one there.
Thanks!
FTR, I had been discussing v1 with Simon offlist. The change to drop
the target method helps with some other work I'm doing that coincidentally
ran into the same issues Simon ran into, so I'm basing it on top of this
series. Namely, I'm teaching GDB and GDBserver about a new "clone" event,
similar to fork and vfork events. This allows fixing displaced stepping
over clone syscalls (PR gdb/19675).
I've rebased my clone work on top of v2, and it all seems to work fine. I
have one point that needs to be discussed though. See replies to patches #1
and #3.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdbserver: hide fork child threads from GDB
2021-11-24 20:04 ` [PATCH 1/3] gdbserver: hide fork child threads from GDB Simon Marchi
@ 2021-11-26 22:51 ` Pedro Alves
2021-11-29 12:46 ` Simon Marchi
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2021-11-26 22:51 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote:
> I split the field in two (fork_parent / fork_child), I think it's
> clearer this way, easier to follow which thread is the parent and which
> is the child, and helps ensure things are consistent. That simplifies
> things a bit in linux_set_resume_request.
This had been conscientiously done that way to avoid storing redundant
information. You could have the same thing with a single field + the
waitstatus, wrapped in fork_child()/fork_parent() methods. But I'm fine with
with that you have. I do see a different reason for taking the two-fields approach
though -- you don't have access to the pending waitstatus in common code.
That would be a better rationale, IMO. OTOH, I think we will end up needing
access to the pending waitstatus anyhow. See below, and the review to patch #3.
> Currently, when
> lwp->fork_relative is set, we have to deduce whether this is a parent or
> child using the pending event. With separate fork_parent and fork_child
> fields, it becomes more obvious. If the thread has a fork parent, then
> it means it's a fork child, and vice-versa.
With clone events, in some spots I'll need to be able to know whether
fork_child being set means the thread is stopped for a fork/vfork or a
clone event. Most of the code related to fork_parent/fork_child is the
same for forks and for clones, but not all.
Wonder whether we should instead have some:
thread_info *target_pending_child (thread_info *parent);
thread_info *target_pending_child_parent (thread_info *child);
target_waitkind target_pending_child_kind (thread_info *parent);
target methods instead of moving the fields. I haven't thought this
fully through, though.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function
2021-11-24 20:04 ` [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
@ 2021-11-26 22:51 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2021-11-26 22:51 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
OK.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent
2021-11-24 20:04 ` [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
@ 2021-11-26 22:54 ` Pedro Alves
2021-11-29 18:27 ` Simon Marchi
0 siblings, 1 reply; 15+ messages in thread
From: Pedro Alves @ 2021-11-26 22:54 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote:
> GDB-side, make the linux-nat and remote targets detach of fork children
> known because of pending fork events. These pending fork events can be
> stored in thread_info::pending_waitstatus, if the core has consumed the
> event but then saved it for later. They can also be stored, for the
> linux-nat target, in lwp_info::status and lwp_info::waitstatus.
...
> For the
> remote target, I suppose that a pending fork event could maybe be in the
> stop reply queue, but I could generate a case where would could detach
> while having pending events there, so I didn't handle that case in this
> patch.
Yes, this can certainly happen. I don't see a way to reliably stop the
program with an event held in the stop reply queue. GDB constantly drains the
events asynchronously. So for a testcase, I think you'd need to take
"constantly hammer" approach. Something like:
- enable non-stop / target non-stop
- make the test program have a few threads constantly fork and exit the child
I say multiple threads so that the stop reply queue has a good chance of
holding an event unprocessed
- make the test program call alarm() to set a timeout. See why below.
- make the testcase constantly attach / continue -a & / detach
Determining whether the fork child was detached correctly would be done by:
- the parent/child sharing a pipe. You'd have an array of pipes, one
entry per thread.
- the child writing to the pipe before exiting
- the parent reading from the pipe after forking
If GDB fails to detach the child, then the parent thread hangs in
the pipe read, and eventually, the parent gets a SIGALRM.
To avoid the case of the test failing, then the parent dying with SIGALRM, and
GDB attaching to the wrong process (because the kernel reused the PID for
something else), you'd make the SIGALRM signal handler set a "timed_out"
global flag, and sleep for a while before exiting the process. Enough time
so that GDB can always reattach before the process disappears due to SIGALRM.
When GDB reattaches, it first always consults that "timed_out" global, to
know whether the detach-from-children succeeded. If it is set, the test
FAILs.
> There is a known limitation: we don't remove breakpoints from the
> children before detaching it. So the children, could hit a trap
> instruction after being detached and crash. I know this is wrong, and
> it should be fixed, but I would like to handle that later. The current
> patch doesn't fix everything, but it's a step in the right direction.
*nod*
> }
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 7963f2faf267..2f3d5341758f 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
> /* We'll need this after PROCESS has been destroyed. */
> int pid = process->pid;
>
> + /* If this process has an unreported fork child, that child is not known to
> + GDB, so detach it as well.
> +
> + Here, we specifically don't want to use "safe iteration", as detaching
> + another process might delete the next thread in the iteration, which is
> + the one saved by the safe iterator. We will never delete the currently
> + iterated on thread, so standard iteration should be safe. */
> + for (thread_info *thread : all_threads)
> + {
> + /* Only threads that are of the process we are detaching. */
> + if (thread->id.pid () != pid)
> + continue;
> +
> + /* Only threads that have a pending fork event. */
> + if (thread->fork_child == nullptr)
> + continue;
> +
> + process_info *fork_child_process
> + = find_process_pid (thread->fork_child->id.pid ());
This can be written as:
process_info *fork_child_process
= get_thread_process (thread->fork_child);
> + gdb_assert (fork_child_process != nullptr);
Hmm, in my clone events work, I'm making clone events also set
the fork_parent/fork_child link, since clone events and fork/vfork
events are so similar. In this spot however, we'll need to skip
detaching the child if the child is a clone child, not a fork child.
I wonder how best to check that. See comments in the review of patch #1.
> +
> + int fork_child_pid = fork_child_process->pid;
> +
> + if (detach_inferior (fork_child_process) != 0)
> + warning (_("Failed to detach fork child %s, child of %s"),
> + target_pid_to_str (ptid_t (fork_child_pid)).c_str (),
> + target_pid_to_str (thread->id).c_str ());
> + }
> +
> if (detach_inferior (process) != 0)
> write_enn (own_buf);
> else
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdbserver: hide fork child threads from GDB
2021-11-26 22:51 ` Pedro Alves
@ 2021-11-29 12:46 ` Simon Marchi
2021-11-29 15:37 ` Pedro Alves
0 siblings, 1 reply; 15+ messages in thread
From: Simon Marchi @ 2021-11-29 12:46 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
On 2021-11-26 17:51, Pedro Alves wrote:
> On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote:
>
>> I split the field in two (fork_parent / fork_child), I think it's
>> clearer this way, easier to follow which thread is the parent and which
>> is the child, and helps ensure things are consistent. That simplifies
>> things a bit in linux_set_resume_request.
>
> This had been conscientiously done that way to avoid storing redundant
> information. You could have the same thing with a single field + the
> waitstatus, wrapped in fork_child()/fork_parent() methods. But I'm fine with
> with that you have. I do see a different reason for taking the two-fields approach
> though -- you don't have access to the pending waitstatus in common code.
> That would be a better rationale, IMO. OTOH, I think we will end up needing
> access to the pending waitstatus anyhow. See below, and the review to patch #3.
>
>> Currently, when
>> lwp->fork_relative is set, we have to deduce whether this is a parent or
>> child using the pending event. With separate fork_parent and fork_child
>> fields, it becomes more obvious. If the thread has a fork parent, then
>> it means it's a fork child, and vice-versa.
>
> With clone events, in some spots I'll need to be able to know whether
> fork_child being set means the thread is stopped for a fork/vfork or a
> clone event. Most of the code related to fork_parent/fork_child is the
> same for forks and for clones, but not all.
>
> Wonder whether we should instead have some:
>
> thread_info *target_pending_child (thread_info *parent);
> thread_info *target_pending_child_parent (thread_info *child);
> target_waitkind target_pending_child_kind (thread_info *parent);
>
> target methods instead of moving the fields. I haven't thought this
> fully through, though.
At first I wasn't sure, I was thinking that having the fields directly
in thread_info would help avoid duplication, in case another OS (e.g.
NetBSD) decided to implement the same thing. But not really: even if
the fields are in thread_info, the bulk of the work (updating these
fields) is done by the target. Since the state is managed by the
target, target methods make sense.
I will add a "target_thread_pending_parent" in this patch, a
"target_thread_pending_child" in patch 3, and in your series you can add
a way to get the kind. Either a new method, or I was thinking of an
output parameter on target_thread_pending_child. Sounds good?
Simon
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] gdbserver: hide fork child threads from GDB
2021-11-29 12:46 ` Simon Marchi
@ 2021-11-29 15:37 ` Pedro Alves
0 siblings, 0 replies; 15+ messages in thread
From: Pedro Alves @ 2021-11-29 15:37 UTC (permalink / raw)
To: Simon Marchi, Simon Marchi, gdb-patches
On 2021-11-29 12:46, Simon Marchi wrote:
> On 2021-11-26 17:51, Pedro Alves wrote:
>> Wonder whether we should instead have some:
>>
>> thread_info *target_pending_child (thread_info *parent);
>> thread_info *target_pending_child_parent (thread_info *child);
>> target_waitkind target_pending_child_kind (thread_info *parent);
>>
>> target methods instead of moving the fields. I haven't thought this
>> fully through, though.
>
> At first I wasn't sure, I was thinking that having the fields directly
> in thread_info would help avoid duplication, in case another OS (e.g.
> NetBSD) decided to implement the same thing. But not really: even if
> the fields are in thread_info, the bulk of the work (updating these
> fields) is done by the target. Since the state is managed by the
> target, target methods make sense.
>
> I will add a "target_thread_pending_parent" in this patch, a
> "target_thread_pending_child" in patch 3, and in your series you can add
> a way to get the kind. Either a new method, or I was thinking of an
> output parameter on target_thread_pending_child. Sounds good?
Yes, output parameter sound great.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent
2021-11-26 22:54 ` Pedro Alves
@ 2021-11-29 18:27 ` Simon Marchi
0 siblings, 0 replies; 15+ messages in thread
From: Simon Marchi @ 2021-11-29 18:27 UTC (permalink / raw)
To: Pedro Alves, Simon Marchi, gdb-patches
> Yes, this can certainly happen. I don't see a way to reliably stop the
> program with an event held in the stop reply queue. GDB constantly drains the
> events asynchronously. So for a testcase, I think you'd need to take
> "constantly hammer" approach. Something like:
>
> - enable non-stop / target non-stop
> - make the test program have a few threads constantly fork and exit the child
> I say multiple threads so that the stop reply queue has a good chance of
> holding an event unprocessed
> - make the test program call alarm() to set a timeout. See why below.
> - make the testcase constantly attach / continue -a & / detach
>
> Determining whether the fork child was detached correctly would be done by:
>
> - the parent/child sharing a pipe. You'd have an array of pipes, one
> entry per thread.
> - the child writing to the pipe before exiting
> - the parent reading from the pipe after forking
>
> If GDB fails to detach the child, then the parent thread hangs in
> the pipe read, and eventually, the parent gets a SIGALRM.
>
> To avoid the case of the test failing, then the parent dying with SIGALRM, and
> GDB attaching to the wrong process (because the kernel reused the PID for
> something else), you'd make the SIGALRM signal handler set a "timed_out"
> global flag, and sleep for a while before exiting the process. Enough time
> so that GDB can always reattach before the process disappears due to SIGALRM.
>
> When GDB reattaches, it first always consults that "timed_out" global, to
> know whether the detach-from-children succeeded. If it is set, the test
> FAILs.
Ok, that sounds good (although not trivial!).
>> }
>> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
>> index 7963f2faf267..2f3d5341758f 100644
>> --- a/gdbserver/server.cc
>> +++ b/gdbserver/server.cc
>> @@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
>> /* We'll need this after PROCESS has been destroyed. */
>> int pid = process->pid;
>>
>> + /* If this process has an unreported fork child, that child is not known to
>> + GDB, so detach it as well.
>> +
>> + Here, we specifically don't want to use "safe iteration", as detaching
>> + another process might delete the next thread in the iteration, which is
>> + the one saved by the safe iterator. We will never delete the currently
>> + iterated on thread, so standard iteration should be safe. */
>> + for (thread_info *thread : all_threads)
>> + {
>> + /* Only threads that are of the process we are detaching. */
>> + if (thread->id.pid () != pid)
>> + continue;
>> +
>> + /* Only threads that have a pending fork event. */
>> + if (thread->fork_child == nullptr)
>> + continue;
>> +
>> + process_info *fork_child_process
>> + = find_process_pid (thread->fork_child->id.pid ());
>
> This can be written as:
>
> process_info *fork_child_process
> = get_thread_process (thread->fork_child);
Done.
>
>> + gdb_assert (fork_child_process != nullptr);
>
>
> Hmm, in my clone events work, I'm making clone events also set
> the fork_parent/fork_child link, since clone events and fork/vfork
> events are so similar. In this spot however, we'll need to skip
> detaching the child if the child is a clone child, not a fork child.
> I wonder how best to check that. See comments in the review of patch #1.
Right. In my new versions, the loop has:
/* Only threads that have a pending fork event. */
thread_info *child = target_thread_pending_child (thread);
if (child == nullptr)
continue;
In your series, you'll make target_thread_pending_child return the kind,
and you can add a check for the kind.
Simon
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-11-29 18:28 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 20:33 [PATCH 1/2] gdbserver: hide fork child threads from GDB Simon Marchi
2021-10-29 20:33 ` [PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
2021-11-12 20:54 ` [PATCH 1/2] gdbserver: hide fork child threads from GDB (ping) Simon Marchi
2021-11-24 20:04 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2021-11-24 20:04 ` [PATCH 1/3] gdbserver: hide fork child threads from GDB Simon Marchi
2021-11-26 22:51 ` Pedro Alves
2021-11-29 12:46 ` Simon Marchi
2021-11-29 15:37 ` Pedro Alves
2021-11-24 20:04 ` [PATCH 2/3] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
2021-11-26 22:51 ` Pedro Alves
2021-11-24 20:04 ` [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
2021-11-26 22:54 ` Pedro Alves
2021-11-29 18:27 ` Simon Marchi
2021-11-24 20:56 ` [PATCH 0/3] Fix handling of pending fork events Simon Marchi
2021-11-26 22:50 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).