Ilya Leoshkevich <[email protected]> writes: > On Thu, 2024-02-01 at 12:11 +0000, Alex Bennée wrote: >> Ilya Leoshkevich <[email protected]> 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 <[email protected]> >> > --- >> > 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/[email protected]/
Oops missed that one, looking at it now. -- Alex Bennée Virtualisation Tech Lead @ Linaro
