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>
#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
--
2.47.0Ilya 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
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
>
© 2016 - 2025 Red Hat, Inc.