| Message ID | 20241216123412.77450-7-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub: Allow late attachment | expand |
Ilya Leoshkevich <iii@linux.ibm.com> writes: > Allow debugging individual processes in multi-process applications by > starting them with export QEMU_GDB=/tmp/qemu-%d.sock,suspend=n. > Currently one would have to attach to every process to ensure the app > makes progress. > > In case suspend=n is not specified, the flow remains unchanged. If it > is specified, then accepting the client connection is delegated to a > thread. In the future this machinery may be reused for handling > reconnections and interruptions. > > On accepting a connection, the thread schedules gdb_handlesig() on the > first CPU and wakes it up with host_interrupt_signal. Note that the > result of this gdb_handlesig() invocation is handled, as opposed to > many other existing call sites. These other call sites probably need to > be fixed separately. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > bsd-user/main.c | 1 - > gdbstub/user.c | 120 ++++++++++++++++++++++++++++++++++++++++++---- > linux-user/main.c | 1 - > 3 files changed, 110 insertions(+), 12 deletions(-) > > diff --git a/bsd-user/main.c b/bsd-user/main.c > index 61ca73c4781..ca4773a3f40 100644 > --- a/bsd-user/main.c > +++ b/bsd-user/main.c > @@ -628,7 +628,6 @@ int main(int argc, char **argv) > > if (gdbstub) { > gdbserver_start(gdbstub); > - gdb_handlesig(cpu, 0, NULL, NULL, 0); > } > cpu_loop(env); > /* never exits */ > diff --git a/gdbstub/user.c b/gdbstub/user.c > index c900d0a52fe..6ada0d687b9 100644 > --- a/gdbstub/user.c > +++ b/gdbstub/user.c > @@ -10,6 +10,7 @@ > */ > > #include "qemu/osdep.h" > +#include <sys/syscall.h> Whats this needed for? I can build without it. > #include "qemu/bitops.h" > #include "qemu/cutils.h" > #include "qemu/sockets.h" > @@ -21,6 +22,7 @@ > #include "gdbstub/user.h" > #include "gdbstub/enums.h" > #include "hw/core/cpu.h" > +#include "user/signal.h" > #include "trace.h" > #include "internals.h" > > @@ -416,11 +418,101 @@ static int gdbserver_open_port(int port) > return fd; > } > > -int gdbserver_start(const char *port_or_path) > +static bool gdbserver_accept(int port, int gdb_fd, const char *port_or_path) > { > - int port = g_ascii_strtoull(port_or_path, NULL, 10); > + bool ret; > + > + if (port > 0) { > + ret = gdb_accept_tcp(gdb_fd); > + } else { > + ret = gdb_accept_socket(gdb_fd); > + if (ret) { > + gdbserver_user_state.socket_path = g_strdup(port_or_path); > + } > + } > + > + if (!ret) { > + close(gdb_fd); > + } > + > + return ret; > +} Is the clean-up worth it here for the extra level of indirection. Is the string even port_or_path at this point because if it was a port the int port > 0? > + > +struct { > + int port; > int gdb_fd; > + char *port_or_path; > +} gdbserver_args; > + > +static void do_gdb_handlesig(CPUState *cs, run_on_cpu_data arg) > +{ > + int sig; > + > + sig = target_to_host_signal(gdb_handlesig(cs, 0, NULL, NULL, 0)); > + if (sig >= 1 && sig < NSIG) { > + qemu_kill_thread(gdb_get_cpu_index(cs), sig); > + } > +} > + > +static void *gdbserver_accept_thread(void *arg) > +{ > + if (gdbserver_accept(gdbserver_args.port, gdbserver_args.gdb_fd, > + gdbserver_args.port_or_path)) { > + CPUState *cs = first_cpu; > + > + async_safe_run_on_cpu(cs, do_gdb_handlesig, RUN_ON_CPU_NULL); > + qemu_kill_thread(gdb_get_cpu_index(cs), host_interrupt_signal); > + } > + > + g_free(gdbserver_args.port_or_path); Should we set gdbserver_args.port_or_path = NULL here to avoid trying to reference or free it again? > + > + return NULL; > +} > + > +__attribute__((__format__(__printf__, 1, 2))) > +static void print_usage(const char *format, ...) > +{ > + va_list ap; > + > + va_start(ap, format); > + vfprintf(stderr, format, ap); > + va_end(ap); > + fprintf(stderr, "Usage: -g {port|path}[,suspend={y|n}]\n"); > +} > + > +#define SUSPEND "suspend=" > + > +int gdbserver_start(const char *args) > +{ > + g_auto(GStrv) argv = g_strsplit(args, ",", 0); > + const char *port_or_path = NULL; > + bool suspend = true; > + int gdb_fd, port; > + GStrv arg; > > + for (arg = argv; *arg; arg++) { > + if (g_str_has_prefix(*arg, SUSPEND)) { > + const char *val = *arg + strlen(SUSPEND); > + > + suspend = (strcmp(val, "y") == 0); > + if (!suspend && (strcmp(val, "n") != 0)) { > + print_usage("Bad option value: %s", *arg); > + return -1; > + } > + } else { > + if (port_or_path) { > + print_usage("Unknown option: %s", *arg); > + return -1; > + } > + port_or_path = *arg; > + } > + } We have some useful utility functions to parsing all the bools, something like: for (arg = argv; *arg; arg++) { g_auto(GStrv) tokens = g_strsplit(*arg, "=", 2); if (g_strcmp0(tokens[0], "suspend") == 0 && tokens[1]) { qapi_bool_parse(tokens[0], tokens[1], &suspend, &error_fatal); } else { if (port_or_path) { print_usage("Unknown option: %s", *arg); return -1; } port_or_path = *arg; } } which also avoids the #define and strlen messing about as well. (side note to no one in particular: should qapi_bool_parse being using g_strcmp0()?) > + if (!port_or_path) { > + print_usage("Port or path not specified"); > + return -1; > + } > + > + port = g_ascii_strtoull(port_or_path, NULL, 10); > if (port > 0) { > gdb_fd = gdbserver_open_port(port); > } else { > @@ -431,16 +523,24 @@ int gdbserver_start(const char *port_or_path) > return -1; > } > > - if (port > 0 && gdb_accept_tcp(gdb_fd)) { > - return 0; > - } else if (gdb_accept_socket(gdb_fd)) { > - gdbserver_user_state.socket_path = g_strdup(port_or_path); > + if (suspend) { > + if (gdbserver_accept(port, gdb_fd, port_or_path)) { > + gdb_handlesig(first_cpu, 0, NULL, NULL, 0); > + return 0; > + } else { > + return -1; > + } > + } else { > + QemuThread thread; > + > + gdbserver_args.port = port; > + gdbserver_args.gdb_fd = gdb_fd; > + gdbserver_args.port_or_path = g_strdup(port_or_path); > + qemu_thread_create(&thread, "gdb-accept", > + &gdbserver_accept_thread, NULL, > + QEMU_THREAD_DETACHED); > return 0; > } > - > - /* gone wrong */ > - close(gdb_fd); > - return -1; > } Not a problem with this patch in particular but it seems to me gdbserver_start should probably look like: bool gdbserver_start(const char *args, Error **errp) so we can do the right thing when starting from the command line or via HMP. I'll see if I can clean that up on gdbstub/next. > > void gdbserver_fork_start(void) > diff --git a/linux-user/main.c b/linux-user/main.c > index b09af8d4365..97245ab37c2 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -1027,7 +1027,6 @@ int main(int argc, char **argv, char **envp) > gdbstub); > exit(EXIT_FAILURE); > } > - gdb_handlesig(cpu, 0, NULL, NULL, 0); > } > > #ifdef CONFIG_SEMIHOSTING
On Wed, 2025-01-08 at 17:20 +0000, Alex Bennée wrote: > Ilya Leoshkevich <iii@linux.ibm.com> writes: > > > Allow debugging individual processes in multi-process applications > > by > > starting them with export QEMU_GDB=/tmp/qemu-%d.sock,suspend=n. > > Currently one would have to attach to every process to ensure the > > app > > makes progress. > > > > In case suspend=n is not specified, the flow remains unchanged. If > > it > > is specified, then accepting the client connection is delegated to > > a > > thread. In the future this machinery may be reused for handling > > reconnections and interruptions. > > > > On accepting a connection, the thread schedules gdb_handlesig() on > > the > > first CPU and wakes it up with host_interrupt_signal. Note that the > > result of this gdb_handlesig() invocation is handled, as opposed to > > many other existing call sites. These other call sites probably > > need to > > be fixed separately. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > bsd-user/main.c | 1 - > > gdbstub/user.c | 120 > > ++++++++++++++++++++++++++++++++++++++++++---- > > linux-user/main.c | 1 - > > 3 files changed, 110 insertions(+), 12 deletions(-) > > > > diff --git a/bsd-user/main.c b/bsd-user/main.c > > index 61ca73c4781..ca4773a3f40 100644 > > --- a/bsd-user/main.c > > +++ b/bsd-user/main.c > > @@ -628,7 +628,6 @@ int main(int argc, char **argv) > > > > if (gdbstub) { > > gdbserver_start(gdbstub); > > - gdb_handlesig(cpu, 0, NULL, NULL, 0); > > } > > cpu_loop(env); > > /* never exits */ > > diff --git a/gdbstub/user.c b/gdbstub/user.c > > index c900d0a52fe..6ada0d687b9 100644 > > --- a/gdbstub/user.c > > +++ b/gdbstub/user.c > > @@ -10,6 +10,7 @@ > > */ > > > > #include "qemu/osdep.h" > > +#include <sys/syscall.h> > > Whats this needed for? I can build without it. Must be a leftover - probably I was calling gettid() in one of the intermediate versions? I don't see a need for it anymore and will drop it in v4. > > #include "qemu/bitops.h" > > #include "qemu/cutils.h" > > #include "qemu/sockets.h" > > @@ -21,6 +22,7 @@ > > #include "gdbstub/user.h" > > #include "gdbstub/enums.h" > > #include "hw/core/cpu.h" > > +#include "user/signal.h" > > #include "trace.h" > > #include "internals.h" > > > > @@ -416,11 +418,101 @@ static int gdbserver_open_port(int port) > > return fd; > > } > > > > -int gdbserver_start(const char *port_or_path) > > +static bool gdbserver_accept(int port, int gdb_fd, const char > > *port_or_path) > > { > > - int port = g_ascii_strtoull(port_or_path, NULL, 10); > > + bool ret; > > + > > + if (port > 0) { > > + ret = gdb_accept_tcp(gdb_fd); > > + } else { > > + ret = gdb_accept_socket(gdb_fd); > > + if (ret) { > > + gdbserver_user_state.socket_path = > > g_strdup(port_or_path); > > + } > > + } > > + > > + if (!ret) { > > + close(gdb_fd); > > + } > > + > > + return ret; > > +} > > Is the clean-up worth it here for the extra level of indirection. Is > the > string even port_or_path at this point because if it was a port the > int > port > 0? The extra level of indirection arises because of using a thread. I think port_or_path is indeed always a path; I will rename it. > > + > > +struct { > > + int port; > > int gdb_fd; > > + char *port_or_path; > > +} gdbserver_args; > > + > > +static void do_gdb_handlesig(CPUState *cs, run_on_cpu_data arg) > > +{ > > + int sig; > > + > > + sig = target_to_host_signal(gdb_handlesig(cs, 0, NULL, NULL, > > 0)); > > + if (sig >= 1 && sig < NSIG) { > > + qemu_kill_thread(gdb_get_cpu_index(cs), sig); > > + } > > +} > > + > > +static void *gdbserver_accept_thread(void *arg) > > +{ > > + if (gdbserver_accept(gdbserver_args.port, > > gdbserver_args.gdb_fd, > > + gdbserver_args.port_or_path)) { > > + CPUState *cs = first_cpu; > > + > > + async_safe_run_on_cpu(cs, do_gdb_handlesig, > > RUN_ON_CPU_NULL); > > + qemu_kill_thread(gdb_get_cpu_index(cs), > > host_interrupt_signal); > > + } > > + > > + g_free(gdbserver_args.port_or_path); > > Should we set gdbserver_args.port_or_path = NULL here to avoid trying > to > reference or free it again? Sounds good, will do. > > + > > + return NULL; > > +} > > + > > +__attribute__((__format__(__printf__, 1, 2))) > > +static void print_usage(const char *format, ...) > > +{ > > + va_list ap; > > + > > + va_start(ap, format); > > + vfprintf(stderr, format, ap); > > + va_end(ap); > > + fprintf(stderr, "Usage: -g {port|path}[,suspend={y|n}]\n"); > > +} > > + > > +#define SUSPEND "suspend=" > > + > > +int gdbserver_start(const char *args) > > +{ > > + g_auto(GStrv) argv = g_strsplit(args, ",", 0); > > + const char *port_or_path = NULL; > > + bool suspend = true; > > + int gdb_fd, port; > > + GStrv arg; > > > > + for (arg = argv; *arg; arg++) { > > + if (g_str_has_prefix(*arg, SUSPEND)) { > > + const char *val = *arg + strlen(SUSPEND); > > + > > + suspend = (strcmp(val, "y") == 0); > > + if (!suspend && (strcmp(val, "n") != 0)) { > > + print_usage("Bad option value: %s", *arg); > > + return -1; > > + } > > + } else { > > + if (port_or_path) { > > + print_usage("Unknown option: %s", *arg); > > + return -1; > > + } > > + port_or_path = *arg; > > + } > > + } > > We have some useful utility functions to parsing all the bools, > something like: > > for (arg = argv; *arg; arg++) { > g_auto(GStrv) tokens = g_strsplit(*arg, "=", 2); > if (g_strcmp0(tokens[0], "suspend") == 0 && tokens[1]) { > qapi_bool_parse(tokens[0], tokens[1], &suspend, > &error_fatal); > } else { > if (port_or_path) { > print_usage("Unknown option: %s", *arg); > return -1; > } > port_or_path = *arg; > } > } > > which also avoids the #define and strlen messing about as well. Looks good, I will adopt it (I think I'll swap error_fatal for warn_report_err() though). > > (side note to no one in particular: should qapi_bool_parse being > using g_strcmp0()?) Makes sense, with the current approach I get Unknown option: suspend for QEMU_GDB=/tmp/1234,suspend which is somewhat puzzling. I will add the change to v4. > > > + if (!port_or_path) { > > + print_usage("Port or path not specified"); > > + return -1; > > + } > > + > > + port = g_ascii_strtoull(port_or_path, NULL, 10); > > if (port > 0) { > > gdb_fd = gdbserver_open_port(port); > > } else { > > @@ -431,16 +523,24 @@ int gdbserver_start(const char *port_or_path) > > return -1; > > } > > > > - if (port > 0 && gdb_accept_tcp(gdb_fd)) { > > - return 0; > > - } else if (gdb_accept_socket(gdb_fd)) { > > - gdbserver_user_state.socket_path = g_strdup(port_or_path); > > + if (suspend) { > > + if (gdbserver_accept(port, gdb_fd, port_or_path)) { > > + gdb_handlesig(first_cpu, 0, NULL, NULL, 0); > > + return 0; > > + } else { > > + return -1; > > + } > > + } else { > > + QemuThread thread; > > + > > + gdbserver_args.port = port; > > + gdbserver_args.gdb_fd = gdb_fd; > > + gdbserver_args.port_or_path = g_strdup(port_or_path); > > + qemu_thread_create(&thread, "gdb-accept", > > + &gdbserver_accept_thread, NULL, > > + QEMU_THREAD_DETACHED); > > return 0; > > } > > - > > - /* gone wrong */ > > - close(gdb_fd); > > - return -1; > > } > > Not a problem with this patch in particular but it seems to me > gdbserver_start should probably look like: > > bool gdbserver_start(const char *args, Error **errp) > > so we can do the right thing when starting from the command line or > via > HMP. I'll see if I can clean that up on gdbstub/next. I like this; in particular, I've added the usage of unix_listen(), which uses Error, and for now I have to handle it using warn_report_err() - just forwarding it up the stack would be much nicer. > > > > void gdbserver_fork_start(void) > > diff --git a/linux-user/main.c b/linux-user/main.c > > index b09af8d4365..97245ab37c2 100644 > > --- a/linux-user/main.c > > +++ b/linux-user/main.c > > @@ -1027,7 +1027,6 @@ int main(int argc, char **argv, char **envp) > > gdbstub); > > exit(EXIT_FAILURE); > > } > > - gdb_handlesig(cpu, 0, NULL, NULL, 0); > > } > > > > #ifdef CONFIG_SEMIHOSTING >
diff --git a/bsd-user/main.c b/bsd-user/main.c index 61ca73c4781..ca4773a3f40 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -628,7 +628,6 @@ int main(int argc, char **argv) if (gdbstub) { gdbserver_start(gdbstub); - gdb_handlesig(cpu, 0, NULL, NULL, 0); } cpu_loop(env); /* never exits */ diff --git a/gdbstub/user.c b/gdbstub/user.c index c900d0a52fe..6ada0d687b9 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" @@ -21,6 +22,7 @@ #include "gdbstub/user.h" #include "gdbstub/enums.h" #include "hw/core/cpu.h" +#include "user/signal.h" #include "trace.h" #include "internals.h" @@ -416,11 +418,101 @@ static int gdbserver_open_port(int port) return fd; } -int gdbserver_start(const char *port_or_path) +static bool gdbserver_accept(int port, int gdb_fd, const char *port_or_path) { - int port = g_ascii_strtoull(port_or_path, NULL, 10); + bool ret; + + if (port > 0) { + ret = gdb_accept_tcp(gdb_fd); + } else { + ret = gdb_accept_socket(gdb_fd); + if (ret) { + gdbserver_user_state.socket_path = g_strdup(port_or_path); + } + } + + if (!ret) { + close(gdb_fd); + } + + return ret; +} + +struct { + int port; int gdb_fd; + char *port_or_path; +} gdbserver_args; + +static void do_gdb_handlesig(CPUState *cs, run_on_cpu_data arg) +{ + int sig; + + sig = target_to_host_signal(gdb_handlesig(cs, 0, NULL, NULL, 0)); + if (sig >= 1 && sig < NSIG) { + qemu_kill_thread(gdb_get_cpu_index(cs), sig); + } +} + +static void *gdbserver_accept_thread(void *arg) +{ + if (gdbserver_accept(gdbserver_args.port, gdbserver_args.gdb_fd, + gdbserver_args.port_or_path)) { + CPUState *cs = first_cpu; + + async_safe_run_on_cpu(cs, do_gdb_handlesig, RUN_ON_CPU_NULL); + qemu_kill_thread(gdb_get_cpu_index(cs), host_interrupt_signal); + } + + g_free(gdbserver_args.port_or_path); + + return NULL; +} + +__attribute__((__format__(__printf__, 1, 2))) +static void print_usage(const char *format, ...) +{ + va_list ap; + + va_start(ap, format); + vfprintf(stderr, format, ap); + va_end(ap); + fprintf(stderr, "Usage: -g {port|path}[,suspend={y|n}]\n"); +} + +#define SUSPEND "suspend=" + +int gdbserver_start(const char *args) +{ + g_auto(GStrv) argv = g_strsplit(args, ",", 0); + const char *port_or_path = NULL; + bool suspend = true; + int gdb_fd, port; + GStrv arg; + for (arg = argv; *arg; arg++) { + if (g_str_has_prefix(*arg, SUSPEND)) { + const char *val = *arg + strlen(SUSPEND); + + suspend = (strcmp(val, "y") == 0); + if (!suspend && (strcmp(val, "n") != 0)) { + print_usage("Bad option value: %s", *arg); + return -1; + } + } else { + if (port_or_path) { + print_usage("Unknown option: %s", *arg); + return -1; + } + port_or_path = *arg; + } + } + if (!port_or_path) { + print_usage("Port or path not specified"); + return -1; + } + + port = g_ascii_strtoull(port_or_path, NULL, 10); if (port > 0) { gdb_fd = gdbserver_open_port(port); } else { @@ -431,16 +523,24 @@ int gdbserver_start(const char *port_or_path) return -1; } - if (port > 0 && gdb_accept_tcp(gdb_fd)) { - return 0; - } else if (gdb_accept_socket(gdb_fd)) { - gdbserver_user_state.socket_path = g_strdup(port_or_path); + if (suspend) { + if (gdbserver_accept(port, gdb_fd, port_or_path)) { + gdb_handlesig(first_cpu, 0, NULL, NULL, 0); + return 0; + } else { + return -1; + } + } else { + QemuThread thread; + + gdbserver_args.port = port; + gdbserver_args.gdb_fd = gdb_fd; + gdbserver_args.port_or_path = g_strdup(port_or_path); + qemu_thread_create(&thread, "gdb-accept", + &gdbserver_accept_thread, NULL, + QEMU_THREAD_DETACHED); return 0; } - - /* gone wrong */ - close(gdb_fd); - return -1; } void gdbserver_fork_start(void) diff --git a/linux-user/main.c b/linux-user/main.c index b09af8d4365..97245ab37c2 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -1027,7 +1027,6 @@ int main(int argc, char **argv, char **envp) gdbstub); exit(EXIT_FAILURE); } - gdb_handlesig(cpu, 0, NULL, NULL, 0); } #ifdef CONFIG_SEMIHOSTING
Allow debugging individual processes in multi-process applications by starting them with export QEMU_GDB=/tmp/qemu-%d.sock,suspend=n. Currently one would have to attach to every process to ensure the app makes progress. In case suspend=n is not specified, the flow remains unchanged. If it is specified, then accepting the client connection is delegated to a thread. In the future this machinery may be reused for handling reconnections and interruptions. On accepting a connection, the thread schedules gdb_handlesig() on the first CPU and wakes it up with host_interrupt_signal. Note that the result of this gdb_handlesig() invocation is handled, as opposed to many other existing call sites. These other call sites probably need to be fixed separately. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- bsd-user/main.c | 1 - gdbstub/user.c | 120 ++++++++++++++++++++++++++++++++++++++++++---- linux-user/main.c | 1 - 3 files changed, 110 insertions(+), 12 deletions(-)