Re: [PATCH v3 6/8] gdbstub: Allow late attachment

[Ilya Leoshkevich <iii@linux.ibm.com>]

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
>