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

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> 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/

Oops missed that one, looking at it now.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro