Re: [PATCH 2/3] gdbstub: Implement follow-fork-mode child

[Ilya Leoshkevich <iii@linux.ibm.com>]

On Thu, 2024-02-01 at 12:11 +0000, Alex Bennée wrote:
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
> 
> > Currently it's not possible to use gdbstub for debugging linux-user
> > code that runs in a forked child, which is normally done using the
> > `set
> > follow-fork-mode child` GDB command. Purely on the protocol level,
> > the
> > missing piece is the fork-events feature.
> > 
> > However, a deeper problem is supporting $Hg switching between
> > different
> > processes - right now it can do only threads. Implementing this for
> > the
> > general case would be quite complicated, but, fortunately, for the
> > follow-fork-mode case there are a few factors that greatly simplify
> > things: fork() happens in the exclusive section, there are only two
> > processes involved, and before one of them is resumed, the second
> > one
> > is detached.
> > 
> > This makes it possible to implement a simplified scheme: the parent
> > and
> > the child share the gdbserver socket, it's used only by one of them
> > at
> > any given time, which is coordinated through a separate socketpair.
> > The
> > processes can read from the gdbserver socket only one byte at a
> > time,
> > which is not great for performance, but, fortunately, the
> > follow-fork-mode involves only a few messages.
> > 
> > Add the hooks for the user-specific handling of $qSupported, $Hg,
> > and
> > $D. Advertise the fork-events support, and remember whether GDB has
> > it
> > as well. Implement the state machine that is initialized on fork(),
> > decides the current owner of the gdbserver socket, and is
> > terminated
> > when one of the two processes is detached. The logic for the parent
> > and
> > the child is the same, only the initial state is different.
> > 
> > Handle the `stepi` of a syscall corner case by disabling the
> > single-stepping in detached processes.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  gdbstub/gdbstub.c   |  29 ++++--
> >  gdbstub/internals.h |   3 +
> >  gdbstub/user.c      | 210
> > +++++++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 234 insertions(+), 8 deletions(-)

[...]
 
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 120eb7fc117..962f4cb74e7 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -10,6 +10,7 @@
> >   */
> >  
> >  #include "qemu/osdep.h"
> > +#include <sys/syscall.h>
> >  #include "qemu/bitops.h"
> >  #include "qemu/cutils.h"
> >  #include "qemu/sockets.h"
> > @@ -25,6 +26,41 @@
> >  #define GDB_NR_SYSCALLS 1024
> >  typedef unsigned long
> > GDBSyscallsMask[BITS_TO_LONGS(GDB_NR_SYSCALLS)];
> >  
> > +/*
> > + * Forked child talks to its parent in order to let GDB enforce
> > the
> > + * follow-fork-mode. This happens inside a start_exclusive()
> > section, so that
> > + * the other threads, which may be forking too, do not interfere.
> > The
> > + * implementation relies on GDB not sending $vCont until it has
> > detached
> > + * either from the parent (follow-fork-mode child) or from the
> > child
> > + * (follow-fork-mode parent).
> > + *
> > + * The parent and the child share the GDB socket; at any given
> > time only one
> > + * of them is allowed to use it, as is reflected in the respective
> > fork_state.
> > + * This is negotiated via the fork_sockets pair as a reaction to
> > $Hg.
> > + */
> > +enum GDBForkState {
> > +    /* Fully owning the GDB socket. */
> > +    GDB_FORK_ENABLED,
> > +    /* Working with the GDB socket; the peer is inactive. */
> > +    GDB_FORK_ACTIVE,
> > +    /* Handing off the GDB socket to the peer. */
> > +    GDB_FORK_DEACTIVATING,
> > +    /* The peer is working with the GDB socket. */
> > +    GDB_FORK_INACTIVE,
> > +    /* Asking the peer to close its GDB socket fd. */
> > +    GDB_FORK_ENABLING,
> > +    /* Asking the peer to take over, closing our GDB socket fd. */
> > +    GDB_FORK_DISABLING,
> > +    /* The peer has taken over, our GDB socket fd is closed. */
> > +    GDB_FORK_DISABLED,
> > +};
> 
> gulp - thats a potentially fairly complex state diagram. Do we just
> work
> through the states sequentially?

Unfortunately no. I had less states at some point, but then realized
it was better to have these things laid out explicitly. Let me try to
summarize the possible transitions:

GDB_FORK_ENABLED: Terminal state; GDB follows the current process.
GDB_FORK_DISABLED: Terminal state; GDB follows the other process.
GDB_FORK_ACTIVE -> GDB_FORK_DEACTIVATING: On $Hg.
GDB_FORK_ACTIVE -> GDB_FORK_ENABLING: On $D.
GDB_FORK_ACTIVE -> GDB_FORK_DISABLING: On $D.
GDB_FORK_ACTIVE -> GDB_FORK_DISABLED: On communication error.
GDB_FORK_DEACTIVATING -> GDB_FORK_INACTIVE: On gdb_read_byte() return.
GDB_FORK_DEACTIVATING -> GDB_FORK_DISABLED: On communication error.
GDB_FORK_INACTIVE -> GDB_FORK_ACTIVE: On $Hg in peer.
GDB_FORK_INACTIVE -> GDB_FORK_ENABLE: On $D in peer.
GDB_FORK_INACTIVE -> GDB_FORK_DISABLE: On $D in peer.
GDB_FORK_INACTIVE -> GDB_FORK_DISABLED: On communication error.
GDB_FORK_ENABLING -> GDB_FORK_ENABLED: On gdb_read_byte() return.
GDB_FORK_ENABLING -> GDB_FORK_DISABLED: On communication error.
GDB_FORK_DISABLING -> GDB_FORK_DISABLED: On gdb_read_byte() return.

Some states have only one meaningful transition:

GDB_FORK_DEACTIVATING -> GDB_FORK_INACTIVE
GDB_FORK_ENABLING -> GDB_FORK_ENABLED

and can in theory be squashed, but then the socketpair communication
would have to be moved to the respective user-hook, which would
complicate the error handling.

[...]

> > @@ -369,16 +421,168 @@ static void disable_gdbstub(void)
> >      CPU_FOREACH(cpu) {
> >          cpu_breakpoint_remove_all(cpu, BP_GDB);
> >          /* no cpu_watchpoint_remove_all for user-mode */
> > +        cpu_single_step(cpu, 0);
> > +        tb_flush(cpu);
> >      }
> >  }
> >  
> > -/* Disable gdb stub for child processes.  */
> >  void gdbserver_fork_end(pid_t pid)
> >  {
> > -    if (pid != 0 || !gdbserver_state.init ||
> > gdbserver_user_state.fd < 0) {
> > +    char b;
> > +    int fd;
> > +
> > +    if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> > +        return;
> > +    }
> > +
> > +    if (pid == -1) {
> > +        if (gdbserver_user_state.fork_state != GDB_FORK_DISABLED)
> > {
> > +            g_assert(gdbserver_user_state.fork_state ==
> > GDB_FORK_INACTIVE);
> > +            close(gdbserver_user_state.fork_sockets[0]);
> > +            close(gdbserver_user_state.fork_sockets[1]);
> > +        }
> >          return;
> >      }
> > -    disable_gdbstub();
> > +
> > +    if (gdbserver_user_state.fork_state == GDB_FORK_DISABLED) {
> > +        if (pid == 0) {
> > +            disable_gdbstub();
> > +        }
> > +        return;
> > +    }
> > +
> > +    if (pid == 0) {
> > +        close(gdbserver_user_state.fork_sockets[0]);
> > +        fd = gdbserver_user_state.fork_sockets[1];
> > +        g_assert(gdbserver_state.process_num == 1);
> > +        g_assert(gdbserver_state.processes[0].pid ==
> > +                     gdbserver_user_state.fork_peer_pid);
> > +        g_assert(gdbserver_state.processes[0].attached);
> > +        gdbserver_state.processes[0].pid = getpid();
> > +    } else {
> > +        close(gdbserver_user_state.fork_sockets[1]);
> > +        fd = gdbserver_user_state.fork_sockets[0];
> > +        gdbserver_user_state.fork_state = GDB_FORK_ACTIVE;
> > +        gdbserver_user_state.fork_peer_pid = pid;
> > +        gdbserver_user_state.fork_peer_tid = pid;
> > +
> > +        if (!gdbserver_state.allow_stop_reply) {
> > +            goto fail;
> > +        }
> > +        g_string_printf(gdbserver_state.str_buf,
> > +                        "T%02xfork:p%02x.%02x;thread:p%02x.%02x;",
> > +                       
> > gdb_target_signal_to_gdb(gdb_target_sigtrap()),
> > +                        pid, pid, (int)getpid(),
> > qemu_get_thread_id());
> 
> I don't think I messed up the merge but:
> 
> ../../gdbstub/user.c: In function ‘gdbserver_fork_end’:
> ../../gdbstub/user.c:461:50: error: implicit declaration of function
> ‘gdb_target_sigtrap’ [-Werror=implicit-function-declaration]
>   461 |                        
> gdb_target_signal_to_gdb(gdb_target_sigtrap()),
>       |                                                 
> ^~~~~~~~~~~~~~~~~~
> ../../gdbstub/user.c:461:50: error: nested extern declaration of
> ‘gdb_target_sigtrap’ [-Werror=nested-externs]
> cc1: all warnings being treated as errors
> 
> I cant see where gdb_target_sigtrap is from?

This is from [1], which this series is Based-on. I can make one series
with both features if it's more convenient to review.

[1] https://patchew.org/QEMU/20240116094411.216665-1-iii@linux.ibm.com/