gdbserver: hide fork child threads from GDB
authorSimon Marchi <simon.marchi@polymtl.ca>
Tue, 28 Sep 2021 20:58:26 +0000 (16:58 -0400)
committerSimon Marchi <simon.marchi@efficios.com>
Thu, 28 Oct 2021 20:18:31 +0000 (16:18 -0400)
commitedf56335c4626a443d3cdf420dd40521f658fa8e
treeb4c1f456b8389c8f8955bc6a2f50bbacc070ea23
parentfdc4bb36dc78ced60d244e02911d5a2156b7926c
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, and that other
is reported to GDB (and the fork event stays pending in GDBserver).
This happens for example when we step a thread and another 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
 - 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.
 - 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.

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 code
yet, so are still lingering as pending stop replies in the remote target
(see remove_new_fork_children in remote.c).  But in our case, the fork
event hasn't made its way to the GDB-side remote target.  We need to
implement the same kind of logic GDBserver-side: if there exists a
thread / inferior that is the result of a fork event GDBserver hasn't
reported yet, it should exclude that thread / inferior from the reported
thread list.

This was actually discussed a while ago, but not implemented AFAIK:

    https://pi.simark.ca/gdb-patches/1ad9f5a8-d00e-9a26-b0c9-3f4066af5142@redhat.com/#t
    https://sourceware.org/pipermail/gdb-patches/2016-June/133906.html

Implementation details-wise, the fix for this is all in GDBserver.  The
Linux layer of GDBserver already tracks unreported fork parent / child
relationships using the lwp_info::fork_relative, in order to avoid
wildcard actions resuming fork childs unknown to GDB.  Move this
information to the common thread_info structure, so that it can be used
in handle_qxfer_threads_worker to filter out unreported fork child
threads.  Plus, that makes it usable for other OSes that use fork if
need be.

I split the field in two (fork_parent / fork_child), I think it's
clearer this way, easier to follow which thread is the parent and which
is the child, and helps ensure things are consistent.  That simplifies
things a bit in linux_set_resume_request.  Currently, when
lwp->fork_relative is set, we have to deduce whether this is a parent or
child using the pending event.  With separate fork_parent and fork_child
fields, it becomes more obvious.  If the thread has a fork parent, then
it means it's a fork child, and vice-versa.

Testing-wise, the test replicates pretty-much the sequence of events
shown above.  The setup of the test makes it such that the main thread
is about to fork.  We stepi the other thread, so that the step completes
very quickly, in a single event.  Meanwhile, the main thread is resumed,
so very likely has time to call fork.  This means that the bug may not
reproduce every time (if the main thread does not have time to call
fork), but it will reproduce more often than not.  The test fails
without the fix applied on the native-gdbserver and
native-extended-gdbserver boards.

At some point I suspected that which thread called fork and which thread
did the step influenced the order in which the events were reported, and
therefore the reproducibility of the bug.  So I made the test try  both
combinations: main thread forks while other thread steps, and vice
versa.  I'm not sure this is still necessary, but I left it there
anyway.  It doesn't hurt to test a few more combinations.

Change-Id: I2158d5732fc7d7ca06b0eb01f88cf27bf527b990
gdb/testsuite/gdb.threads/pending-fork-event.c [new file with mode: 0644]
gdb/testsuite/gdb.threads/pending-fork-event.exp [new file with mode: 0644]
gdbserver/gdbthread.h
gdbserver/linux-low.cc
gdbserver/linux-low.h
gdbserver/server.cc
This page took 0.026156 seconds and 4 git commands to generate.