gdbserver: hide fork child threads from GDB

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

diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
new file mode 100644 (file)
index 0000000..a39ca75
--- /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 (file)
index 0000000..51af07f
--- /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 8bf24031205ae8232820dce6384b295de3cddc51..3c68c57384be38c83ac4c0190eb417aa4ba80d1f 100644 (file)
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7125,6 +7125,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 106d4893312833bebb26e62ec27af36dae64b0c8..6e4549fab2cfee52a26870a36cbffd79da963111 100644 (file)
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -311,6 +311,8 @@ public:
                      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
@@ -726,6 +728,33 @@ struct lwp_info
     this->waitstatus.kind = TARGET_WAITKIND_IGNORE;
   }
 
+  /* 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 97854e71b45996b1c7c6df0b970bce8a58bc0aeb..725fb3b757e271087f8199d00f6d64c11e6d8853 100644 (file)
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1657,6 +1657,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 451961001553502b6a573d8d32a585bc7ea1e140..585503e41b128151a29b3eb7cc1652adf462d196 100644 (file)
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -838,6 +838,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 3bfbd7ddbe36019c307a333cc615d92a78176a64..e557f41cbdf3ca4fed38191754a766aef4ca4fa4 100644 (file)
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -488,6 +488,10 @@ public:
   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 ();