[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 6/8] gdbstub: Allow late attachment
From: |
Alex Bennée |
Subject: |
Re: [PATCH v3 6/8] gdbstub: Allow late attachment |
Date: |
Wed, 08 Jan 2025 17:20:52 +0000 |
User-agent: |
mu4e 1.12.8; emacs 29.4 |
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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
- Re: [PATCH v3 6/8] gdbstub: Allow late attachment,
Alex Bennée <=