[PATCH v3 0/7] Fix handling of pending fork events

public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed

* [PATCH v3 0/7] Fix handling of pending fork events
@ 2021-12-01 14:44 Simon Marchi
  2021-12-01 14:44 ` [PATCH v3 1/7] gdbserver: hide fork child threads from GDB Simon Marchi
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

This is v3 of:

    https://sourceware.org/pipermail/gdb-patches/2021-November/183797.html

The series grew a little bit:

  - gdbserver: hide fork child threads from GDB

    Don't move the fork_relative field to thread_info, add a new target
    method instead.

  - gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function

    No changes.

  - gdbserver: suppress "Detaching from process" message

    New, makes testing in last patch easier.  Potentially controversial.

  - gdb/remote.c: move some things up

    New, trivial changes.

  - gdb/remote.c: refactor pending fork status functions

    New, small refactor to clean things up.

  - gdb: move clearing of tp->pending_follow to follow_fork_inferior

    New, fix necessary for last patch.

  - gdb, gdbserver: detach fork child when detaching from fork parent

    Handle detaching after a "catch fork" stop.  Handle fork statuses in
    pending stop replies in the remote target, add new test for this.


Simon Marchi (7):
  gdbserver: hide fork child threads from GDB
  gdb/linux-nat: factor ptrace-detach code to new detach_one_pid
    function
  gdbserver: suppress "Detaching from process" message
  gdb/remote.c: move some things up
  gdb/remote.c: refactor pending fork status functions
  gdb: move clearing of tp->pending_follow to follow_fork_inferior
  gdb, gdbserver: detach fork child when detaching from fork parent

 gdb/infrun.c                                  |  28 ++-
 gdb/linux-nat.c                               | 129 +++++++---
 gdb/remote.c                                  | 232 ++++++++++--------
 .../pending-fork-event-detach-ns.c            |  98 ++++++++
 .../pending-fork-event-detach-ns.exp          |  67 +++++
 .../pending-fork-event-detach-touch-file.c    |  26 ++
 .../gdb.threads/pending-fork-event-detach.c   |  86 +++++++
 .../gdb.threads/pending-fork-event-detach.exp | 127 ++++++++++
 gdb/testsuite/lib/gdb.exp                     |  21 ++
 gdbserver/linux-low.cc                        |  22 ++
 gdbserver/linux-low.h                         |  56 +++++
 gdbserver/server.cc                           |  36 ++-
 gdbserver/target.cc                           |  12 +
 gdbserver/target.h                            |  20 ++
 14 files changed, 806 insertions(+), 154 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach.c
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach.exp

-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 1/7] gdbserver: hide fork child threads from GDB
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:30   ` Pedro Alves
  2021-12-01 14:44 ` [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

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.  This information
needs to be made available to the handle_qxfer_threads_worker function,
so it can filter the reported threads.  Add a new thread_pending_parent
target function that allows the Linux target to return the parent of an
eventual fork child.

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        | 79 ++++++++++++++++++
 gdbserver/linux-low.cc                        | 11 +++
 gdbserver/linux-low.h                         | 29 +++++++
 gdbserver/server.cc                           |  6 ++
 gdbserver/target.cc                           |  6 ++
 gdbserver/target.h                            | 10 +++
 7 files changed, 223 insertions(+)
 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..51af07f56bdd
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -0,0 +1,79 @@
+# 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.
+#
+# 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/linux-low.cc b/gdbserver/linux-low.cc
index d214aff7051e..34991df449bf 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7119,6 +7119,17 @@ linux_process_target::thread_handle (ptid_t ptid, gdb_byte **handle,
 }
 #endif
 
+thread_info *
+linux_process_target::thread_pending_parent (thread_info *thread)
+{
+  lwp_info *parent = get_thread_lwp (thread)->pending_parent ();
+
+  if (parent == nullptr)
+    return nullptr;
+
+  return get_lwp_thread (parent);
+}
+
 /* Default implementation of linux_target_ops method "set_pc" for
    32-bit pc register which is literally named "pc".  */
 
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 05067ffc6e6f..819f915ea9a3 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -311,6 +311,8 @@ class linux_process_target : public process_stratum_target
 		      int *handle_len) override;
 #endif
 
+  thread_info *thread_pending_parent (thread_info *thread) override;
+
   bool supports_catch_syscall () override;
 
   /* Return the information to access registers.  This has public
@@ -721,6 +723,33 @@ struct pending_signal
 
 struct lwp_info
 {
+  /* If this LWP is a fork child that wasn't reported to GDB yet, return
+     its parent, else nullptr.  */
+  lwp_info *pending_parent () const
+  {
+    if (this->fork_relative == nullptr)
+      return nullptr;
+
+    gdb_assert (this->fork_relative->fork_relative == this);
+
+    /* In a fork parent/child relationship, the parent has a status pending and
+       the child does not, and a thread can only be in one such relationship
+       at most.  So we can recognize who is the parent based on which one has
+       a pending status.  */
+    gdb_assert (!!this->status_pending_p
+		!= !!this->fork_relative->status_pending_p);
+
+    if (!this->fork_relative->status_pending_p)
+      return nullptr;
+
+    const target_waitstatus &ws
+      = this->fork_relative->waitstatus;
+    gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED
+		|| ws.kind () == TARGET_WAITKIND_VFORKED);
+
+    return this->fork_relative;
+  }
+
   /* Backlink to the parent object.  */
   struct thread_info *thread = nullptr;
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b1d4c92b3d97..8dde6fb07295 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 (target_thread_pending_parent (thread) != nullptr)
+    return;
+
   write_ptid (ptid_s, ptid);
 
   buffer_xml_printf (buffer, "<thread id=\"%s\"", ptid_s);
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index bfa860546a0a..136b5104de85 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -835,6 +835,12 @@ process_stratum_target::thread_handle (ptid_t ptid, gdb_byte **handle,
   return false;
 }
 
+thread_info *
+process_stratum_target::thread_pending_parent (thread_info *thread)
+{
+  return nullptr;
+}
+
 bool
 process_stratum_target::supports_software_single_step ()
 {
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 6c863c84666a..1b0a1201d755 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -488,6 +488,10 @@ class process_stratum_target
   virtual bool thread_handle (ptid_t ptid, gdb_byte **handle,
 			      int *handle_len);
 
+  /* If THREAD is a fork child that was not reported to GDB, return its parent
+     else nullptr.  */
+  virtual thread_info *thread_pending_parent (thread_info *thread);
+
   /* Returns true if the target can software single step.  */
   virtual bool supports_software_single_step ();
 
@@ -698,6 +702,12 @@ void done_accessing_memory (void);
 #define target_thread_handle(ptid, handle, handle_len) \
   the_target->thread_handle (ptid, handle, handle_len)
 
+static inline thread_info *
+target_thread_pending_parent (thread_info *thread)
+{
+  return the_target->thread_pending_parent (thread);
+}
+
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
 int set_desired_thread ();
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
  2021-12-01 14:44 ` [PATCH v3 1/7] gdbserver: hide fork child threads from GDB Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:30   ` Pedro Alves
  2021-12-01 14:44 ` [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message Simon Marchi
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

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 fbb60a398b0c..f00732d3cd0c 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.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
  2021-12-01 14:44 ` [PATCH v3 1/7] gdbserver: hide fork child threads from GDB Simon Marchi
  2021-12-01 14:44 ` [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:42   ` Pedro Alves
  2021-12-01 14:44 ` [PATCH v3 4/7] gdb/remote.c: move some things up Simon Marchi
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

The motivation behind this patch is that a following patch in the series
adds a test that constantly spawns forks.  This makes GDBserver flood
its stdout with "Detaching from process" messages.  Because the
testsuite infrastructure does not consume GDBserver's stdout, the pipe
fills up quickly, and GDBserver eventually blocks on a write to stdout
forever.

I first tried to make the testsuite consume GDBserver's stdout, but that
wasn't very pretty.  We would need to do it at various random points,
and it could easily break depending on the particular timing.  Note that
it could still be useful to have the testsuite consume GDBserver's
output from time to time, so that we could see any GDBserver output in
the logs, but it would not be a reliable way of handling this particular
case, where GDBserver outputs so much.

I'm wondering if we could simply remove that message, if anybody would
miss it.  Personally, I don't think it's particularly useful.  The user
already gets notifications in GDB, if that's what they want.

An alternative would be to add an option to control whether we print
this or that message.  But I'd like to avoid adding options and increase
the complexity if that's not really necessary.

I thought about starting GDBserver with its stdout redirected to
/dev/null, since we don't consume it anyway, but that wouldn't work: we
actually consume the output a little bit in the beginning to see if
GDBserver was started successfully, or if the specified port was already
taken.

Change-Id: Ib371a4bcaeb6dfb5c03077e816019c8473d4d198
---
 gdbserver/server.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 8dde6fb07295..03074fde51da 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1244,7 +1244,6 @@ handle_detach (char *own_buf)
       return;
     }
 
-  fprintf (stderr, "Detaching from process %d\n", process->pid);
   stop_tracing ();
 
   /* We'll need this after PROCESS has been destroyed.  */
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 4/7] gdb/remote.c: move some things up
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
                   ` (2 preceding siblings ...)
  2021-12-01 14:44 ` [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:42   ` Pedro Alves
  2021-12-01 14:44 ` [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions Simon Marchi
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

Move the stop_reply and a few functions up.  Some code above them in the
file will need to use them in a following patch.  No behavior changes
expected here.

Change-Id: I3ca57d0e3ec253f56e1ba401289d9d167de14ad2
---
 gdb/remote.c | 144 +++++++++++++++++++++++++--------------------------
 1 file changed, 71 insertions(+), 73 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index f53e31e126e3..9b569b827413 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -996,6 +996,36 @@ class extended_remote_target final : public remote_target
   bool supports_disable_randomization () override;
 };
 
+struct stop_reply : public notif_event
+{
+  ~stop_reply ();
+
+  /* The identifier of the thread about this event  */
+  ptid_t ptid;
+
+  /* The remote state this event is associated with.  When the remote
+     connection, represented by a remote_state object, is closed,
+     all the associated stop_reply events should be released.  */
+  struct remote_state *rs;
+
+  struct target_waitstatus ws;
+
+  /* The architecture associated with the expedited registers.  */
+  gdbarch *arch;
+
+  /* Expedited registers.  This makes remote debugging a bit more
+     efficient for those targets that provide critical registers as
+     part of their normal status mechanism (as another roundtrip to
+     fetch them is avoided).  */
+  std::vector<cached_reg_t> regcache;
+
+  enum target_stop_reason stop_reason;
+
+  CORE_ADDR watch_data_address;
+
+  int core;
+};
+
 /* See remote.h.  */
 
 bool
@@ -5815,6 +5845,47 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
     rs->wait_forever_enabled_p = 1;
 }
 
+/* Determine if THREAD_PTID is a pending fork parent thread.  ARG contains
+   the pid of the process that owns the threads we want to check, or
+   -1 if we want to check all threads.  */
+
+static int
+is_pending_fork_parent (const target_waitstatus &ws, int event_pid,
+			ptid_t thread_ptid)
+{
+  if (ws.kind () == TARGET_WAITKIND_FORKED
+      || ws.kind () == TARGET_WAITKIND_VFORKED)
+    {
+      if (event_pid == -1 || event_pid == thread_ptid.pid ())
+	return 1;
+    }
+
+  return 0;
+}
+
+/* Return the thread's pending status used to determine whether the
+   thread is a fork parent stopped at a fork event.  */
+
+static const target_waitstatus &
+thread_pending_fork_status (struct thread_info *thread)
+{
+  if (thread->has_pending_waitstatus ())
+    return thread->pending_waitstatus ();
+  else
+    return thread->pending_follow;
+}
+
+/* Determine if THREAD is a pending fork parent thread.  */
+
+static int
+is_pending_fork_parent_thread (struct thread_info *thread)
+{
+  const target_waitstatus &ws = thread_pending_fork_status (thread);
+  int pid = -1;
+
+  return is_pending_fork_parent (ws, pid, thread->ptid);
+}
+
 /* Detach the specified process.  */
 
 void
@@ -6507,8 +6578,6 @@ remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal)
     rs->waiting_for_stop_reply = 1;
 }
 
-static int is_pending_fork_parent_thread (struct thread_info *thread);
-
 /* Private per-inferior info for target remote processes.  */
 
 struct remote_inferior : public private_inferior
@@ -6528,36 +6597,6 @@ get_remote_inferior (inferior *inf)
   return static_cast<remote_inferior *> (inf->priv.get ());
 }
 
-struct stop_reply : public notif_event
-{
-  ~stop_reply ();
-
-  /* The identifier of the thread about this event  */
-  ptid_t ptid;
-
-  /* The remote state this event is associated with.  When the remote
-     connection, represented by a remote_state object, is closed,
-     all the associated stop_reply events should be released.  */
-  struct remote_state *rs;
-
-  struct target_waitstatus ws;
-
-  /* The architecture associated with the expedited registers.  */
-  gdbarch *arch;
-
-  /* Expedited registers.  This makes remote debugging a bit more
-     efficient for those targets that provide critical registers as
-     part of their normal status mechanism (as another roundtrip to
-     fetch them is avoided).  */
-  std::vector<cached_reg_t> regcache;
-
-  enum target_stop_reason stop_reason;
-
-  CORE_ADDR watch_data_address;
-
-  int core;
-};
-
 /* Class used to track the construction of a vCont packet in the
    outgoing packet buffer.  This is used to send multiple vCont
    packets if we have more actions than would fit a single packet.  */
@@ -7224,47 +7263,6 @@ struct notif_client notif_client_stop =
   REMOTE_NOTIF_STOP,
 };
 
-/* Determine if THREAD_PTID is a pending fork parent thread.  ARG contains
-   the pid of the process that owns the threads we want to check, or
-   -1 if we want to check all threads.  */
-
-static int
-is_pending_fork_parent (const target_waitstatus &ws, int event_pid,
-			ptid_t thread_ptid)
-{
-  if (ws.kind () == TARGET_WAITKIND_FORKED
-      || ws.kind () == TARGET_WAITKIND_VFORKED)
-    {
-      if (event_pid == -1 || event_pid == thread_ptid.pid ())
-	return 1;
-    }
-
-  return 0;
-}
-
-/* Return the thread's pending status used to determine whether the
-   thread is a fork parent stopped at a fork event.  */
-
-static const target_waitstatus &
-thread_pending_fork_status (struct thread_info *thread)
-{
-  if (thread->has_pending_waitstatus ())
-    return thread->pending_waitstatus ();
-  else
-    return thread->pending_follow;
-}
-
-/* Determine if THREAD is a pending fork parent thread.  */
-
-static int
-is_pending_fork_parent_thread (struct thread_info *thread)
-{
-  const target_waitstatus &ws = thread_pending_fork_status (thread);
-  int pid = -1;
-
-  return is_pending_fork_parent (ws, pid, thread->ptid);
-}
-
 /* If CONTEXT contains any fork child threads that have not been
    reported yet, remove them from the CONTEXT list.  If such a
    thread exists it is because we are stopped at a fork catchpoint
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
                   ` (3 preceding siblings ...)
  2021-12-01 14:44 ` [PATCH v3 4/7] gdb/remote.c: move some things up Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:43   ` Pedro Alves
  2021-12-01 14:44 ` [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior Simon Marchi
  2021-12-01 14:45 ` [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

In preparation for a following patch, refactor a few things that I did
find a bit awkward, and to make them a bit more reusable.

 - Pass an inferior to kill_new_fork_children instead of a pid.  That
   allows iterating on only this inferior's threads and avoid further
   filtering on the thread's pid.
 - Change thread_pending_fork_status to return a non-nullptr value only
   if the thread does have a pending fork status.
 - Remove is_pending_fork_parent_thread, as one can just use
   thread_pending_fork_status and check for nullptr.
 - Replace is_pending_fork_parent with is_fork_status, which just
   returns if the given targeet_waitkind if a fork or a vfork.  Push
   filtering on the pid to the callers, when it is necessary.

Change-Id: I0764ccc684d40f054e39df6fa5458cc4c5d1cd7b
---
 gdb/remote.c | 109 +++++++++++++++++++++++----------------------------
 1 file changed, 50 insertions(+), 59 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9b569b827413..747f9f15af2f 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -776,7 +776,7 @@ class remote_target : public process_stratum_target
   void remote_btrace_maybe_reopen ();
 
   void remove_new_fork_children (threads_listing_context *context);
-  void kill_new_fork_children (int pid);
+  void kill_new_fork_children (inferior *inf);
   void discard_pending_stop_replies (struct inferior *inf);
   int stop_reply_queue_length ();
 
@@ -5845,45 +5845,30 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)
     rs->wait_forever_enabled_p = 1;
 }
 
-/* Determine if THREAD_PTID is a pending fork parent thread.  ARG contains
-   the pid of the process that owns the threads we want to check, or
-   -1 if we want to check all threads.  */
+/* Determine if WS represents a fork status.  */
 
-static int
-is_pending_fork_parent (const target_waitstatus &ws, int event_pid,
-			ptid_t thread_ptid)
+static bool
+is_fork_status (target_waitkind kind)
 {
-  if (ws.kind () == TARGET_WAITKIND_FORKED
-      || ws.kind () == TARGET_WAITKIND_VFORKED)
-    {
-      if (event_pid == -1 || event_pid == thread_ptid.pid ())
-	return 1;
-    }
-
-  return 0;
+  return (kind == TARGET_WAITKIND_FORKED
+	  || kind == TARGET_WAITKIND_VFORKED);
 }
 
-/* Return the thread's pending status used to determine whether the
-   thread is a fork parent stopped at a fork event.  */
+/* Return THREAD's pending status if the it is a pending fork parent, else
+   return nullptr.  */
 
-static const target_waitstatus &
+static const target_waitstatus *
 thread_pending_fork_status (struct thread_info *thread)
 {
-  if (thread->has_pending_waitstatus ())
-    return thread->pending_waitstatus ();
-  else
-    return thread->pending_follow;
-}
+  const target_waitstatus &ws
+    = (thread->has_pending_waitstatus ()
+       ? thread->pending_waitstatus ()
+       : thread->pending_follow);
 
-/* Determine if THREAD is a pending fork parent thread.  */
+  if (!is_fork_status (ws.kind ()))
+    return nullptr;
 
-static int
-is_pending_fork_parent_thread (struct thread_info *thread)
-{
-  const target_waitstatus &ws = thread_pending_fork_status (thread);
-  int pid = -1;
-
-  return is_pending_fork_parent (ws, pid, thread->ptid);
+  return &ws;
 }
 
 /* Detach the specified process.  */
@@ -6797,7 +6782,7 @@ remote_target::commit_resumed ()
       /* If a thread is the parent of an unfollowed fork, then we
 	 can't do a global wildcard, as that would resume the fork
 	 child.  */
-      if (is_pending_fork_parent_thread (tp))
+      if (thread_pending_fork_status (tp) != nullptr)
 	may_global_wildcard_vcont = false;
     }
 
@@ -7272,17 +7257,18 @@ struct notif_client notif_client_stop =
 void
 remote_target::remove_new_fork_children (threads_listing_context *context)
 {
-  int pid = -1;
   struct notif_client *notif = &notif_client_stop;
 
   /* For any threads stopped at a fork event, remove the corresponding
      fork child threads from the CONTEXT list.  */
   for (thread_info *thread : all_non_exited_threads (this))
     {
-      const target_waitstatus &ws = thread_pending_fork_status (thread);
+      const target_waitstatus *ws = thread_pending_fork_status (thread);
+
+      if (ws == nullptr)
+	continue;
 
-      if (is_pending_fork_parent (ws, pid, thread->ptid))
-	context->remove_thread (ws.child_ptid ());
+      context->remove_thread (ws->child_ptid ());
     }
 
   /* Check for any pending fork events (not reported or processed yet)
@@ -10035,45 +10021,48 @@ remote_target::getpkt_or_notif_sane (gdb::char_vector *buf, int forever,
   return getpkt_or_notif_sane_1 (buf, forever, 1, is_notif);
 }
 
-/* Kill any new fork children of process PID that haven't been
+/* Kill any new fork children of inferior INF that haven't been
    processed by follow_fork.  */
 
 void
-remote_target::kill_new_fork_children (int pid)
+remote_target::kill_new_fork_children (inferior *inf)
 {
   remote_state *rs = get_remote_state ();
   struct notif_client *notif = &notif_client_stop;
 
   /* Kill the fork child threads of any threads in process PID
      that are stopped at a fork event.  */
-  for (thread_info *thread : all_non_exited_threads (this))
+  for (thread_info *thread : inf->non_exited_threads ())
     {
-      const target_waitstatus &ws = thread->pending_follow;
+      const target_waitstatus *ws = thread_pending_fork_status (thread);
 
-      if (is_pending_fork_parent (ws, pid, thread->ptid))
-	{
-	  int child_pid = ws.child_ptid ().pid ();
-	  int res;
+      if (ws == nullptr)
+	continue;
 
-	  res = remote_vkill (child_pid);
-	  if (res != 0)
-	    error (_("Can't kill fork child process %d"), child_pid);
-	}
+      int child_pid = ws->child_ptid ().pid ();
+      int res = remote_vkill (child_pid);
+
+      if (res != 0)
+	error (_("Can't kill fork child process %d"), child_pid);
     }
 
   /* Check for any pending fork events (not reported or processed yet)
      in process PID and kill those fork child threads as well.  */
   remote_notif_get_pending_events (notif);
   for (auto &event : rs->stop_reply_queue)
-    if (is_pending_fork_parent (event->ws, pid, event->ptid))
-      {
-	int child_pid = event->ws.child_ptid ().pid ();
-	int res;
+    {
+      if (event->ptid.pid () != inf->pid)
+	continue;
 
-	res = remote_vkill (child_pid);
-	if (res != 0)
-	  error (_("Can't kill fork child process %d"), child_pid);
-      }
+      if (!is_fork_status (event->ws.kind ()))
+	continue;
+
+      int child_pid = event->ws.child_ptid ().pid ();
+      int res = remote_vkill (child_pid);
+
+      if (res != 0)
+	error (_("Can't kill fork child process %d"), child_pid);
+    }
 }
 
 \f
@@ -10083,18 +10072,20 @@ void
 remote_target::kill ()
 {
   int res = -1;
-  int pid = inferior_ptid.pid ();
+  inferior *inf = find_inferior_pid (this, inferior_ptid.pid ());
   struct remote_state *rs = get_remote_state ();
 
+  gdb_assert (inf != nullptr);
+
   if (packet_support (PACKET_vKill) != PACKET_DISABLE)
     {
       /* If we're stopped while forking and we haven't followed yet,
 	 kill the child task.  We need to do this before killing the
 	 parent task because if this is a vfork then the parent will
 	 be sleeping.  */
-      kill_new_fork_children (pid);
+      kill_new_fork_children (inf);
 
-      res = remote_vkill (pid);
+      res = remote_vkill (inf->pid);
       if (res == 0)
 	{
 	  target_mourn_inferior (inferior_ptid);
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
                   ` (4 preceding siblings ...)
  2021-12-01 14:44 ` [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions Simon Marchi
@ 2021-12-01 14:44 ` Simon Marchi
  2021-12-03 23:43   ` Pedro Alves
  2021-12-01 14:45 ` [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Simon Marchi

From: Simon Marchi <simon.marchi@efficios.com>

A following patch will change targets so that when they detach an
inferior, they also detach any pending fork children this inferior may
have.  While doing this, I hit a case where we couldn't differentiate
two cases, where in one we should detach the fork detach but not in the
other.

Suppose we continue past a fork with "follow-fork-mode == child" &&
"detach-on-fork on".  follow_fork_inferior calls target_detach to detach
the parent.  In that case the target should not detach the fork
child, as we'll continue debugging the child.  As of now, the
tp->pending_follow field of the thread who called fork still contains
the details about the fork.

Then, suppose we run to a fork catchpoint and the user types "detach".
In that case, the target should detach the fork child in addition to the
parent.  In that case as well, the tp->pending_follow field contains
the details about the fork.

To allow targets to differentiate the two cases, clear
tp->pending_follow a bit earlier, when following a fork.  Targets will
then see that tp->pending_follow contains TARGET_WAITKIND_SPURIOUS, and
won't detach the fork child.

As of this patch, no behavior changes are expected.

Change-Id: I537741859ed712cb531baaefc78bb934e2a28153
---
 gdb/infrun.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index e4739ed14f66..a1264f77f9bf 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -601,6 +601,23 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
   if (child_inf != nullptr)
     gdb_assert (!child_inf->thread_list.empty ());
 
+  /* Clear the parent thread's pending follow field.  Do this before calling
+     target_detach, so that the target can differentiate the two following
+     cases:
+
+      - We continue past a fork with "follow-fork-mode == child" &&
+	"detach-on-fork on", and therefore detach the parent.  In that
+	case the target should not detach the fork child.
+      - We run to a fork catchpoint and the user types "detach".  In that
+	case, the target should detach the fork child in addition to the
+	parent.
+
+     The former case will have pending_follow cleared, the later will have
+     pending_follow set.  */
+  thread_info *parent_thread = find_thread_ptid (parent_inf, parent_ptid);
+  gdb_assert (parent_thread != nullptr);
+  parent_thread->pending_follow.set_spurious ();
+
   /* Detach the parent if needed.  */
   if (follow_child)
     {
@@ -668,7 +685,6 @@ follow_fork ()
 {
   bool follow_child = (follow_fork_mode_string == follow_fork_mode_child);
   bool should_resume = true;
-  struct thread_info *tp;
 
   /* Copy user stepping state to the new inferior thread.  FIXME: the
      followed fork child thread should have a copy of most of the
@@ -714,7 +730,7 @@ follow_fork ()
 	}
     }
 
-  tp = inferior_thread ();
+  thread_info *tp = inferior_thread ();
 
   /* If there were any forks/vforks that were caught and are now to be
      followed, then do so now.  */
@@ -768,14 +784,6 @@ follow_fork ()
 	  }
 	else
 	  {
-	    /* This pending follow fork event is now handled, one way
-	       or another.  The previous selected thread may be gone
-	       from the lists by now, but if it is still around, need
-	       to clear the pending follow request.  */
-	    tp = find_thread_ptid (parent_targ, parent);
-	    if (tp)
-	      tp->pending_follow.set_spurious ();
-
 	    /* This makes sure we don't try to apply the "Switched
 	       over from WAIT_PID" logic above.  */
 	    nullify_last_target_wait_ptid ();
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent
  2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
                   ` (5 preceding siblings ...)
  2021-12-01 14:44 ` [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior Simon Marchi
@ 2021-12-01 14:45 ` Simon Marchi
  2021-12-03 23:44   ` Pedro Alves
  6 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-01 14:45 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.

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 (for example, because it got the event
   while stopping all threads, to present an all-stop stop on top of a
   non-stop target)
 - thread_info::pending_follow: if we ran to a "catch fork" and we
   detach at that moment

Additionally, pending fork events can be in target-specific fields:

 - For linux-nat, they can be in lwp_info::status and
   lwp_info::waitstatus.
 - For the remote target, they could be stored as pending stop replies,
   saved in `remote_state::notif_state::pending_event`, if not
   acknowledged yet, or in `remote_state::stop_reply_queue`, if
   acknowledged.  I followed the model of remove_new_fork_children for
   this: call remote_notif_get_pending_events to process /
   acknowledge any unacknowledged notification, then look through
   stop_reply_queue.

Update the gdb.threads/pending-fork-event.exp test (and rename it to
gdb.threads/pending-fork-event-detach.exp) to try to detach the process
while it is stopped with 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).

This 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).

Add another test, gdb.threads/pending-fork-event-ns.exp, specifically to
verify that we consider events in pending stop replies in the remote
target.  This test has many threads constantly forking, and we detach
from the program while the program is executing.  That gives us some
chance that we detach while a fork stop reply is stored in the remote
target.  To verify that we correctly detach all fork children, we ask
the parent to exit by sending it a SIGUSR1 signal and have it write a
file to the filesystem before exiting.  Because the parent's main thread
joins the forking threads, and the forking threads wait for their fork
children to exit, if some fork child is not detach by GDB, the parent
will not write the file, and the test will time out.  If I remove the
new remote_detach_pid calls in remote.c, the test fails eventually if I
run it in a loop.

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                               |  53 +++++++-
 gdb/remote.c                                  |  39 +++++-
 .../pending-fork-event-detach-ns.c            |  98 ++++++++++++++
 .../pending-fork-event-detach-ns.exp          |  67 +++++++++
 .../pending-fork-event-detach-touch-file.c    |  26 ++++
 ...rk-event.c => pending-fork-event-detach.c} |  10 +-
 .../gdb.threads/pending-fork-event-detach.exp | 127 ++++++++++++++++++
 .../gdb.threads/pending-fork-event.exp        |  79 -----------
 gdb/testsuite/lib/gdb.exp                     |  21 +++
 gdbserver/linux-low.cc                        |  11 ++
 gdbserver/linux-low.h                         |  27 ++++
 gdbserver/server.cc                           |  29 ++++
 gdbserver/target.cc                           |   6 +
 gdbserver/target.h                            |  10 ++
 14 files changed, 516 insertions(+), 87 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c
 rename gdb/testsuite/gdb.threads/{pending-fork-event.c => pending-fork-event-detach.c} (89%)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
 delete mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index f00732d3cd0c..5c5069dda795 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,48 @@ 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);
+    }
+
+  /* Check in thread_info::pending_follow.  */
+  if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
+      || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
+    detach_one_pid (tp->pending_follow.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 747f9f15af2f..6d83b40be1af 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5937,6 +5937,32 @@ 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 ())
+    {
+      const target_waitstatus *ws = thread_pending_fork_status (thread);
+
+      if (ws == nullptr)
+	continue;
+
+      remote_detach_pid (ws->child_ptid ().pid ());
+    }
+
+  /* Check also for any pending fork events in the stop reply queue.  */
+  remote_notif_get_pending_events (&notif_client_stop);
+  for (stop_reply_up &reply : rs->stop_reply_queue)
+    {
+      if (reply->ptid.pid () != pid)
+	continue;
+
+      if (!is_fork_status (reply->ws.kind ()))
+	continue;
+
+      remote_detach_pid (reply->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
@@ -7340,11 +7366,11 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
       /* Leave the notification pending, since the server expects that
 	 we acknowledge it with vStopped.  But clear its contents, so
 	 that later on when we acknowledge it, we also discard it.  */
+      remote_debug_printf
+	("discarding in-flight notification: ptid: %s, ws: %s\n",
+	 reply->ptid.to_string().c_str(),
+	 reply->ws.to_string ().c_str ());
       reply->ws.set_ignore ();
-
-      if (remote_debug)
-	fprintf_unfiltered (gdb_stdlog,
-			    "discarded in-flight notification\n");
     }
 
   /* Discard the stop replies we have already pulled with
@@ -7355,6 +7381,11 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
 			      {
 				return event->ptid.pid () == inf->pid;
 			      });
+  for (auto it = iter; it != rs->stop_reply_queue.end (); ++it)
+    remote_debug_printf
+      ("discarding queued stop reply: ptid: %s, ws: %s\n",
+       reply->ptid.to_string().c_str(),
+       reply->ws.to_string ().c_str ());
   rs->stop_reply_queue.erase (iter, rs->stop_reply_queue.end ());
 }
 
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
new file mode 100644
index 000000000000..7580b104345c
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
@@ -0,0 +1,98 @@
+#include <assert.h>
+#include <errno.h>
+#include <poll.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#define NUM_FORKING_THREADS 12
+
+static pthread_barrier_t barrier;
+static volatile int should_exit = 0;
+
+static void
+sigusr1_handler (int sig, siginfo_t *siginfo, void *context)
+{
+  should_exit = 1;
+}
+
+static void *
+forking_thread (void *arg)
+{
+  /* Wait for all forking threads to have spawned before fork-spamming.  */
+  pthread_barrier_wait (&barrier);
+
+  while (!should_exit)
+    {
+      pid_t pid = fork ();
+      if (pid == 0)
+	{
+	  /* Child */
+	  exit (8);
+	}
+      else
+	{
+	  /* Parent */
+	  int status;
+	  int ret = waitpid (pid, &status, 0);
+	  assert (ret == pid);
+	  assert (WIFEXITED (status));
+	  assert (WEXITSTATUS (status) == 8);
+	}
+    }
+
+  return NULL;
+}
+
+static void
+break_here_first (void)
+{
+}
+
+static pid_t my_pid;
+
+int
+main (void)
+{
+  pthread_t forking_threads[NUM_FORKING_THREADS];
+  int ret;
+  struct sigaction sa;
+
+  /* Just to make sure we don't run for ever.  */
+  alarm (30);
+
+  my_pid = getpid ();
+
+  break_here_first ();
+
+  pthread_barrier_init (&barrier, NULL, NUM_FORKING_THREADS);
+
+  memset (&sa, 0, sizeof (sa));
+  sa.sa_sigaction = sigusr1_handler;
+  ret = sigaction (SIGUSR1, &sa, NULL);
+  assert (ret == 0);
+
+  for (int i = 0; i < NUM_FORKING_THREADS; ++i)
+    {
+      ret = pthread_create (&forking_threads[i], NULL, forking_thread, NULL);
+      assert (ret == 0);
+    }
+
+  for (int i = 0; i < NUM_FORKING_THREADS; ++i)
+    {
+      ret = pthread_join (forking_threads[i], NULL);
+      assert (ret == 0);
+    }
+
+  printf("WRITING FILE\n");
+  FILE *f = fopen (TOUCH_FILE_PATH, "w");
+  assert (f != NULL);
+  ret = fclose (f);
+  assert (ret == 0);
+  printf("KBYE\n");
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
new file mode 100644
index 000000000000..2e85cd2ba297
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
@@ -0,0 +1,67 @@
+# 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/>.
+
+# Detach a running program that constantly forks, verify that we correctly
+# detach all fork children, for which events are pending.
+#
+# The strategy is:
+#
+#   - Resume a program in background (continue &) with many threads that
+#     constantly fork and wait for their fork children to exit.
+#   - Detach the program.  If testing against GDBserver, hope that the detach
+#     CLI command is processed while there is a stop reply pending in the
+#     remote target.
+#   - Signal the parent program to exit, by sending it a SIGUSR1 signal.
+#   - Have the parent write a flag file to the filesystem just before exiting.
+#   - If a pending fork child is mistakenly still attached, it will prevent its
+#     parent thread from waitpid'ing it, preventing the main thread from joining
+#     it, prevent it from writing the flag file, failing the test.
+
+standard_testfile
+
+set touch_file_path [standard_output_file flag]
+
+if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	executable [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]] != "" } {
+    return
+}
+
+proc do_test { } {
+    file delete $::touch_file_path
+    gdb_assert { ![file_exists $::touch_file_path] } "file does not exist before test"
+
+    save_vars { ::GDBFLAGS } {
+	append ::GDBFLAGS " -ex \"set non-stop on\""
+	clean_restart $::binfile
+    }
+
+    if { ![runto break_here_first] } {
+	return
+    }
+
+    set pid [get_integer_valueof "my_pid" -1]
+    if { $pid == -1 } {
+	error "could not get inferior pid"
+    }
+
+    gdb_test_no_output "set print inferior-events off"
+    gdb_test "continue &" ".*"
+    sleep 2
+    gdb_test "detach" ".*"
+    remote_exec target "kill -USR1 ${pid}"
+    gdb_assert { [file_exists_with_timeout $::touch_file_path] } "file exists after detach"
+}
+
+do_test
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c b/gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c
new file mode 100644
index 000000000000..5536381847b1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-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-detach.c
similarity index 89%
rename from gdb/testsuite/gdb.threads/pending-fork-event.c
rename to gdb/testsuite/gdb.threads/pending-fork-event-detach.c
index a39ca75a49ac..ecfed98fdfda 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.c
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach.c
@@ -32,18 +32,22 @@ break_here (void)
 static void
 do_fork (void)
 {
-  pthread_barrier_wait (&barrier);
-
   while (!release_forking_thread);
 
   if (FORK_FUNCTION () == 0)
-    _exit (0);
+    {
+      /* We create the file in a separate program that we exec: if FORK_FUNCTION
+	 is vfork, we shouldn't do anything more than an exec.  */
+      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-detach.exp b/gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
new file mode 100644
index 000000000000..869ea3e60da6
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
@@ -0,0 +1,127 @@
+# 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/>.
+
+# Then, test that if we detach an inferior with a pending fork child, that
+# child is correctly detached and resumes execution normally.  There are two
+# kinds of "pending fork child" we test:
+#
+#   - resulting of a fork catchpoint: we stop at a fork catchpoint and detach.
+#   - resulting of an all-stop stop on top of a non-stop target, where a fork
+#     event is saved as a pending wait status.  To test this, we stepi a thread
+#     while another one forks.  The stepi generally completes at least as fast
+#     as the fork, so we have a chance that the stop due to the stepi being
+#     complete is shown to the user while the fork event is saved for later.
+#
+# To verify that the child process is detached and resumes execution, we have
+# it write a file on the filesystem.  If we don't see the file after a certain
+# delay, it means the child was likely not detached, and the test fails.
+#
+# At the same time, this tests that having this pending fork event does not
+# cause other problems in general.  For example, 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 .c -touch-file.c
+
+set touch_file_bin $binfile-touch-file
+set touch_file_path [standard_output_file flag]
+
+set opts [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]
+if { [gdb_compile "$srcdir/$subdir/$srcfile2" $touch_file_bin executable $opts] != "" } {
+    return
+}
+
+proc do_test { target-non-stop who_forks fork_function stop_mode } {
+    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.
+    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
+    }
+
+    file delete $::touch_file_path
+    gdb_assert { ![file_exists $::touch_file_path] } "file does not exist before test"
+
+    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"
+
+    # There are two "pending fork child" modes we can test here:
+    #
+    #   - catch: set up a "catch fork" / "catch vfork" and run to it.
+    #   - stepi: stepi the non-forking thread while the forking thread,
+    #     well, forks.
+    if { $stop_mode == "catch" } {
+	gdb_test "catch fork"
+	gdb_test "catch vfork"
+	gdb_test "continue" "hit Catchpoint $::decimal.*fork.*"
+    } elseif { $stop_mode == "stepi" } {
+	# stepi the non-forking thread.
+	gdb_test "stepi"
+    } else {
+	error "invalid stop_mode value: $stop_mode"
+    }
+
+    # Make sure there's still a single inferior.
+    gdb_test "info inferior" {\* 1 [^\r\n]+}
+
+    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 $::touch_file_path] } "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 } {
+	    foreach_with_prefix stop_mode { stepi catch } {
+		do_test ${target-non-stop} $who_forks $fork_function $stop_mode
+	    }
+	}
+    }
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
deleted file mode 100644
index 51af07f56bdd..000000000000
--- a/gdb/testsuite/gdb.threads/pending-fork-event.exp
+++ /dev/null
@@ -1,79 +0,0 @@
-# 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.
-#
-# 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/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index b145fe8e2e83..313f2a932c43 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8317,5 +8317,26 @@ proc require { fn arg1 {arg2 ""} } {
     return -code return 0
 }
 
+# Return 1 if file PATH exists, else 0.
+
+proc file_exists { path } {
+    return [file exists $path]
+}
+
+# Wait up to ::TIMEOUT seconds for file PATH to exist.  Return
+# 1 if it does exist, 0 otherwise.
+
+proc file_exists_with_timeout { path } {
+    for {set i 0} {$i < $::timeout} {incr i} {
+	if { [file_exists $path] } {
+	    return 1
+	}
+
+	sleep 1
+    }
+
+    return 0
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 34991df449bf..87888044c1f8 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7130,6 +7130,17 @@ linux_process_target::thread_pending_parent (thread_info *thread)
   return get_lwp_thread (parent);
 }
 
+thread_info *
+linux_process_target::thread_pending_child (thread_info *thread)
+{
+  lwp_info *child = get_thread_lwp (thread)->pending_child ();
+
+  if (child == nullptr)
+    return nullptr;
+
+  return get_lwp_thread (child);
+}
+
 /* Default implementation of linux_target_ops method "set_pc" for
    32-bit pc register which is literally named "pc".  */
 
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 819f915ea9a3..b563537216a6 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -312,6 +312,7 @@ class linux_process_target : public process_stratum_target
 #endif
 
   thread_info *thread_pending_parent (thread_info *thread) override;
+  thread_info *thread_pending_child (thread_info *thread) override;
 
   bool supports_catch_syscall () override;
 
@@ -750,6 +751,32 @@ struct lwp_info
     return this->fork_relative;
   }
 
+  /* If this LWP is the parent of a fork child we haven't reported to GDB yet,
+     return that child, else nullptr.  */
+  lwp_info *pending_child () const
+  {
+    if (this->fork_relative == nullptr)
+      return nullptr;
+
+    gdb_assert (this->fork_relative->fork_relative == this);
+
+    /* In a fork parent/child relationship, the parent has a status pending and
+       the child does not, and a thread can only be in one such relationship
+       at most.  So we can recognize who is the parent based on which one has
+       a pending status.  */
+    gdb_assert (!!this->status_pending_p
+		!= !!this->fork_relative->status_pending_p);
+
+    if (!this->status_pending_p)
+      return nullptr;
+
+    const target_waitstatus &ws = this->waitstatus;
+    gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED
+		|| ws.kind () == TARGET_WAITKIND_VFORKED);
+
+    return this->fork_relative;
+  }
+
   /* Backlink to the parent object.  */
   struct thread_info *thread = nullptr;
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 03074fde51da..3dea522b2b72 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1249,6 +1249,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 GDB won't take care of detaching it.  We must do it here.
+
+     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.  */
+      thread_info *child = target_thread_pending_child (thread);
+      if (child == nullptr)
+	continue;
+
+      process_info *fork_child_process = get_thread_process (child);
+      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
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index 136b5104de85..aa3d42462f52 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -841,6 +841,12 @@ process_stratum_target::thread_pending_parent (thread_info *thread)
   return nullptr;
 }
 
+thread_info *
+process_stratum_target::thread_pending_child (thread_info *thread)
+{
+  return nullptr;
+}
+
 bool
 process_stratum_target::supports_software_single_step ()
 {
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 1b0a1201d755..331a21aa57a3 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -492,6 +492,10 @@ class process_stratum_target
      else nullptr.  */
   virtual thread_info *thread_pending_parent (thread_info *thread);
 
+  /* If THREAD is the parent of a fork child that was not reported to GDB,
+     return this child, else nullptr.  */
+  virtual thread_info *thread_pending_child (thread_info *thread);
+
   /* Returns true if the target can software single step.  */
   virtual bool supports_software_single_step ();
 
@@ -708,6 +712,12 @@ target_thread_pending_parent (thread_info *thread)
   return the_target->thread_pending_parent (thread);
 }
 
+static inline thread_info *
+target_thread_pending_child (thread_info *thread)
+{
+  return the_target->thread_pending_child (thread);
+}
+
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
 int set_desired_thread ();
-- 
2.33.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 1/7] gdbserver: hide fork child threads from GDB
  2021-12-01 14:44 ` [PATCH v3 1/7] gdbserver: hide fork child threads from GDB Simon Marchi
@ 2021-12-03 23:30   ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:30 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> 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.  This information
> needs to be made available to the handle_qxfer_threads_worker function,
> so it can filter the reported threads.  Add a new thread_pending_parent
> target function that allows the Linux target to return the parent of an
> eventual fork child.
> 
> 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

OK.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function
  2021-12-01 14:44 ` [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
@ 2021-12-03 23:30   ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:30 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> 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

OK.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message
  2021-12-01 14:44 ` [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message Simon Marchi
@ 2021-12-03 23:42   ` Pedro Alves
  2021-12-04  2:57     ` Simon Marchi
  0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:42 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> The motivation behind this patch is that a following patch in the series
> adds a test that constantly spawns forks.  This makes GDBserver flood
> its stdout with "Detaching from process" messages.  Because the
> testsuite infrastructure does not consume GDBserver's stdout, the pipe
> fills up quickly, and GDBserver eventually blocks on a write to stdout
> forever.
> 
> I first tried to make the testsuite consume GDBserver's stdout, but that
> wasn't very pretty.  We would need to do it at various random points,

Did you try doing it from within gdb_test_multiple?  Like, adding something
like '-i $server_spawnid" "." { exp_continue }' to the built-in matches?
I guess that might require an option to suppress that match.

Hmm, maybe instead, use expect_after/expect_before ?

> and it could easily break depending on the particular timing.  Note that
> it could still be useful to have the testsuite consume GDBserver's
> output from time to time, so that we could see any GDBserver output in
> the logs, but it would not be a reliable way of handling this particular
> case, where GDBserver outputs so much.

OOC, WDYM, by "not be a reliable way"?

> 
> I'm wondering if we could simply remove that message, if anybody would
> miss it.  Personally, I don't think it's particularly useful.  The user
> already gets notifications in GDB, if that's what they want.
> 
> An alternative would be to add an option to control whether we print
> this or that message.  But I'd like to avoid adding options and increase
> the complexity if that's not really necessary.

A "This or that" option, I agree, that'd be too much.  Maybe --verbose
would make sense.  Not sure.  See below.

> 
> I thought about starting GDBserver with its stdout redirected to
> /dev/null, since we don't consume it anyway, but that wouldn't work: we
> actually consume the output a little bit in the beginning to see if
> GDBserver was started successfully, or if the specified port was already
> taken.
> 
> Change-Id: Ib371a4bcaeb6dfb5c03077e816019c8473d4d198

The thing to me is that this looks like an arbitrary change in
isolation.

I mean, GDBserver prints this on attach:

  fprintf (stderr, "Attached; pid = %d\n", pid);

and this on kill / 'k':

  fprintf (stderr, "Killing all inferiors\n");

and likely other messages in other cases, including warnings.  Do we want a
policy that gdbserver never prints anything after startup?  Maybe we do.
Maybe add a "from_tty" argument to some functions, and only print if
we get there due to a command line option?  So traffic from GDB would not
cause extra prints.  I'd think you'd want to do something about those
messages mentioned above too and perhaps others, all at the same time.

Maybe it makes sense to have a --verbose mode that isn't full-blown
debug output, and put messages under it.  Not sure.  Maybe we don't and
instead move the messages under --debug.

Let's discuss the policy.  I'm fine with incremental progress toward it,
but we need to know where we're going.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 4/7] gdb/remote.c: move some things up
  2021-12-01 14:44 ` [PATCH v3 4/7] gdb/remote.c: move some things up Simon Marchi
@ 2021-12-03 23:42   ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:42 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Move the stop_reply and a few functions up.  Some code above them in the
> file will need to use them in a following patch.  No behavior changes
> expected here.
> 
> Change-Id: I3ca57d0e3ec253f56e1ba401289d9d167de14ad2

OK.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions
  2021-12-01 14:44 ` [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions Simon Marchi
@ 2021-12-03 23:43   ` Pedro Alves
  2021-12-04  3:03     ` Simon Marchi
  0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:43 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> In preparation for a following patch, refactor a few things that I did
> find a bit awkward, and to make them a bit more reusable.
> 
>  - Pass an inferior to kill_new_fork_children instead of a pid.  That
>    allows iterating on only this inferior's threads and avoid further
>    filtering on the thread's pid.
>  - Change thread_pending_fork_status to return a non-nullptr value only
>    if the thread does have a pending fork status.
>  - Remove is_pending_fork_parent_thread, as one can just use
>    thread_pending_fork_status and check for nullptr.
>  - Replace is_pending_fork_parent with is_fork_status, which just
>    returns if the given targeet_waitkind if a fork or a vfork.  Push

typo "targeet_waitkind".

>  
> -/* Return the thread's pending status used to determine whether the
> -   thread is a fork parent stopped at a fork event.  */
> +/* Return THREAD's pending status if the it is a pending fork parent, else
> +   return nullptr.  */

Some typo in "if the it is".

>  
> -static const target_waitstatus &
> +static const target_waitstatus *
>  thread_pending_fork_status (struct thread_info *thread)
>  {


> -/* Kill any new fork children of process PID that haven't been
> +/* Kill any new fork children of inferior INF that haven't been
>     processed by follow_fork.  */
>  
>  void
> -remote_target::kill_new_fork_children (int pid)
> +remote_target::kill_new_fork_children (inferior *inf)
>  {
>    remote_state *rs = get_remote_state ();
>    struct notif_client *notif = &notif_client_stop;
>  
>    /* Kill the fork child threads of any threads in process PID
>       that are stopped at a fork event.  */

Stale comment, refers to PID.

> -  for (thread_info *thread : all_non_exited_threads (this))
> +  for (thread_info *thread : inf->non_exited_threads ())
>      {
> -      const target_waitstatus &ws = thread->pending_follow;
> +      const target_waitstatus *ws = thread_pending_fork_status (thread);
>  
> -      if (is_pending_fork_parent (ws, pid, thread->ptid))
> -	{
> -	  int child_pid = ws.child_ptid ().pid ();
> -	  int res;
> +      if (ws == nullptr)
> +	continue;
>  
> -	  res = remote_vkill (child_pid);
> -	  if (res != 0)
> -	    error (_("Can't kill fork child process %d"), child_pid);
> -	}
> +      int child_pid = ws->child_ptid ().pid ();
> +      int res = remote_vkill (child_pid);
> +
> +      if (res != 0)
> +	error (_("Can't kill fork child process %d"), child_pid);
>      }
>  
>    /* Check for any pending fork events (not reported or processed yet)
>       in process PID and kill those fork child threads as well.  */

Ditto.

Otherwise LGTM.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior
  2021-12-01 14:44 ` [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior Simon Marchi
@ 2021-12-03 23:43   ` Pedro Alves
  0 siblings, 0 replies; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:43 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> A following patch will change targets so that when they detach an
> inferior, they also detach any pending fork children this inferior may
> have.  While doing this, I hit a case where we couldn't differentiate
> two cases, where in one we should detach the fork detach but not in the
> other.
> 
> Suppose we continue past a fork with "follow-fork-mode == child" &&
> "detach-on-fork on".  follow_fork_inferior calls target_detach to detach
> the parent.  In that case the target should not detach the fork
> child, as we'll continue debugging the child.  As of now, the
> tp->pending_follow field of the thread who called fork still contains
> the details about the fork.
> 
> Then, suppose we run to a fork catchpoint and the user types "detach".
> In that case, the target should detach the fork child in addition to the
> parent.  In that case as well, the tp->pending_follow field contains
> the details about the fork.
> 
> To allow targets to differentiate the two cases, clear
> tp->pending_follow a bit earlier, when following a fork.  Targets will
> then see that tp->pending_follow contains TARGET_WAITKIND_SPURIOUS, and
> won't detach the fork child.
> 
> As of this patch, no behavior changes are expected.
> 

OK.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent
  2021-12-01 14:45 ` [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
@ 2021-12-03 23:44   ` Pedro Alves
  2021-12-04  3:36     ` Simon Marchi
  0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2021-12-03 23:44 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

On 2021-12-01 14:45, Simon Marchi via Gdb-patches wrote:

> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
> @@ -0,0 +1,98 @@
> +#include <assert.h>

Missing copyright header.


> +int
> +main (void)
> +{
> +  pthread_t forking_threads[NUM_FORKING_THREADS];
> +  int ret;
> +  struct sigaction sa;
> +
> +  /* Just to make sure we don't run for ever.  */
> +  alarm (30);
> +
> +  my_pid = getpid ();
> +
> +  break_here_first ();
> +
> +  pthread_barrier_init (&barrier, NULL, NUM_FORKING_THREADS);
> +
> +  memset (&sa, 0, sizeof (sa));
> +  sa.sa_sigaction = sigusr1_handler;
> +  ret = sigaction (SIGUSR1, &sa, NULL);
> +  assert (ret == 0);
> +
> +  for (int i = 0; i < NUM_FORKING_THREADS; ++i)

This will fail to compile against gcc 4.8, because that didn't default to C99 yet.

I.e., move the "int i" declaration out of the "for" loop.

> +    {
> +      ret = pthread_create (&forking_threads[i], NULL, forking_thread, NULL);
> +      assert (ret == 0);
> +    }
> +
> +  for (int i = 0; i < NUM_FORKING_THREADS; ++i)
> +    {
> +      ret = pthread_join (forking_threads[i], NULL);
> +      assert (ret == 0);
> +    }
> +
> +  printf("WRITING FILE\n");

Space before parens.

> +  FILE *f = fopen (TOUCH_FILE_PATH, "w");
> +  assert (f != NULL);
> +  ret = fclose (f);
> +  assert (ret == 0);
> +  printf("KBYE\n");
> +
> +  return 0;
> +}

> +
> +proc do_test { } {
> +    file delete $::touch_file_path
> +    gdb_assert { ![file_exists $::touch_file_path] } "file does not exist before test"

These filesystem operations work on the build system's filesystem, while they should
be working on the target filesystem.  "remote_file target delete" etc.

> +
> +    save_vars { ::GDBFLAGS } {
> +	append ::GDBFLAGS " -ex \"set non-stop on\""
> +	clean_restart $::binfile
> +    }
> +
> +    if { ![runto break_here_first] } {
> +	return
> +    }
> +
> +    set pid [get_integer_valueof "my_pid" -1]
> +    if { $pid == -1 } {
> +	error "could not get inferior pid"
> +    }
> +
> +    gdb_test_no_output "set print inferior-events off"
> +    gdb_test "continue &" ".*"

Is this garanteed to not print something immediately after the prompt?
If not, then this is racy, and you should instead use gdb_test_multiple
with a pattern that ends in "$gdb_prompt ", without the $ anchor.

> +    sleep 2
> +    gdb_test "detach" ".*"
> +    remote_exec target "kill -USR1 ${pid}"
> +    gdb_assert { [file_exists_with_timeout $::touch_file_path] } "file exists after detach"
> +}
> +
> +do_test
> +

Otherwise LGTM.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message
  2021-12-03 23:42   ` Pedro Alves
@ 2021-12-04  2:57     ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2021-12-04  2:57 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches; +Cc: Simon Marchi

On 2021-12-03 18:42, Pedro Alves wrote:
> On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> The motivation behind this patch is that a following patch in the series
>> adds a test that constantly spawns forks.  This makes GDBserver flood
>> its stdout with "Detaching from process" messages.  Because the
>> testsuite infrastructure does not consume GDBserver's stdout, the pipe
>> fills up quickly, and GDBserver eventually blocks on a write to stdout
>> forever.
>>
>> I first tried to make the testsuite consume GDBserver's stdout, but that
>> wasn't very pretty.  We would need to do it at various random points,
> 
> Did you try doing it from within gdb_test_multiple?  Like, adding something
> like '-i $server_spawnid" "." { exp_continue }' to the built-in matches?
> I guess that might require an option to suppress that match.

When re-reading this, I understood that you meant changing
gdb_test_multiple itself.  That might not be a bad idea, but as you
said, it might require an option to not do it, as some tests do manually
consume the server_spawn_id output.

Before re-reading though, I thought you mean to use gdb_test_multiple in
my test to consume the GDBserver output.  So that's what I implemented,
and I think it's a reasonable solution for now.  My last patch will be
updated with this.

> Hmm, maybe instead, use expect_after/expect_before ?

I don't need to match anything else, just consume all that GDBserver
outputs while we sleep.  So just a single gdb_test_multiple + after
works fine.

>> and it could easily break depending on the particular timing.  Note that
>> it could still be useful to have the testsuite consume GDBserver's
>> output from time to time, so that we could see any GDBserver output in
>> the logs, but it would not be a reliable way of handling this particular
>> case, where GDBserver outputs so much.
> 
> OOC, WDYM, by "not be a reliable way"?

I don't quite remember what I was thinking about when I wrote that.
When I was trying to fix this, I tried to make the testsuite consume the
GDBserver inside gdb_test_multiple, but as a separate expect call,
before the main one.  So while we would drain all GDBserver's output at
that point, the command we then issue to GDB could cause GDBserver to
output a lot of messages, and fill up its buffer again.  But if we use
a `-i $server_spawn_id` clause inside the main expect call, then we
would consume the GDBserver output while waiting for the GDB command to
finish, so it would work fine.

So, disregard what I said.

>> I'm wondering if we could simply remove that message, if anybody would
>> miss it.  Personally, I don't think it's particularly useful.  The user
>> already gets notifications in GDB, if that's what they want.
>>
>> An alternative would be to add an option to control whether we print
>> this or that message.  But I'd like to avoid adding options and increase
>> the complexity if that's not really necessary.
> 
> A "This or that" option, I agree, that'd be too much.  Maybe --verbose
> would make sense.  Not sure.  See below.
> 
>>
>> I thought about starting GDBserver with its stdout redirected to
>> /dev/null, since we don't consume it anyway, but that wouldn't work: we
>> actually consume the output a little bit in the beginning to see if
>> GDBserver was started successfully, or if the specified port was already
>> taken.
>>
>> Change-Id: Ib371a4bcaeb6dfb5c03077e816019c8473d4d198
> 
> The thing to me is that this looks like an arbitrary change in
> isolation.

It is.  I sent this knowing it wasn't right, as I was stuck without a
solution.

> I mean, GDBserver prints this on attach:
> 
>   fprintf (stderr, "Attached; pid = %d\n", pid);
> 
> and this on kill / 'k':
> 
>   fprintf (stderr, "Killing all inferiors\n");
> 
> and likely other messages in other cases, including warnings.  Do we want a
> policy that gdbserver never prints anything after startup?  Maybe we do.
> Maybe add a "from_tty" argument to some functions, and only print if
> we get there due to a command line option?  So traffic from GDB would not
> cause extra prints.  I'd think you'd want to do something about those
> messages mentioned above too and perhaps others, all at the same time.
> 
> Maybe it makes sense to have a --verbose mode that isn't full-blown
> debug output, and put messages under it.  Not sure.  Maybe we don't and
> instead move the messages under --debug.
> 
> Let's discuss the policy.  I'm fine with incremental progress toward it,
> but we need to know where we're going.

Since we don't need this for the current patch, it will be work for
another day.  But yeah, I think it would make sense for GDBserver to
output nothing by default about its regular business, except for some
error messages that could help the user know that something went wrong.

Simon

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions
  2021-12-03 23:43   ` Pedro Alves
@ 2021-12-04  3:03     ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2021-12-04  3:03 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches; +Cc: Simon Marchi

On 2021-12-03 18:43, Pedro Alves wrote:
> On 2021-12-01 14:44, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> In preparation for a following patch, refactor a few things that I did
>> find a bit awkward, and to make them a bit more reusable.
>>
>>  - Pass an inferior to kill_new_fork_children instead of a pid.  That
>>    allows iterating on only this inferior's threads and avoid further
>>    filtering on the thread's pid.
>>  - Change thread_pending_fork_status to return a non-nullptr value only
>>    if the thread does have a pending fork status.
>>  - Remove is_pending_fork_parent_thread, as one can just use
>>    thread_pending_fork_status and check for nullptr.
>>  - Replace is_pending_fork_parent with is_fork_status, which just
>>    returns if the given targeet_waitkind if a fork or a vfork.  Push
> 
> typo "targeet_waitkind".

Fixed.

>> -/* Return the thread's pending status used to determine whether the
>> -   thread is a fork parent stopped at a fork event.  */
>> +/* Return THREAD's pending status if the it is a pending fork parent, else
>> +   return nullptr.  */
> 
> Some typo in "if the it is".

Fixed.

>> -static const target_waitstatus &
>> +static const target_waitstatus *
>>  thread_pending_fork_status (struct thread_info *thread)
>>  {
> 
> 
>> -/* Kill any new fork children of process PID that haven't been
>> +/* Kill any new fork children of inferior INF that haven't been
>>     processed by follow_fork.  */
>>  
>>  void
>> -remote_target::kill_new_fork_children (int pid)
>> +remote_target::kill_new_fork_children (inferior *inf)
>>  {
>>    remote_state *rs = get_remote_state ();
>>    struct notif_client *notif = &notif_client_stop;
>>  
>>    /* Kill the fork child threads of any threads in process PID
>>       that are stopped at a fork event.  */
> 
> Stale comment, refers to PID.

Fixed.

>> -  for (thread_info *thread : all_non_exited_threads (this))
>> +  for (thread_info *thread : inf->non_exited_threads ())
>>      {
>> -      const target_waitstatus &ws = thread->pending_follow;
>> +      const target_waitstatus *ws = thread_pending_fork_status (thread);
>>  
>> -      if (is_pending_fork_parent (ws, pid, thread->ptid))
>> -	{
>> -	  int child_pid = ws.child_ptid ().pid ();
>> -	  int res;
>> +      if (ws == nullptr)
>> +	continue;
>>  
>> -	  res = remote_vkill (child_pid);
>> -	  if (res != 0)
>> -	    error (_("Can't kill fork child process %d"), child_pid);
>> -	}
>> +      int child_pid = ws->child_ptid ().pid ();
>> +      int res = remote_vkill (child_pid);
>> +
>> +      if (res != 0)
>> +	error (_("Can't kill fork child process %d"), child_pid);
>>      }
>>  
>>    /* Check for any pending fork events (not reported or processed yet)
>>       in process PID and kill those fork child threads as well.  */
> 
> Ditto.

Fixed.

> Otherwise LGTM.

Thanks,

Simon

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent
  2021-12-03 23:44   ` Pedro Alves
@ 2021-12-04  3:36     ` Simon Marchi
  2021-12-07 23:25       ` Pedro Alves
  0 siblings, 1 reply; 20+ messages in thread
From: Simon Marchi @ 2021-12-04  3:36 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches; +Cc: Simon Marchi



On 2021-12-03 18:44, Pedro Alves wrote:
> On 2021-12-01 14:45, Simon Marchi via Gdb-patches wrote:
> 
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
>> @@ -0,0 +1,98 @@
>> +#include <assert.h>
> 
> Missing copyright header.

Done.

>> +int
>> +main (void)
>> +{
>> +  pthread_t forking_threads[NUM_FORKING_THREADS];
>> +  int ret;
>> +  struct sigaction sa;
>> +
>> +  /* Just to make sure we don't run for ever.  */
>> +  alarm (30);
>> +
>> +  my_pid = getpid ();
>> +
>> +  break_here_first ();
>> +
>> +  pthread_barrier_init (&barrier, NULL, NUM_FORKING_THREADS);
>> +
>> +  memset (&sa, 0, sizeof (sa));
>> +  sa.sa_sigaction = sigusr1_handler;
>> +  ret = sigaction (SIGUSR1, &sa, NULL);
>> +  assert (ret == 0);
>> +
>> +  for (int i = 0; i < NUM_FORKING_THREADS; ++i)
> 
> This will fail to compile against gcc 4.8, because that didn't default to C99 yet.
> 
> I.e., move the "int i" declaration out of the "for" loop.

Done.

>> +    {
>> +      ret = pthread_create (&forking_threads[i], NULL, forking_thread, NULL);
>> +      assert (ret == 0);
>> +    }
>> +
>> +  for (int i = 0; i < NUM_FORKING_THREADS; ++i)
>> +    {
>> +      ret = pthread_join (forking_threads[i], NULL);
>> +      assert (ret == 0);
>> +    }
>> +
>> +  printf("WRITING FILE\n");
> 
> Space before parens.

Woops, these printfs weren't supposed to make it in the final patch,
I'll remove them.  In my experience, having the inferior output things
can make tests flaky, as its output ends up the expect buffer
interleaved with GDB output, since they share a tty.

>> +  FILE *f = fopen (TOUCH_FILE_PATH, "w");
>> +  assert (f != NULL);
>> +  ret = fclose (f);
>> +  assert (ret == 0);
>> +  printf("KBYE\n");

Removing this one too.

>> +
>> +  return 0;
>> +}
> 
>> +
>> +proc do_test { } {
>> +    file delete $::touch_file_path
>> +    gdb_assert { ![file_exists $::touch_file_path] } "file does not exist before test"
> 
> These filesystem operations work on the build system's filesystem, while they should
> be working on the target filesystem.  "remote_file target delete" etc.

Hmm right.  I changed the operations to use remote_file.  I didn't test
with a remote target though, I don't have a setup for that on hand.

>> +
>> +    save_vars { ::GDBFLAGS } {
>> +	append ::GDBFLAGS " -ex \"set non-stop on\""
>> +	clean_restart $::binfile
>> +    }
>> +
>> +    if { ![runto break_here_first] } {
>> +	return
>> +    }
>> +
>> +    set pid [get_integer_valueof "my_pid" -1]
>> +    if { $pid == -1 } {
>> +	error "could not get inferior pid"
>> +    }
>> +
>> +    gdb_test_no_output "set print inferior-events off"
>> +    gdb_test "continue &" ".*"
> 
> Is this garanteed to not print something immediately after the prompt?
> If not, then this is racy, and you should instead use gdb_test_multiple
> with a pattern that ends in "$gdb_prompt ", without the $ anchor.

Done.

>> +    sleep 2
>> +    gdb_test "detach" ".*"
>> +    remote_exec target "kill -USR1 ${pid}"
>> +    gdb_assert { [file_exists_with_timeout $::touch_file_path] } "file exists after detach"
>> +}
>> +
>> +do_test
>> +
> 
> Otherwise LGTM.
> 

Here's a new version, since there are some quite significant changes.
All you mentioned above, plus the consuming from GDBserver in the
-ns.exp test.



From 68778a034956cc815919a3142e6397b10f07f5c7 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 1 Dec 2021 09:40:03 -0500
Subject: [PATCH] gdb, gdbserver: detach fork child when detaching from fork
 parent

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 (for example, because it got the event
   while stopping all threads, to present an all-stop stop on top of a
   non-stop target)
 - thread_info::pending_follow: if we ran to a "catch fork" and we
   detach at that moment

Additionally, pending fork events can be in target-specific fields:

 - For linux-nat, they can be in lwp_info::status and
   lwp_info::waitstatus.
 - For the remote target, they could be stored as pending stop replies,
   saved in `remote_state::notif_state::pending_event`, if not
   acknowledged yet, or in `remote_state::stop_reply_queue`, if
   acknowledged.  I followed the model of remove_new_fork_children for
   this: call remote_notif_get_pending_events to process /
   acknowledge any unacknowledged notification, then look through
   stop_reply_queue.

Update the gdb.threads/pending-fork-event.exp test (and rename it to
gdb.threads/pending-fork-event-detach.exp) to try to detach the process
while it is stopped with 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).

This 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).

Add another test, gdb.threads/pending-fork-event-ns.exp, specifically to
verify that we consider events in pending stop replies in the remote
target.  This test has many threads constantly forking, and we detach
from the program while the program is executing.  That gives us some
chance that we detach while a fork stop reply is stored in the remote
target.  To verify that we correctly detach all fork children, we ask
the parent to exit by sending it a SIGUSR1 signal and have it write a
file to the filesystem before exiting.  Because the parent's main thread
joins the forking threads, and the forking threads wait for their fork
children to exit, if some fork child is not detach by GDB, the parent
will not write the file, and the test will time out.  If I remove the
new remote_detach_pid calls in remote.c, the test fails eventually if I
run it in a loop.

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                               |  53 ++++++-
 gdb/remote.c                                  |  39 ++++-
 .../pending-fork-event-detach-ns.c            | 114 ++++++++++++++
 .../pending-fork-event-detach-ns.exp          | 120 +++++++++++++++
 .../pending-fork-event-detach-touch-file.c    |  26 ++++
 ...rk-event.c => pending-fork-event-detach.c} |  10 +-
 .../gdb.threads/pending-fork-event-detach.exp | 139 ++++++++++++++++++
 .../gdb.threads/pending-fork-event.exp        |  79 ----------
 gdb/testsuite/lib/gdb.exp                     |  15 ++
 gdbserver/linux-low.cc                        |  11 ++
 gdbserver/linux-low.h                         |  27 ++++
 gdbserver/server.cc                           |  29 ++++
 gdbserver/target.cc                           |   6 +
 gdbserver/target.h                            |  10 ++
 14 files changed, 591 insertions(+), 87 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c
 rename gdb/testsuite/gdb.threads/{pending-fork-event.c => pending-fork-event-detach.c} (89%)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
 delete mode 100644 gdb/testsuite/gdb.threads/pending-fork-event.exp

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 2f8cf498b73c..9d4b05a2167d 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,48 @@ 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);
+    }
+
+  /* Check in thread_info::pending_follow.  */
+  if (tp->pending_follow.kind () == TARGET_WAITKIND_VFORKED
+      || tp->pending_follow.kind () == TARGET_WAITKIND_FORKED)
+    detach_one_pid (tp->pending_follow.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 2397ee587be5..67111187e001 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5968,6 +5968,32 @@ 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 ())
+    {
+      const target_waitstatus *ws = thread_pending_fork_status (thread);
+
+      if (ws == nullptr)
+	continue;
+
+      remote_detach_pid (ws->child_ptid ().pid ());
+    }
+
+  /* Check also for any pending fork events in the stop reply queue.  */
+  remote_notif_get_pending_events (&notif_client_stop);
+  for (stop_reply_up &reply : rs->stop_reply_queue)
+    {
+      if (reply->ptid.pid () != pid)
+	continue;
+
+      if (!is_fork_status (reply->ws.kind ()))
+	continue;
+
+      remote_detach_pid (reply->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
@@ -7371,11 +7397,11 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
       /* Leave the notification pending, since the server expects that
 	 we acknowledge it with vStopped.  But clear its contents, so
 	 that later on when we acknowledge it, we also discard it.  */
+      remote_debug_printf
+	("discarding in-flight notification: ptid: %s, ws: %s\n",
+	 reply->ptid.to_string().c_str(),
+	 reply->ws.to_string ().c_str ());
       reply->ws.set_ignore ();
-
-      if (remote_debug)
-	fprintf_unfiltered (gdb_stdlog,
-			    "discarded in-flight notification\n");
     }
 
   /* Discard the stop replies we have already pulled with
@@ -7386,6 +7412,11 @@ remote_target::discard_pending_stop_replies (struct inferior *inf)
 			      {
 				return event->ptid.pid () == inf->pid;
 			      });
+  for (auto it = iter; it != rs->stop_reply_queue.end (); ++it)
+    remote_debug_printf
+      ("discarding queued stop reply: ptid: %s, ws: %s\n",
+       reply->ptid.to_string().c_str(),
+       reply->ws.to_string ().c_str ());
   rs->stop_reply_queue.erase (iter, rs->stop_reply_queue.end ());
 }
 
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
new file mode 100644
index 000000000000..5b9f9940f832
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.c
@@ -0,0 +1,114 @@
+/* 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 <assert.h>
+#include <errno.h>
+#include <poll.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#define NUM_FORKING_THREADS 12
+
+static pthread_barrier_t barrier;
+static volatile int should_exit = 0;
+
+static void
+sigusr1_handler (int sig, siginfo_t *siginfo, void *context)
+{
+  should_exit = 1;
+}
+
+static void *
+forking_thread (void *arg)
+{
+  /* Wait for all forking threads to have spawned before fork-spamming.  */
+  pthread_barrier_wait (&barrier);
+
+  while (!should_exit)
+    {
+      pid_t pid = fork ();
+      if (pid == 0)
+	{
+	  /* Child */
+	  exit (8);
+	}
+      else
+	{
+	  /* Parent */
+	  int status;
+	  int ret = waitpid (pid, &status, 0);
+	  assert (ret == pid);
+	  assert (WIFEXITED (status));
+	  assert (WEXITSTATUS (status) == 8);
+	}
+    }
+
+  return NULL;
+}
+
+static void
+break_here_first (void)
+{
+}
+
+static pid_t my_pid;
+
+int
+main (void)
+{
+  pthread_t forking_threads[NUM_FORKING_THREADS];
+  int ret;
+  struct sigaction sa;
+  int i;
+
+  /* Just to make sure we don't run for ever.  */
+  alarm (30);
+
+  my_pid = getpid ();
+
+  break_here_first ();
+
+  pthread_barrier_init (&barrier, NULL, NUM_FORKING_THREADS);
+
+  memset (&sa, 0, sizeof (sa));
+  sa.sa_sigaction = sigusr1_handler;
+  ret = sigaction (SIGUSR1, &sa, NULL);
+  assert (ret == 0);
+
+  for (i = 0; i < NUM_FORKING_THREADS; ++i)
+    {
+      ret = pthread_create (&forking_threads[i], NULL, forking_thread, NULL);
+      assert (ret == 0);
+    }
+
+  for (i = 0; i < NUM_FORKING_THREADS; ++i)
+    {
+      ret = pthread_join (forking_threads[i], NULL);
+      assert (ret == 0);
+    }
+
+  FILE *f = fopen (TOUCH_FILE_PATH, "w");
+  assert (f != NULL);
+  ret = fclose (f);
+  assert (ret == 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
new file mode 100644
index 000000000000..67c8058a12b3
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-ns.exp
@@ -0,0 +1,120 @@
+# 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/>.
+
+# Detach a running program that constantly forks, verify that we correctly
+# detach all fork children, for which events are pending.
+#
+# The strategy is:
+#
+#   - Resume a program in background (continue &) with many threads that
+#     constantly fork and wait for their fork children to exit.
+#   - Detach the program.  If testing against GDBserver, hope that the detach
+#     CLI command is processed while there is a stop reply pending in the
+#     remote target.
+#   - Signal the parent program to exit, by sending it a SIGUSR1 signal.
+#   - Have the parent write a flag file to the filesystem just before exiting.
+#   - If a pending fork child is mistakenly still attached, it will prevent its
+#     parent thread from waitpid'ing it, preventing the main thread from joining
+#     it, prevent it from writing the flag file, failing the test.
+
+standard_testfile
+
+if { [is_remote target] } {
+    # If the target is remote, write the file in whatever the current working
+    # directory is, with a somewhat unique name.
+    set touch_file_path ${testfile}-flag
+} else {
+    set touch_file_path [standard_output_file flag]
+}
+
+if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	executable [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]] != "" } {
+    return
+}
+
+proc do_test { } {
+    remote_file target delete $::touch_file_path
+    gdb_assert { ![remote_file target exists $::touch_file_path] } "file does not exist before test"
+
+    save_vars { ::GDBFLAGS } {
+	append ::GDBFLAGS " -ex \"set non-stop on\""
+	clean_restart $::binfile
+    }
+
+    if { ![runto break_here_first] } {
+	return
+    }
+
+    set pid [get_integer_valueof "my_pid" -1]
+    if { $pid == -1 } {
+	error "could not get inferior pid"
+    }
+
+    gdb_test_no_output "set print inferior-events off"
+
+    gdb_test_multiple "continue &" "" {
+	-re "Continuing.\r\n$::gdb_prompt " {
+	    pass $gdb_test_name
+	}
+    }
+
+    set wait_time_s 2
+
+    if { [info exists server_spawn_id] } {
+	# Let the program run for 2 seconds, during which it will fork many times.
+	# When running against GDBserver, this makes server print a ton of
+	# "Detaching from process X" message, to the point where its output buffer
+	# gets full and it hangs in a write to stdout.  During these 2 seconds,
+	# drain the messages from GDBserver to keep that from happening.
+	set ::pending_fork_detach_ns_stop 0
+	after [expr $wait_time_s * 1000] { set ::pending_fork_detach_ns_stop 1 }
+	gdb_test_multiple "" "" {
+	    -i $::server_spawn_id
+	    -re "Detaching from process $::decimal\r\n" {
+		if { !$::pending_fork_detach_ns_stop } {
+		    exp_continue
+		}
+	    }
+	}
+    } else {
+	# Not using GDBserver, just sleep 2 seconds.
+	sleep $wait_time_s
+    }
+
+    gdb_test "detach" "Detaching from program: .*"
+
+    if { [info exists server_spawn_id] } {
+	# Drain GDBserver's output buffer, in the (unlikely) event that enough
+	# messages were output to fill the buffer between the moment we stopped
+	# consuming it and the moment GDBserver detached the process.
+	gdb_test_multiple "" "" {
+	    -i $::server_spawn_id
+	    -re "Detaching from process $::decimal\r\n" {
+		exp_continue
+	    }
+	    -re "^$" {}
+	}
+    }
+
+    remote_exec target "kill -USR1 ${pid}"
+    gdb_assert { [target_file_exists_with_timeout $::touch_file_path] } "file exists after detach"
+
+    # Don't leave random files on the target system.
+    if { [is_remote target] } {
+	remote_file target delete $::touch_file_path
+    }
+}
+
+do_test
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c b/gdb/testsuite/gdb.threads/pending-fork-event-detach-touch-file.c
new file mode 100644
index 000000000000..5536381847b1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach-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-detach.c
similarity index 89%
rename from gdb/testsuite/gdb.threads/pending-fork-event.c
rename to gdb/testsuite/gdb.threads/pending-fork-event-detach.c
index a39ca75a49ac..ecfed98fdfda 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.c
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach.c
@@ -32,18 +32,22 @@ break_here (void)
 static void
 do_fork (void)
 {
-  pthread_barrier_wait (&barrier);
-
   while (!release_forking_thread);
 
   if (FORK_FUNCTION () == 0)
-    _exit (0);
+    {
+      /* We create the file in a separate program that we exec: if FORK_FUNCTION
+	 is vfork, we shouldn't do anything more than an exec.  */
+      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-detach.exp b/gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
new file mode 100644
index 000000000000..433660a07ae3
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-detach.exp
@@ -0,0 +1,139 @@
+# 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/>.
+
+# Then, test that if we detach an inferior with a pending fork child, that
+# child is correctly detached and resumes execution normally.  There are two
+# kinds of "pending fork child" we test:
+#
+#   - resulting of a fork catchpoint: we stop at a fork catchpoint and detach.
+#   - resulting of an all-stop stop on top of a non-stop target, where a fork
+#     event is saved as a pending wait status.  To test this, we stepi a thread
+#     while another one forks.  The stepi generally completes at least as fast
+#     as the fork, so we have a chance that the stop due to the stepi being
+#     complete is shown to the user while the fork event is saved for later.
+#
+# To verify that the child process is detached and resumes execution, we have
+# it write a file on the filesystem.  If we don't see the file after a certain
+# delay, it means the child was likely not detached, and the test fails.
+#
+# At the same time, this tests that having this pending fork event does not
+# cause other problems in general.  For example, 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 .c -touch-file.c
+
+set touch_file_bin $binfile-touch-file
+
+if { [is_remote target] } {
+    # If the target is remote, write the file in whatever the current working
+    # directory is, with a somewhat unique name.
+    set touch_file_path ${testfile}-flag
+} else {
+    set touch_file_path [standard_output_file flag]
+}
+
+set opts [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]
+if { [gdb_compile "$srcdir/$subdir/$srcfile2" $touch_file_bin executable $opts] != "" } {
+    return
+}
+
+proc do_test { target-non-stop who_forks fork_function stop_mode } {
+    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.
+    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
+    }
+
+    remote_file target delete $::touch_file_path
+    gdb_assert { ![remote_file target exists $::touch_file_path] } "file does not exist before test"
+
+    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"
+
+    # There are two "pending fork child" modes we can test here:
+    #
+    #   - catch: set up a "catch fork" / "catch vfork" and run to it.
+    #   - stepi: stepi the non-forking thread while the forking thread,
+    #     well, forks.
+    if { $stop_mode == "catch" } {
+	gdb_test "catch fork"
+	gdb_test "catch vfork"
+	gdb_test "continue" "hit Catchpoint $::decimal.*fork.*"
+    } elseif { $stop_mode == "stepi" } {
+	# stepi the non-forking thread.
+	gdb_test "stepi"
+    } else {
+	error "invalid stop_mode value: $stop_mode"
+    }
+
+    # Make sure there's still a single inferior.
+    gdb_test "info inferior" {\* 1 [^\r\n]+}
+
+    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 { [target_file_exists_with_timeout $::touch_file_path] } "file exists after detach"
+
+    # Don't leave random files on the target system.
+    if { [is_remote target] } {
+	remote_file target delete $::touch_file_path
+    }
+}
+
+foreach_with_prefix target-non-stop { auto on off } {
+    foreach_with_prefix who_forks { main other } {
+	foreach_with_prefix fork_function { fork vfork } {
+	    foreach_with_prefix stop_mode { stepi catch } {
+		do_test ${target-non-stop} $who_forks $fork_function $stop_mode
+	    }
+	}
+    }
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
deleted file mode 100644
index 51af07f56bdd..000000000000
--- a/gdb/testsuite/gdb.threads/pending-fork-event.exp
+++ /dev/null
@@ -1,79 +0,0 @@
-# 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.
-#
-# 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/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 70fa2b3a8013..8b7445b75819 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8317,5 +8317,20 @@ proc require { fn arg1 {arg2 ""} } {
     return -code return 0
 }
 
+# Wait up to ::TIMEOUT seconds for file PATH to exist on the target system.
+# Return 1 if it does exist, 0 otherwise.
+
+proc target_file_exists_with_timeout { path } {
+    for {set i 0} {$i < $::timeout} {incr i} {
+	if { [remote_file target exists $path] } {
+	    return 1
+	}
+
+	sleep 1
+    }
+
+    return 0
+}
+
 # Always load compatibility stuff.
 load_lib future.exp
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 34991df449bf..87888044c1f8 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7130,6 +7130,17 @@ linux_process_target::thread_pending_parent (thread_info *thread)
   return get_lwp_thread (parent);
 }
 
+thread_info *
+linux_process_target::thread_pending_child (thread_info *thread)
+{
+  lwp_info *child = get_thread_lwp (thread)->pending_child ();
+
+  if (child == nullptr)
+    return nullptr;
+
+  return get_lwp_thread (child);
+}
+
 /* Default implementation of linux_target_ops method "set_pc" for
    32-bit pc register which is literally named "pc".  */
 
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 819f915ea9a3..b563537216a6 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -312,6 +312,7 @@ class linux_process_target : public process_stratum_target
 #endif
 
   thread_info *thread_pending_parent (thread_info *thread) override;
+  thread_info *thread_pending_child (thread_info *thread) override;
 
   bool supports_catch_syscall () override;
 
@@ -750,6 +751,32 @@ struct lwp_info
     return this->fork_relative;
   }
 
+  /* If this LWP is the parent of a fork child we haven't reported to GDB yet,
+     return that child, else nullptr.  */
+  lwp_info *pending_child () const
+  {
+    if (this->fork_relative == nullptr)
+      return nullptr;
+
+    gdb_assert (this->fork_relative->fork_relative == this);
+
+    /* In a fork parent/child relationship, the parent has a status pending and
+       the child does not, and a thread can only be in one such relationship
+       at most.  So we can recognize who is the parent based on which one has
+       a pending status.  */
+    gdb_assert (!!this->status_pending_p
+		!= !!this->fork_relative->status_pending_p);
+
+    if (!this->status_pending_p)
+      return nullptr;
+
+    const target_waitstatus &ws = this->waitstatus;
+    gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED
+		|| ws.kind () == TARGET_WAITKIND_VFORKED);
+
+    return this->fork_relative;
+  }
+
   /* Backlink to the parent object.  */
   struct thread_info *thread = nullptr;
 
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 8dde6fb07295..27e2aba01214 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 GDB won't take care of detaching it.  We must do it here.
+
+     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.  */
+      thread_info *child = target_thread_pending_child (thread);
+      if (child == nullptr)
+	continue;
+
+      process_info *fork_child_process = get_thread_process (child);
+      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
diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index 136b5104de85..aa3d42462f52 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -841,6 +841,12 @@ process_stratum_target::thread_pending_parent (thread_info *thread)
   return nullptr;
 }
 
+thread_info *
+process_stratum_target::thread_pending_child (thread_info *thread)
+{
+  return nullptr;
+}
+
 bool
 process_stratum_target::supports_software_single_step ()
 {
diff --git a/gdbserver/target.h b/gdbserver/target.h
index 1b0a1201d755..331a21aa57a3 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -492,6 +492,10 @@ class process_stratum_target
      else nullptr.  */
   virtual thread_info *thread_pending_parent (thread_info *thread);
 
+  /* If THREAD is the parent of a fork child that was not reported to GDB,
+     return this child, else nullptr.  */
+  virtual thread_info *thread_pending_child (thread_info *thread);
+
   /* Returns true if the target can software single step.  */
   virtual bool supports_software_single_step ();
 
@@ -708,6 +712,12 @@ target_thread_pending_parent (thread_info *thread)
   return the_target->thread_pending_parent (thread);
 }
 
+static inline thread_info *
+target_thread_pending_child (thread_info *thread)
+{
+  return the_target->thread_pending_child (thread);
+}
+
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
 int set_desired_thread ();
-- 
2.33.1



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent
  2021-12-04  3:36     ` Simon Marchi
@ 2021-12-07 23:25       ` Pedro Alves
  2021-12-08 19:54         ` Simon Marchi
  0 siblings, 1 reply; 20+ messages in thread
From: Pedro Alves @ 2021-12-07 23:25 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches; +Cc: Simon Marchi

Hi Simon,

This LGTM.  Just a couple comments below.

On 2021-12-04 03:36, Simon Marchi wrote:

> +    set wait_time_s 2
> +
> +    if { [info exists server_spawn_id] } {
> +	# Let the program run for 2 seconds, during which it will fork many times.
> +	# When running against GDBserver, this makes server print a ton of
> +	# "Detaching from process X" message, to the point where its output buffer
> +	# gets full and it hangs in a write to stdout.  During these 2 seconds,
> +	# drain the messages from GDBserver to keep that from happening.
> +	set ::pending_fork_detach_ns_stop 0
> +	after [expr $wait_time_s * 1000] { set ::pending_fork_detach_ns_stop 1 }
> +	gdb_test_multiple "" "" {
> +	    -i $::server_spawn_id
> +	    -re "Detaching from process $::decimal\r\n" {
> +		if { !$::pending_fork_detach_ns_stop } {
> +		    exp_continue
> +		}
> +	    }
> +	}

This is fine, but FYI, you don't need "after" for this, if you use
"exp_continue -continue_timer".  The following should do it.  Untested, but
I've used a similar pattern in gdb.multi/multi-term-settings.exp.

	gdb_test_multiple "" "flush server output" {
            -timeout 2

	    -i $::server_spawn_id
	    -re "Detaching from process $::decimal\r\n" {
	       exp_continue -continue_timer
	    }

	    timeout {
	       pass $gdb_test_name
            }
	}

> +    } else {
> +	# Not using GDBserver, just sleep 2 seconds.
> +	sleep $wait_time_s
> +    }
> +
> +    gdb_test "detach" "Detaching from program: .*"
> +
> +    if { [info exists server_spawn_id] } {
> +	# Drain GDBserver's output buffer, in the (unlikely) event that enough
> +	# messages were output to fill the buffer between the moment we stopped
> +	# consuming it and the moment GDBserver detached the process.
> +	gdb_test_multiple "" "" {
> +	    -i $::server_spawn_id
> +	    -re "Detaching from process $::decimal\r\n" {
> +		exp_continue
> +	    }
> +	    -re "^$" {}
> +	}
> +    }

In these drain cases, I wonder whether it wouldn't be better to drain any and all
output.  Like, match ".+".  Imagine something goes wrong and GDBserver printed some
warning or some error or some such instead of the expected "Detaching from" message.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent
  2021-12-07 23:25       ` Pedro Alves
@ 2021-12-08 19:54         ` Simon Marchi
  0 siblings, 0 replies; 20+ messages in thread
From: Simon Marchi @ 2021-12-08 19:54 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches; +Cc: Simon Marchi

On 2021-12-07 6:25 p.m., Pedro Alves wrote:
> Hi Simon,
>
> This LGTM.  Just a couple comments below.
>
> On 2021-12-04 03:36, Simon Marchi wrote:
>
>> +    set wait_time_s 2
>> +
>> +    if { [info exists server_spawn_id] } {
>> +	# Let the program run for 2 seconds, during which it will fork many times.
>> +	# When running against GDBserver, this makes server print a ton of
>> +	# "Detaching from process X" message, to the point where its output buffer
>> +	# gets full and it hangs in a write to stdout.  During these 2 seconds,
>> +	# drain the messages from GDBserver to keep that from happening.
>> +	set ::pending_fork_detach_ns_stop 0
>> +	after [expr $wait_time_s * 1000] { set ::pending_fork_detach_ns_stop 1 }
>> +	gdb_test_multiple "" "" {
>> +	    -i $::server_spawn_id
>> +	    -re "Detaching from process $::decimal\r\n" {
>> +		if { !$::pending_fork_detach_ns_stop } {
>> +		    exp_continue
>> +		}
>> +	    }
>> +	}
>
> This is fine, but FYI, you don't need "after" for this, if you use
> "exp_continue -continue_timer".  The following should do it.  Untested, but
> I've used a similar pattern in gdb.multi/multi-term-settings.exp.

Cool, I converted the test to use that.

> 	gdb_test_multiple "" "flush server output" {
>             -timeout 2
>
> 	    -i $::server_spawn_id
> 	    -re "Detaching from process $::decimal\r\n" {
> 	       exp_continue -continue_timer
> 	    }
>
> 	    timeout {
> 	       pass $gdb_test_name
>             }
> 	}
>
>> +    } else {
>> +	# Not using GDBserver, just sleep 2 seconds.
>> +	sleep $wait_time_s
>> +    }
>> +
>> +    gdb_test "detach" "Detaching from program: .*"
>> +
>> +    if { [info exists server_spawn_id] } {
>> +	# Drain GDBserver's output buffer, in the (unlikely) event that enough
>> +	# messages were output to fill the buffer between the moment we stopped
>> +	# consuming it and the moment GDBserver detached the process.
>> +	gdb_test_multiple "" "" {
>> +	    -i $::server_spawn_id
>> +	    -re "Detaching from process $::decimal\r\n" {
>> +		exp_continue
>> +	    }
>> +	    -re "^$" {}
>> +	}
>> +    }
>
> In these drain cases, I wonder whether it wouldn't be better to drain any and all
> output.  Like, match ".+".  Imagine something goes wrong and GDBserver printed some
> warning or some error or some such instead of the expected "Detaching from" message.

I agree, we are not here to validate the GDBserver output.  I changed
both to ".+".

I'll give it another quick test and push the series if it all looks
good.  Thanks for the reviews!

Simon

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-12-08 19:54 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 14:44 [PATCH v3 0/7] Fix handling of pending fork events Simon Marchi
2021-12-01 14:44 ` [PATCH v3 1/7] gdbserver: hide fork child threads from GDB Simon Marchi
2021-12-03 23:30   ` Pedro Alves
2021-12-01 14:44 ` [PATCH v3 2/7] gdb/linux-nat: factor ptrace-detach code to new detach_one_pid function Simon Marchi
2021-12-03 23:30   ` Pedro Alves
2021-12-01 14:44 ` [PATCH v3 3/7] gdbserver: suppress "Detaching from process" message Simon Marchi
2021-12-03 23:42   ` Pedro Alves
2021-12-04  2:57     ` Simon Marchi
2021-12-01 14:44 ` [PATCH v3 4/7] gdb/remote.c: move some things up Simon Marchi
2021-12-03 23:42   ` Pedro Alves
2021-12-01 14:44 ` [PATCH v3 5/7] gdb/remote.c: refactor pending fork status functions Simon Marchi
2021-12-03 23:43   ` Pedro Alves
2021-12-04  3:03     ` Simon Marchi
2021-12-01 14:44 ` [PATCH v3 6/7] gdb: move clearing of tp->pending_follow to follow_fork_inferior Simon Marchi
2021-12-03 23:43   ` Pedro Alves
2021-12-01 14:45 ` [PATCH v3 7/7] gdb, gdbserver: detach fork child when detaching from fork parent Simon Marchi
2021-12-03 23:44   ` Pedro Alves
2021-12-04  3:36     ` Simon Marchi
2021-12-07 23:25       ` Pedro Alves
2021-12-08 19:54         ` Simon Marchi

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).