[PATCH 0/8] gdbstub: Allow late attachment

[PATCH 0/8] gdbstub: Allow late attachment

[Ilya Leoshkevich]

Hi,

This series adds the ability to attach GDB to a running qemu-user
instance. This is useful for debugging multi-process apps.

Patches 1 and 2 implement a small related feature: the ability to
create individual UNIX sockets for each child process.

Patches 3-5 add the required infrastructure. In particular, we need
to reserve a host signal for waking up threads, as discussed in [1].
By the way, the problem with atomicity of checking for pending signals
and invoking syscalls that I'm describing in that thread seems to
have already been solved by the safe_syscall infrastructure, so the
changes are fairly simple.

If this series is accepted, I will rebase the all-stop series on top
of it.

Patch 6 is the implementation, patch 7 is a documentation update,
patch 8 is a test. I tested this series on Linux and only
compile-tested on the BSDs.

Best regards,
Ilya

[1] https://lore.kernel.org/qemu-devel/94ebebf2-e775-4fd2-8fcf-921610261a7e@linaro.org/

Ilya Leoshkevich (8):
  gdbstub: Allow the %d placeholder in the socket path
  gdbstub: Try unlinking the unix socket before binding
  user: Introduce user/signal.h
  user: Introduce host_interrupt_signal
  osdep: Introduce qemu_kill_thread()
  gdbstub: Allow late attachment
  docs/user: Document the %d placeholder and suspend=n QEMU_GDB features
  tests/tcg: Add late gdbstub attach test

 bsd-user/main.c                            |   1 -
 bsd-user/signal-common.h                   |   1 -
 bsd-user/signal.c                          |  13 ++
 docs/user/main.rst                         |  16 ++-
 gdbstub/user.c                             | 131 +++++++++++++++++++--
 include/qemu/osdep.h                       |   9 ++
 include/user/signal.h                      |  25 ++++
 linux-user/main.c                          |   1 -
 linux-user/signal-common.h                 |   1 -
 linux-user/signal.c                        |  12 ++
 linux-user/syscall.c                       |   1 +
 tests/guest-debug/run-test.py              |  15 ++-
 tests/tcg/multiarch/Makefile.target        |   9 +-
 tests/tcg/multiarch/gdbstub/late-attach.py |  28 +++++
 tests/tcg/multiarch/late-attach.c          |  41 +++++++
 util/oslib-posix.c                         |  15 +++
 16 files changed, 299 insertions(+), 20 deletions(-)
 create mode 100644 include/user/signal.h
 create mode 100644 tests/tcg/multiarch/gdbstub/late-attach.py
 create mode 100644 tests/tcg/multiarch/late-attach.c

-- 
2.47.0

[PATCH 1/8] gdbstub: Allow the %d placeholder in the socket path

[Ilya Leoshkevich]

Just like for QEMU_LOG_FILENAME, replace %d with PID in the GDB socket
path. This allows running multi-process applications with, e.g.,
export QEMU_GDB=/tmp/qemu-%d.sock. Currently this is not possible,
since the first process will cause the subsequent ones to fail due to
not being able to bind() the GDB socket.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/user.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdbstub/user.c b/gdbstub/user.c
index 0b4bfa9c488..cdf5affae15 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -316,9 +316,19 @@ static bool gdb_accept_socket(int gdb_fd)
 
 static int gdbserver_open_socket(const char *path)
 {
+    g_autoptr(GString) buf = g_string_new("");
     struct sockaddr_un sockaddr = {};
+    char *pid_placeholder;
     int fd, ret;
 
+    pid_placeholder = strstr(path, "%d");
+    if (pid_placeholder != NULL) {
+        g_string_append_len(buf, path, pid_placeholder - path);
+        g_string_append_printf(buf, "%d", getpid());
+        g_string_append(buf, pid_placeholder + 2);
+        path = buf->str;
+    }
+
     fd = socket(AF_UNIX, SOCK_STREAM, 0);
     if (fd < 0) {
         perror("create socket");
-- 
2.47.0

Re: [PATCH 1/8] gdbstub: Allow the %d placeholder in the socket path

[Richard Henderson]

On 10/24/24 20:59, Ilya Leoshkevich wrote:
> Just like for QEMU_LOG_FILENAME, replace %d with PID in the GDB socket
> path. This allows running multi-process applications with, e.g.,
> export QEMU_GDB=/tmp/qemu-%d.sock. Currently this is not possible,
> since the first process will cause the subsequent ones to fail due to
> not being able to bind() the GDB socket.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   gdbstub/user.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index 0b4bfa9c488..cdf5affae15 100644
> --- a/gdbstub/user.c
> +++ b/gdbstub/user.c
> @@ -316,9 +316,19 @@ static bool gdb_accept_socket(int gdb_fd)
>   
>   static int gdbserver_open_socket(const char *path)
>   {
> +    g_autoptr(GString) buf = g_string_new("");
>       struct sockaddr_un sockaddr = {};
> +    char *pid_placeholder;
>       int fd, ret;
>   
> +    pid_placeholder = strstr(path, "%d");
> +    if (pid_placeholder != NULL) {
> +        g_string_append_len(buf, path, pid_placeholder - path);
> +        g_string_append_printf(buf, "%d", getpid());

qemu_get_thread_id().

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/8] gdbstub: Allow the %d placeholder in the socket path

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Tue, Nov 5, 2024 at 7:41 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 10/24/24 20:59, Ilya Leoshkevich wrote:
> > Just like for QEMU_LOG_FILENAME, replace %d with PID in the GDB socket
> > path. This allows running multi-process applications with, e.g.,
> > export QEMU_GDB=/tmp/qemu-%d.sock. Currently this is not possible,
> > since the first process will cause the subsequent ones to fail due to
> > not being able to bind() the GDB socket.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   gdbstub/user.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> >
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 0b4bfa9c488..cdf5affae15 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -316,9 +316,19 @@ static bool gdb_accept_socket(int gdb_fd)
> >
> >   static int gdbserver_open_socket(const char *path)
> >   {
> > +    g_autoptr(GString) buf = g_string_new("");
> >       struct sockaddr_un sockaddr = {};
> > +    char *pid_placeholder;
> >       int fd, ret;
> >
> > +    pid_placeholder = strstr(path, "%d");
> > +    if (pid_placeholder != NULL) {
> > +        g_string_append_len(buf, path, pid_placeholder - path);
> > +        g_string_append_printf(buf, "%d", getpid());
>
> qemu_get_thread_id().
>
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>

Same.

Reviewed-by: Warner Losh <imp@bsdimp.com>

[-- Attachment #2: Type: text/html, Size: 2229 bytes --]

[PATCH 2/8] gdbstub: Try unlinking the unix socket before binding

[Ilya Leoshkevich]

In case an emulated process execve()s another emulated process, bind()
will fail, because the socket already exists. So try deleting it.

Note that it is not possible to handle this in do_execv(): deleting
gdbserver_user_state.socket_path before safe_execve() is not correct,
because the latter may fail, and afterwards we may lose control.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdbstub/user.c b/gdbstub/user.c
index cdf5affae15..26b25b1b7e9 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -337,6 +337,7 @@ static int gdbserver_open_socket(const char *path)
 
     sockaddr.sun_family = AF_UNIX;
     pstrcpy(sockaddr.sun_path, sizeof(sockaddr.sun_path) - 1, path);
+    unlink(sockaddr.sun_path);
     ret = bind(fd, (struct sockaddr *)&sockaddr, sizeof(sockaddr));
     if (ret < 0) {
         perror("bind socket");
-- 
2.47.0

[PATCH 3/8] user: Introduce user/signal.h

[Ilya Leoshkevich]

gdbstub needs target_to_host_signal(), so move its declaration to a
public header.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 bsd-user/signal-common.h   |  1 -
 bsd-user/signal.c          |  1 +
 include/user/signal.h      | 23 +++++++++++++++++++++++
 linux-user/signal-common.h |  1 -
 linux-user/signal.c        |  1 +
 linux-user/syscall.c       |  1 +
 6 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 include/user/signal.h

diff --git a/bsd-user/signal-common.h b/bsd-user/signal-common.h
index 77d7c7a78b7..4e634e04a30 100644
--- a/bsd-user/signal-common.h
+++ b/bsd-user/signal-common.h
@@ -42,7 +42,6 @@ void process_pending_signals(CPUArchState *env);
 void queue_signal(CPUArchState *env, int sig, int si_type,
                   target_siginfo_t *info);
 void signal_init(void);
-int target_to_host_signal(int sig);
 void target_to_host_sigset(sigset_t *d, const target_sigset_t *s);
 
 /*
diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index da49b9bffc1..a2b11a97131 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -22,6 +22,7 @@
 #include "qemu/log.h"
 #include "qemu.h"
 #include "exec/page-protection.h"
+#include "user/signal.h"
 #include "user/tswap-target.h"
 #include "gdbstub/user.h"
 #include "signal-common.h"
diff --git a/include/user/signal.h b/include/user/signal.h
new file mode 100644
index 00000000000..19b6b9e5ddc
--- /dev/null
+++ b/include/user/signal.h
@@ -0,0 +1,23 @@
+/*
+ * Signal-related declarations.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef USER_SIGNAL_H
+#define USER_SIGNAL_H
+
+#ifndef CONFIG_USER_ONLY
+#error Cannot include this header from system emulation
+#endif
+
+/**
+ * target_to_host_signal:
+ * @sig: target signal.
+ *
+ * On success, return the host signal between 0 (inclusive) and NSIG
+ * (exclusive) corresponding to the target signal @sig. Return any other value
+ * on failure.
+ */
+int target_to_host_signal(int sig);
+
+#endif
diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
index f4cbe6185e1..f82185ec0f5 100644
--- a/linux-user/signal-common.h
+++ b/linux-user/signal-common.h
@@ -61,7 +61,6 @@ void queue_signal(CPUArchState *env, int sig, int si_type,
                   target_siginfo_t *info);
 void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info);
 void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t *tinfo);
-int target_to_host_signal(int sig);
 int host_to_target_signal(int sig);
 long do_sigreturn(CPUArchState *env);
 long do_rt_sigreturn(CPUArchState *env);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 63ac2df53b7..84bb8a34808 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -33,6 +33,7 @@
 #include "signal-common.h"
 #include "host-signal.h"
 #include "user/safe-syscall.h"
+#include "user/signal.h"
 #include "tcg/tcg.h"
 
 /* target_siginfo_t must fit in gdbstub's siginfo save area. */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d1b0f7c5bbc..0a3c4d5a946 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -136,6 +136,7 @@
 #include "loader.h"
 #include "user-mmap.h"
 #include "user/safe-syscall.h"
+#include "user/signal.h"
 #include "qemu/guest-random.h"
 #include "qemu/selfmap.h"
 #include "user/syscall-trace.h"
-- 
2.47.0

Re: [PATCH 3/8] user: Introduce user/signal.h

[Richard Henderson]

On 10/24/24 20:59, Ilya Leoshkevich wrote:
> gdbstub needs target_to_host_signal(), so move its declaration to a
> public header.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   bsd-user/signal-common.h   |  1 -
>   bsd-user/signal.c          |  1 +
>   include/user/signal.h      | 23 +++++++++++++++++++++++
>   linux-user/signal-common.h |  1 -
>   linux-user/signal.c        |  1 +
>   linux-user/syscall.c       |  1 +
>   6 files changed, 26 insertions(+), 2 deletions(-)
>   create mode 100644 include/user/signal.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/8] user: Introduce user/signal.h

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3765 bytes --]

On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> gdbstub needs target_to_host_signal(), so move its declaration to a
> public header.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  bsd-user/signal-common.h   |  1 -
>  bsd-user/signal.c          |  1 +
>  include/user/signal.h      | 23 +++++++++++++++++++++++
>  linux-user/signal-common.h |  1 -
>  linux-user/signal.c        |  1 +
>  linux-user/syscall.c       |  1 +
>  6 files changed, 26 insertions(+), 2 deletions(-)
>  create mode 100644 include/user/signal.h
>

Reviewed-by: Warner Losh <imp@bsdimp.com>


> diff --git a/bsd-user/signal-common.h b/bsd-user/signal-common.h
> index 77d7c7a78b7..4e634e04a30 100644
> --- a/bsd-user/signal-common.h
> +++ b/bsd-user/signal-common.h
> @@ -42,7 +42,6 @@ void process_pending_signals(CPUArchState *env);
>  void queue_signal(CPUArchState *env, int sig, int si_type,
>                    target_siginfo_t *info);
>  void signal_init(void);
> -int target_to_host_signal(int sig);
>  void target_to_host_sigset(sigset_t *d, const target_sigset_t *s);
>
>  /*
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index da49b9bffc1..a2b11a97131 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -22,6 +22,7 @@
>  #include "qemu/log.h"
>  #include "qemu.h"
>  #include "exec/page-protection.h"
> +#include "user/signal.h"
>  #include "user/tswap-target.h"
>  #include "gdbstub/user.h"
>  #include "signal-common.h"
> diff --git a/include/user/signal.h b/include/user/signal.h
> new file mode 100644
> index 00000000000..19b6b9e5ddc
> --- /dev/null
> +++ b/include/user/signal.h
> @@ -0,0 +1,23 @@
> +/*
> + * Signal-related declarations.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef USER_SIGNAL_H
> +#define USER_SIGNAL_H
> +
> +#ifndef CONFIG_USER_ONLY
> +#error Cannot include this header from system emulation
> +#endif
> +
> +/**
> + * target_to_host_signal:
> + * @sig: target signal.
> + *
> + * On success, return the host signal between 0 (inclusive) and NSIG
> + * (exclusive) corresponding to the target signal @sig. Return any other
> value
> + * on failure.
> + */
> +int target_to_host_signal(int sig);
> +
> +#endif
> diff --git a/linux-user/signal-common.h b/linux-user/signal-common.h
> index f4cbe6185e1..f82185ec0f5 100644
> --- a/linux-user/signal-common.h
> +++ b/linux-user/signal-common.h
> @@ -61,7 +61,6 @@ void queue_signal(CPUArchState *env, int sig, int
> si_type,
>                    target_siginfo_t *info);
>  void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t
> *info);
>  void target_to_host_siginfo(siginfo_t *info, const target_siginfo_t
> *tinfo);
> -int target_to_host_signal(int sig);
>  int host_to_target_signal(int sig);
>  long do_sigreturn(CPUArchState *env);
>  long do_rt_sigreturn(CPUArchState *env);
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 63ac2df53b7..84bb8a34808 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -33,6 +33,7 @@
>  #include "signal-common.h"
>  #include "host-signal.h"
>  #include "user/safe-syscall.h"
> +#include "user/signal.h"
>  #include "tcg/tcg.h"
>
>  /* target_siginfo_t must fit in gdbstub's siginfo save area. */
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d1b0f7c5bbc..0a3c4d5a946 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -136,6 +136,7 @@
>  #include "loader.h"
>  #include "user-mmap.h"
>  #include "user/safe-syscall.h"
> +#include "user/signal.h"
>  #include "qemu/guest-random.h"
>  #include "qemu/selfmap.h"
>  #include "user/syscall-trace.h"
> --
> 2.47.0
>
>

[-- Attachment #2: Type: text/html, Size: 4803 bytes --]

[PATCH 4/8] user: Introduce host_interrupt_signal

[Ilya Leoshkevich]

Attaching to the gdbstub of a running process requires stopping its
threads. For threads that run on a CPU, cpu_exit() is enough, but the
only way to grab attention of a thread that is stuck in a long-running
syscall is to interrupt it with a signal.

Reserve a host realtime signal for this, just like it's already done
for TARGET_SIGABRT on Linux. This may reduce the number of available
guest realtime signals by one, but this is acceptable, since there are
quite a lot of them, and it's unlikely that there are apps that need
them all.

Set signal_pending for the safe_sycall machinery to prevent invoking
the syscall. This is a lie, since we don't queue a guest signal, but
process_pending_signals() can handle the absence of pending signals.
The syscall returns with QEMU_ERESTARTSYS errno, which arranges for
the automatic restart. This is important, because it helps avoiding
disturbing poorly written guests.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 bsd-user/signal.c     | 12 ++++++++++++
 include/user/signal.h |  2 ++
 linux-user/signal.c   | 11 +++++++++++
 3 files changed, 25 insertions(+)

diff --git a/bsd-user/signal.c b/bsd-user/signal.c
index a2b11a97131..992736df5c5 100644
--- a/bsd-user/signal.c
+++ b/bsd-user/signal.c
@@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts, unsigned long sp)
         on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
 }
 
+int host_interrupt_signal = SIGRTMAX;
+
 /*
  * The BSD ABIs use the same signal numbers across all the CPU architectures, so
  * (unlike Linux) these functions are just the identity mapping. This might not
@@ -489,6 +491,12 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
     uintptr_t pc = 0;
     bool sync_sig = false;
 
+    if (host_sig == host_interrupt_signal) {
+        ts->signal_pending = 1;
+        cpu_exit(thread_cpu);
+        return;
+    }
+
     /*
      * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
      * handling wrt signal blocking and unwinding.
@@ -852,6 +860,9 @@ void signal_init(void)
 
     for (i = 1; i <= TARGET_NSIG; i++) {
         host_sig = target_to_host_signal(i);
+        if (host_sig == host_interrupt_signal) {
+            continue;
+        }
         sigaction(host_sig, NULL, &oact);
         if (oact.sa_sigaction == (void *)SIG_IGN) {
             sigact_table[i - 1]._sa_handler = TARGET_SIG_IGN;
@@ -870,6 +881,7 @@ void signal_init(void)
             sigaction(host_sig, &act, NULL);
         }
     }
+    sigaction(host_interrupt_signal, &act, NULL);
 }
 
 static void handle_pending_signal(CPUArchState *env, int sig,
diff --git a/include/user/signal.h b/include/user/signal.h
index 19b6b9e5ddc..7fa33b05d91 100644
--- a/include/user/signal.h
+++ b/include/user/signal.h
@@ -20,4 +20,6 @@
  */
 int target_to_host_signal(int sig);
 
+extern int host_interrupt_signal;
+
 #endif
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 84bb8a34808..f0bcbd367d5 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -514,6 +514,8 @@ static int core_dump_signal(int sig)
     }
 }
 
+int host_interrupt_signal;
+
 static void signal_table_init(void)
 {
     int hsig, tsig, count;
@@ -540,6 +542,7 @@ static void signal_table_init(void)
     hsig = SIGRTMIN;
     host_to_target_signal_table[SIGABRT] = 0;
     host_to_target_signal_table[hsig++] = TARGET_SIGABRT;
+    host_interrupt_signal = hsig++;
 
     for (tsig = TARGET_SIGRTMIN;
          hsig <= SIGRTMAX && tsig <= TARGET_NSIG;
@@ -619,6 +622,8 @@ void signal_init(void)
         }
         sigact_table[tsig - 1]._sa_handler = thand;
     }
+
+    sigaction(host_interrupt_signal, &act, NULL);
 }
 
 /* Force a synchronously taken signal. The kernel force_sig() function
@@ -966,6 +971,12 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
     bool sync_sig = false;
     void *sigmask;
 
+    if (host_sig == host_interrupt_signal) {
+        ts->signal_pending = 1;
+        cpu_exit(thread_cpu);
+        return;
+    }
+
     /*
      * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
      * handling wrt signal blocking and unwinding.  Non-spoofed SIGILL,
-- 
2.47.0

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Richard Henderson]

On 10/24/24 20:59, Ilya Leoshkevich wrote:
> Attaching to the gdbstub of a running process requires stopping its
> threads. For threads that run on a CPU, cpu_exit() is enough, but the
> only way to grab attention of a thread that is stuck in a long-running
> syscall is to interrupt it with a signal.
> 
> Reserve a host realtime signal for this, just like it's already done
> for TARGET_SIGABRT on Linux. This may reduce the number of available
> guest realtime signals by one, but this is acceptable, since there are
> quite a lot of them, and it's unlikely that there are apps that need
> them all.
> 
> Set signal_pending for the safe_sycall machinery to prevent invoking
> the syscall. This is a lie, since we don't queue a guest signal, but
> process_pending_signals() can handle the absence of pending signals.
> The syscall returns with QEMU_ERESTARTSYS errno, which arranges for
> the automatic restart. This is important, because it helps avoiding
> disturbing poorly written guests.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   bsd-user/signal.c     | 12 ++++++++++++
>   include/user/signal.h |  2 ++
>   linux-user/signal.c   | 11 +++++++++++
>   3 files changed, 25 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 5235 bytes --]

On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> Attaching to the gdbstub of a running process requires stopping its
> threads. For threads that run on a CPU, cpu_exit() is enough, but the
> only way to grab attention of a thread that is stuck in a long-running
> syscall is to interrupt it with a signal.
>
> Reserve a host realtime signal for this, just like it's already done
> for TARGET_SIGABRT on Linux. This may reduce the number of available
> guest realtime signals by one, but this is acceptable, since there are
> quite a lot of them, and it's unlikely that there are apps that need
> them all.
>
> Set signal_pending for the safe_sycall machinery to prevent invoking
> the syscall. This is a lie, since we don't queue a guest signal, but
> process_pending_signals() can handle the absence of pending signals.
> The syscall returns with QEMU_ERESTARTSYS errno, which arranges for
> the automatic restart. This is important, because it helps avoiding
> disturbing poorly written guests.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  bsd-user/signal.c     | 12 ++++++++++++
>  include/user/signal.h |  2 ++
>  linux-user/signal.c   | 11 +++++++++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> index a2b11a97131..992736df5c5 100644
> --- a/bsd-user/signal.c
> +++ b/bsd-user/signal.c
> @@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts, unsigned
> long sp)
>          on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
>  }
>
> +int host_interrupt_signal = SIGRTMAX;
> +
>

I'd be tempted to use SIGRTMAX + 1 or even TARGET_NSIG. 127 or 128 would
work and not overflow any arrays (or hit any bounds tests) I'd likely use
SIGRTMAX + 1,
though, since it avoids any edge-cases from sig == NSIG that might be in
the code
unnoticed.

Now, having said that, I don't think that there's too many (any?) programs
we need
to run as bsd-user that have real-time signals, much less one that uses
SIGRTMAX,
but stranger things have happened. But it is a little wiggle room just in
case.

Other than that:

Reviewed-by: Warner Losh <imp@bsdimp.com>



>  /*
>   * The BSD ABIs use the same signal numbers across all the CPU
> architectures, so
>   * (unlike Linux) these functions are just the identity mapping. This
> might not
> @@ -489,6 +491,12 @@ static void host_signal_handler(int host_sig,
> siginfo_t *info, void *puc)
>      uintptr_t pc = 0;
>      bool sync_sig = false;
>
> +    if (host_sig == host_interrupt_signal) {
> +        ts->signal_pending = 1;
> +        cpu_exit(thread_cpu);
> +        return;
> +    }
> +
>      /*
>       * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
>       * handling wrt signal blocking and unwinding.
> @@ -852,6 +860,9 @@ void signal_init(void)
>
>      for (i = 1; i <= TARGET_NSIG; i++) {
>          host_sig = target_to_host_signal(i);
> +        if (host_sig == host_interrupt_signal) {
> +            continue;
> +        }
>          sigaction(host_sig, NULL, &oact);
>          if (oact.sa_sigaction == (void *)SIG_IGN) {
>              sigact_table[i - 1]._sa_handler = TARGET_SIG_IGN;
> @@ -870,6 +881,7 @@ void signal_init(void)
>              sigaction(host_sig, &act, NULL);
>          }
>      }
> +    sigaction(host_interrupt_signal, &act, NULL);
>  }
>
>  static void handle_pending_signal(CPUArchState *env, int sig,
> diff --git a/include/user/signal.h b/include/user/signal.h
> index 19b6b9e5ddc..7fa33b05d91 100644
> --- a/include/user/signal.h
> +++ b/include/user/signal.h
> @@ -20,4 +20,6 @@
>   */
>  int target_to_host_signal(int sig);
>
> +extern int host_interrupt_signal;
> +
>  #endif
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 84bb8a34808..f0bcbd367d5 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -514,6 +514,8 @@ static int core_dump_signal(int sig)
>      }
>  }
>
> +int host_interrupt_signal;
> +
>  static void signal_table_init(void)
>  {
>      int hsig, tsig, count;
> @@ -540,6 +542,7 @@ static void signal_table_init(void)
>      hsig = SIGRTMIN;
>      host_to_target_signal_table[SIGABRT] = 0;
>      host_to_target_signal_table[hsig++] = TARGET_SIGABRT;
> +    host_interrupt_signal = hsig++;
>
>      for (tsig = TARGET_SIGRTMIN;
>           hsig <= SIGRTMAX && tsig <= TARGET_NSIG;
> @@ -619,6 +622,8 @@ void signal_init(void)
>          }
>          sigact_table[tsig - 1]._sa_handler = thand;
>      }
> +
> +    sigaction(host_interrupt_signal, &act, NULL);
>  }
>
>  /* Force a synchronously taken signal. The kernel force_sig() function
> @@ -966,6 +971,12 @@ static void host_signal_handler(int host_sig,
> siginfo_t *info, void *puc)
>      bool sync_sig = false;
>      void *sigmask;
>
> +    if (host_sig == host_interrupt_signal) {
> +        ts->signal_pending = 1;
> +        cpu_exit(thread_cpu);
> +        return;
> +    }
> +
>      /*
>       * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
>       * handling wrt signal blocking and unwinding.  Non-spoofed SIGILL,
> --
> 2.47.0
>
>

[-- Attachment #2: Type: text/html, Size: 6427 bytes --]

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Ilya Leoshkevich]

On Tue, 2024-11-05 at 08:39 -0700, Warner Losh wrote:
> On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > Attaching to the gdbstub of a running process requires stopping its
> > threads. For threads that run on a CPU, cpu_exit() is enough, but
> > the
> > only way to grab attention of a thread that is stuck in a long-
> > running
> > syscall is to interrupt it with a signal.
> > 
> > Reserve a host realtime signal for this, just like it's already
> > done
> > for TARGET_SIGABRT on Linux. This may reduce the number of
> > available
> > guest realtime signals by one, but this is acceptable, since there
> > are
> > quite a lot of them, and it's unlikely that there are apps that
> > need
> > them all.
> > 
> > Set signal_pending for the safe_sycall machinery to prevent
> > invoking
> > the syscall. This is a lie, since we don't queue a guest signal,
> > but
> > process_pending_signals() can handle the absence of pending
> > signals.
> > The syscall returns with QEMU_ERESTARTSYS errno, which arranges for
> > the automatic restart. This is important, because it helps avoiding
> > disturbing poorly written guests.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  bsd-user/signal.c     | 12 ++++++++++++
> >  include/user/signal.h |  2 ++
> >  linux-user/signal.c   | 11 +++++++++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > index a2b11a97131..992736df5c5 100644
> > --- a/bsd-user/signal.c
> > +++ b/bsd-user/signal.c
> > @@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts,
> > unsigned long sp)
> >          on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
> >  }
> > 
> > +int host_interrupt_signal = SIGRTMAX;
> > +
> > 
> 
> 
> I'd be tempted to use SIGRTMAX + 1 or even TARGET_NSIG. 127 or 128
> would
> work and not overflow any arrays (or hit any bounds tests) I'd likely
> use SIGRTMAX + 1,
> though, since it avoids any edge-cases from sig == NSIG that might be
> in the code
> unnoticed.
> 
> Now, having said that, I don't think that there's too many (any?)
> programs we need
> to run as bsd-user that have real-time signals, much less one that
> uses SIGRTMAX,
> but stranger things have happened. But it is a little wiggle room
> just in case.
> 
> Other than that:
> 
> Reviewed-by: Warner Losh <imp@bsdimp.com>

Thanks for the suggestion, I'll use SIGRTMAX + 1 in v2.

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Richard Henderson]

On 11/5/24 15:50, Ilya Leoshkevich wrote:
> On Tue, 2024-11-05 at 08:39 -0700, Warner Losh wrote:
>> On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich <iii@linux.ibm.com>
>> wrote:
>>> Attaching to the gdbstub of a running process requires stopping its
>>> threads. For threads that run on a CPU, cpu_exit() is enough, but
>>> the
>>> only way to grab attention of a thread that is stuck in a long-
>>> running
>>> syscall is to interrupt it with a signal.
>>>
>>> Reserve a host realtime signal for this, just like it's already
>>> done
>>> for TARGET_SIGABRT on Linux. This may reduce the number of
>>> available
>>> guest realtime signals by one, but this is acceptable, since there
>>> are
>>> quite a lot of them, and it's unlikely that there are apps that
>>> need
>>> them all.
>>>
>>> Set signal_pending for the safe_sycall machinery to prevent
>>> invoking
>>> the syscall. This is a lie, since we don't queue a guest signal,
>>> but
>>> process_pending_signals() can handle the absence of pending
>>> signals.
>>> The syscall returns with QEMU_ERESTARTSYS errno, which arranges for
>>> the automatic restart. This is important, because it helps avoiding
>>> disturbing poorly written guests.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>   bsd-user/signal.c     | 12 ++++++++++++
>>>   include/user/signal.h |  2 ++
>>>   linux-user/signal.c   | 11 +++++++++++
>>>   3 files changed, 25 insertions(+)
>>>
>>> diff --git a/bsd-user/signal.c b/bsd-user/signal.c
>>> index a2b11a97131..992736df5c5 100644
>>> --- a/bsd-user/signal.c
>>> +++ b/bsd-user/signal.c
>>> @@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts,
>>> unsigned long sp)
>>>           on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
>>>   }
>>>
>>> +int host_interrupt_signal = SIGRTMAX;
>>> +
>>>
>>
>>
>> I'd be tempted to use SIGRTMAX + 1 or even TARGET_NSIG. 127 or 128
>> would
>> work and not overflow any arrays (or hit any bounds tests) I'd likely
>> use SIGRTMAX + 1,
>> though, since it avoids any edge-cases from sig == NSIG that might be
>> in the code
>> unnoticed.
>>
>> Now, having said that, I don't think that there's too many (any?)
>> programs we need
>> to run as bsd-user that have real-time signals, much less one that
>> uses SIGRTMAX,
>> but stranger things have happened. But it is a little wiggle room
>> just in case.
>>
>> Other than that:
>>
>> Reviewed-by: Warner Losh <imp@bsdimp.com>
> 
> Thanks for the suggestion, I'll use SIGRTMAX + 1 in v2.


That can't be right -- SIGRTMAX+1 is not a valid signal.


r~

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Ilya Leoshkevich]

On Tue, 2024-11-05 at 22:30 +0000, Richard Henderson wrote:
> On 11/5/24 15:50, Ilya Leoshkevich wrote:
> > On Tue, 2024-11-05 at 08:39 -0700, Warner Losh wrote:
> > > On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich
> > > <iii@linux.ibm.com>
> > > wrote:
> > > > Attaching to the gdbstub of a running process requires stopping
> > > > its
> > > > threads. For threads that run on a CPU, cpu_exit() is enough,
> > > > but
> > > > the
> > > > only way to grab attention of a thread that is stuck in a long-
> > > > running
> > > > syscall is to interrupt it with a signal.
> > > > 
> > > > Reserve a host realtime signal for this, just like it's already
> > > > done
> > > > for TARGET_SIGABRT on Linux. This may reduce the number of
> > > > available
> > > > guest realtime signals by one, but this is acceptable, since
> > > > there
> > > > are
> > > > quite a lot of them, and it's unlikely that there are apps that
> > > > need
> > > > them all.
> > > > 
> > > > Set signal_pending for the safe_sycall machinery to prevent
> > > > invoking
> > > > the syscall. This is a lie, since we don't queue a guest
> > > > signal,
> > > > but
> > > > process_pending_signals() can handle the absence of pending
> > > > signals.
> > > > The syscall returns with QEMU_ERESTARTSYS errno, which arranges
> > > > for
> > > > the automatic restart. This is important, because it helps
> > > > avoiding
> > > > disturbing poorly written guests.
> > > > 
> > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > ---
> > > >   bsd-user/signal.c     | 12 ++++++++++++
> > > >   include/user/signal.h |  2 ++
> > > >   linux-user/signal.c   | 11 +++++++++++
> > > >   3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > > > index a2b11a97131..992736df5c5 100644
> > > > --- a/bsd-user/signal.c
> > > > +++ b/bsd-user/signal.c
> > > > @@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts,
> > > > unsigned long sp)
> > > >           on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
> > > >   }
> > > > 
> > > > +int host_interrupt_signal = SIGRTMAX;
> > > > +
> > > > 
> > > 
> > > 
> > > I'd be tempted to use SIGRTMAX + 1 or even TARGET_NSIG. 127 or
> > > 128
> > > would
> > > work and not overflow any arrays (or hit any bounds tests) I'd
> > > likely
> > > use SIGRTMAX + 1,
> > > though, since it avoids any edge-cases from sig == NSIG that
> > > might be
> > > in the code
> > > unnoticed.
> > > 
> > > Now, having said that, I don't think that there's too many (any?)
> > > programs we need
> > > to run as bsd-user that have real-time signals, much less one
> > > that
> > > uses SIGRTMAX,
> > > but stranger things have happened. But it is a little wiggle room
> > > just in case.
> > > 
> > > Other than that:
> > > 
> > > Reviewed-by: Warner Losh <imp@bsdimp.com>
> > 
> > Thanks for the suggestion, I'll use SIGRTMAX + 1 in v2.
> 
> 
> That can't be right -- SIGRTMAX+1 is not a valid signal.
> 
> 
> r~

I have to admit I didn't look into this too deeply, but I ran the
following experiment on a FreeBSD 14.1 box:

    /usr/include $ grep -R SIGRTMAX .
    ./sys/signal.h:#define  SIGRTMAX        126

    $ sleep 100 &
    $ kill -126 %1
    [1]   Unknown signal: 126     sleep 100

    $ sleep 100 &
    $ kill -127 %1
    [1] + Unknown signal: 0       sleep 100

Clearly, something is wrong - at least with the shell - but at the
same time the signal delivery seems to have occurred.

Warner, does the above look normal to you?

Re: [PATCH 4/8] user: Introduce host_interrupt_signal

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]

On Tue, Nov 5, 2024 at 3:49 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> On Tue, 2024-11-05 at 22:30 +0000, Richard Henderson wrote:
> > On 11/5/24 15:50, Ilya Leoshkevich wrote:
> > > On Tue, 2024-11-05 at 08:39 -0700, Warner Losh wrote:
> > > > On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich
> > > > <iii@linux.ibm.com>
> > > > wrote:
> > > > > Attaching to the gdbstub of a running process requires stopping
> > > > > its
> > > > > threads. For threads that run on a CPU, cpu_exit() is enough,
> > > > > but
> > > > > the
> > > > > only way to grab attention of a thread that is stuck in a long-
> > > > > running
> > > > > syscall is to interrupt it with a signal.
> > > > >
> > > > > Reserve a host realtime signal for this, just like it's already
> > > > > done
> > > > > for TARGET_SIGABRT on Linux. This may reduce the number of
> > > > > available
> > > > > guest realtime signals by one, but this is acceptable, since
> > > > > there
> > > > > are
> > > > > quite a lot of them, and it's unlikely that there are apps that
> > > > > need
> > > > > them all.
> > > > >
> > > > > Set signal_pending for the safe_sycall machinery to prevent
> > > > > invoking
> > > > > the syscall. This is a lie, since we don't queue a guest
> > > > > signal,
> > > > > but
> > > > > process_pending_signals() can handle the absence of pending
> > > > > signals.
> > > > > The syscall returns with QEMU_ERESTARTSYS errno, which arranges
> > > > > for
> > > > > the automatic restart. This is important, because it helps
> > > > > avoiding
> > > > > disturbing poorly written guests.
> > > > >
> > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > > ---
> > > > >   bsd-user/signal.c     | 12 ++++++++++++
> > > > >   include/user/signal.h |  2 ++
> > > > >   linux-user/signal.c   | 11 +++++++++++
> > > > >   3 files changed, 25 insertions(+)
> > > > >
> > > > > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > > > > index a2b11a97131..992736df5c5 100644
> > > > > --- a/bsd-user/signal.c
> > > > > +++ b/bsd-user/signal.c
> > > > > @@ -49,6 +49,8 @@ static inline int sas_ss_flags(TaskState *ts,
> > > > > unsigned long sp)
> > > > >           on_sig_stack(ts, sp) ? SS_ONSTACK : 0;
> > > > >   }
> > > > >
> > > > > +int host_interrupt_signal = SIGRTMAX;
> > > > > +
> > > > >
> > > >
> > > >
> > > > I'd be tempted to use SIGRTMAX + 1 or even TARGET_NSIG. 127 or
> > > > 128
> > > > would
> > > > work and not overflow any arrays (or hit any bounds tests) I'd
> > > > likely
> > > > use SIGRTMAX + 1,
> > > > though, since it avoids any edge-cases from sig == NSIG that
> > > > might be
> > > > in the code
> > > > unnoticed.
> > > >
> > > > Now, having said that, I don't think that there's too many (any?)
> > > > programs we need
> > > > to run as bsd-user that have real-time signals, much less one
> > > > that
> > > > uses SIGRTMAX,
> > > > but stranger things have happened. But it is a little wiggle room
> > > > just in case.
> > > >
> > > > Other than that:
> > > >
> > > > Reviewed-by: Warner Losh <imp@bsdimp.com>
> > >
> > > Thanks for the suggestion, I'll use SIGRTMAX + 1 in v2.
> >
> >
> > That can't be right -- SIGRTMAX+1 is not a valid signal.
> >
> >
> > r~
>
> I have to admit I didn't look into this too deeply, but I ran the
> following experiment on a FreeBSD 14.1 box:
>
>     /usr/include $ grep -R SIGRTMAX .
>     ./sys/signal.h:#define  SIGRTMAX        126
>
>     $ sleep 100 &
>     $ kill -126 %1
>     [1]   Unknown signal: 126     sleep 100
>
>     $ sleep 100 &
>     $ kill -127 %1
>     [1] + Unknown signal: 0       sleep 100
>
> Clearly, something is wrong - at least with the shell - but at the
> same time the signal delivery seems to have occurred.
>
> Warner, does the above look normal to you?
>

Oh! I understand.... I thought there was a gap above SIGRTMAX. It
sure looks like there is. However, 0177 (127) is used to signal that
the process is STOPPED, so can't be used. This is why SIGRTMAX
is 126 and not 127. There's room in sigset_t, but that's not sufficient.
And it has to be an actual signal we send, not just a flag.

So forget I said anything. This was a silly idea. If we find any real thing
that's using SIGRTMAX, we'll cope.

Warner

[-- Attachment #2: Type: text/html, Size: 6204 bytes --]

[PATCH 5/8] osdep: Introduce qemu_kill_thread()

[Ilya Leoshkevich]

Add a function for sending signals to individual threads. It does not make
sense on Windows, so do not provide an implementation, so that if someone
uses it by accident, they will get a linker error.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 include/qemu/osdep.h |  9 +++++++++
 util/oslib-posix.c   | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index fe7c3c5f673..2350477787a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -626,6 +626,15 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp);
 
 int qemu_get_thread_id(void);
 
+/**
+ * qemu_kill_thread:
+ * @tid: thread id.
+ * @sig: host signal.
+ *
+ * Send @sig to one of QEMU's own threads with identifier @tid.
+ */
+int qemu_kill_thread(int tid, int sig);
+
 #ifndef CONFIG_IOVEC
 struct iovec {
     void *iov_base;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 11b35e48fb8..32a41fa8640 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -111,6 +111,21 @@ int qemu_get_thread_id(void)
 #endif
 }
 
+int qemu_kill_thread(int tid, int sig)
+{
+#if defined(__linux__)
+    return syscall(__NR_tgkill, getpid(), tid, sig);
+#elif defined(__FreeBSD__)
+    return thr_kill2(getpid(), tid, sig);
+#elif defined(__NetBSD__)
+    return _lwp_kill(tid, sig);
+#elif defined(__OpenBSD__)
+    return thrkill(tid, sig, NULL);
+#else
+    return kill(tid, sig);
+#endif
+}
+
 int qemu_daemon(int nochdir, int noclose)
 {
     return daemon(nochdir, noclose);
-- 
2.47.0

Re: [PATCH 5/8] osdep: Introduce qemu_kill_thread()

[Richard Henderson]

On 10/24/24 20:59, Ilya Leoshkevich wrote:
> Add a function for sending signals to individual threads. It does not make
> sense on Windows, so do not provide an implementation, so that if someone
> uses it by accident, they will get a linker error.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   include/qemu/osdep.h |  9 +++++++++
>   util/oslib-posix.c   | 15 +++++++++++++++
>   2 files changed, 24 insertions(+)

I am annoyed that musl does not have tgkill in <signal.h>.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 5/8] osdep: Introduce qemu_kill_thread()

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]

On Thu, Oct 24, 2024 at 2:00 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:

> Add a function for sending signals to individual threads. It does not make
> sense on Windows, so do not provide an implementation, so that if someone
> uses it by accident, they will get a linker error.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  include/qemu/osdep.h |  9 +++++++++
>  util/oslib-posix.c   | 15 +++++++++++++++
>  2 files changed, 24 insertions(+)
>

Reviewed-by: Warner Losh <imp@bsidmp.com>


> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index fe7c3c5f673..2350477787a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -626,6 +626,15 @@ bool qemu_write_pidfile(const char *pidfile, Error
> **errp);
>
>  int qemu_get_thread_id(void);
>
> +/**
> + * qemu_kill_thread:
> + * @tid: thread id.
> + * @sig: host signal.
> + *
> + * Send @sig to one of QEMU's own threads with identifier @tid.
> + */
> +int qemu_kill_thread(int tid, int sig);
> +
>  #ifndef CONFIG_IOVEC
>  struct iovec {
>      void *iov_base;
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 11b35e48fb8..32a41fa8640 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -111,6 +111,21 @@ int qemu_get_thread_id(void)
>  #endif
>  }
>
> +int qemu_kill_thread(int tid, int sig)
> +{
> +#if defined(__linux__)
> +    return syscall(__NR_tgkill, getpid(), tid, sig);
> +#elif defined(__FreeBSD__)
> +    return thr_kill2(getpid(), tid, sig);
> +#elif defined(__NetBSD__)
> +    return _lwp_kill(tid, sig);
> +#elif defined(__OpenBSD__)
> +    return thrkill(tid, sig, NULL);
> +#else
> +    return kill(tid, sig);
> +#endif
> +}
> +
>  int qemu_daemon(int nochdir, int noclose)
>  {
>      return daemon(nochdir, noclose);
> --
> 2.47.0
>
>

[-- Attachment #2: Type: text/html, Size: 2569 bytes --]

[PATCH 6/8] gdbstub: Allow late attachment

[Ilya Leoshkevich]

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 cc980e6f401..7c2f8e4e96e 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -618,7 +618,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 26b25b1b7e9..455ecd6f831 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 8143a0d4b02..db54925a6c9 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1017,7 +1017,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.0

[PATCH 7/8] docs/user: Document the %d placeholder and suspend=n QEMU_GDB features

[Ilya Leoshkevich]

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 docs/user/main.rst | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/user/main.rst b/docs/user/main.rst
index 7a126ee8093..8dcb1d90a8b 100644
--- a/docs/user/main.rst
+++ b/docs/user/main.rst
@@ -50,7 +50,7 @@ Command line options
 
 ::
 
-   qemu-i386 [-h] [-d] [-L path] [-s size] [-cpu model] [-g port] [-B offset] [-R size] program [arguments...]
+   qemu-i386 [-h] [-d] [-L path] [-s size] [-cpu model] [-g endpoint] [-B offset] [-R size] program [arguments...]
 
 ``-h``
    Print the help
@@ -87,8 +87,18 @@ Debug options:
    Activate logging of the specified items (use '-d help' for a list of
    log items)
 
-``-g port``
-   Wait gdb connection to port
+``-g endpoint``
+   Wait gdb connection to a port (e.g., ``1234``) or a unix socket (e.g.,
+   ``/tmp/qemu.sock``).
+
+   If a unix socket path contains single ``%d`` placeholder (e.g.,
+   ``/tmp/qemu-%d.sock``), it is replaced by the emulator PID, which is useful
+   when passing this option via the ``QEMU_GDB`` environment variable to a
+   multi-process application.
+
+   If the endpoint address is followed by ``,suspend=n`` (e.g.,
+   ``1234,suspend=n``), then the emulated program starts without waiting for a
+   connection, which can be established at any later point in time.
 
 ``-one-insn-per-tb``
    Run the emulation with one guest instruction per translation block.
-- 
2.47.0

[PATCH 8/8] tests/tcg: Add late gdbstub attach test

[Ilya Leoshkevich]

Add a small test to prevent regressions.
Make sure that host_interrupt_signal is not visible to the guest.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/guest-debug/run-test.py              | 15 ++++++--
 tests/tcg/multiarch/Makefile.target        |  9 ++++-
 tests/tcg/multiarch/gdbstub/late-attach.py | 28 +++++++++++++++
 tests/tcg/multiarch/late-attach.c          | 41 ++++++++++++++++++++++
 4 files changed, 90 insertions(+), 3 deletions(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/late-attach.py
 create mode 100644 tests/tcg/multiarch/late-attach.c

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index 5a091db8be9..75e9c92e036 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -36,6 +36,8 @@ def get_args():
     parser.add_argument("--gdb-args", help="Additional gdb arguments")
     parser.add_argument("--output", help="A file to redirect output to")
     parser.add_argument("--stderr", help="A file to redirect stderr to")
+    parser.add_argument("--no-suspend", action="store_true",
+                        help="Ask the binary to not wait for GDB connection")
 
     return parser.parse_args()
 
@@ -73,10 +75,19 @@ def log(output, msg):
 
     # Launch QEMU with binary
     if "system" in args.qemu:
+        if args.no_suspend:
+            suspend = ''
+        else:
+            suspend = ' -S'
         cmd = f'{args.qemu} {args.qargs} {args.binary}' \
-            f' -S -gdb unix:path={socket_name},server=on'
+            f'{suspend} -gdb unix:path={socket_name},server=on'
     else:
-        cmd = f'{args.qemu} {args.qargs} -g {socket_name} {args.binary}'
+        if args.no_suspend:
+            suspend = ',suspend=n'
+        else:
+            suspend = ''
+        cmd = f'{args.qemu} {args.qargs} -g {socket_name}{suspend}' \
+            f' {args.binary}'
 
     log(output, "QEMU CMD: %s" % (cmd))
     inferior = subprocess.Popen(shlex.split(cmd))
diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 78b83d5575a..29433470fcf 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -127,6 +127,13 @@ run-gdbstub-follow-fork-mode-parent: follow-fork-mode
 		--bin $< --test $(MULTIARCH_SRC)/gdbstub/follow-fork-mode-parent.py, \
 	following parents on fork)
 
+run-gdbstub-late-attach: late-attach
+	$(call run-test, $@, env LATE_ATTACH_PY=1 $(GDB_SCRIPT) \
+		--gdb $(GDB) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" --no-suspend \
+		--bin $< --test $(MULTIARCH_SRC)/gdbstub/late-attach.py, \
+	attaching to a running process)
+
 else
 run-gdbstub-%:
 	$(call skip-test, "gdbstub test $*", "need working gdb with $(patsubst -%,,$(TARGET_NAME)) support")
@@ -136,7 +143,7 @@ EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
 	      run-gdbstub-registers run-gdbstub-prot-none \
 	      run-gdbstub-catch-syscalls run-gdbstub-follow-fork-mode-child \
 	      run-gdbstub-follow-fork-mode-parent \
-	      run-gdbstub-qxfer-siginfo-read
+	      run-gdbstub-qxfer-siginfo-read run-gdbstub-late-attach
 
 # ARM Compatible Semi Hosting Tests
 #
diff --git a/tests/tcg/multiarch/gdbstub/late-attach.py b/tests/tcg/multiarch/gdbstub/late-attach.py
new file mode 100644
index 00000000000..1d40efb5ec8
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/late-attach.py
@@ -0,0 +1,28 @@
+"""Test attaching GDB to a running process.
+
+SPDX-License-Identifier: GPL-2.0-or-later
+"""
+from test_gdbstub import main, report
+
+
+def run_test():
+    """Run through the tests one by one"""
+    try:
+        phase = gdb.parse_and_eval("phase").string()
+    except gdb.error:
+        # Assume the guest did not reach main().
+        phase = "start"
+
+    if phase == "start":
+        gdb.execute("break sigwait")
+        gdb.execute("continue")
+        phase = gdb.parse_and_eval("phase").string()
+    report(phase == "sigwait", "{} == \"sigwait\"".format(phase))
+
+    gdb.execute("signal SIGUSR1")
+
+    exitcode = int(gdb.parse_and_eval("$_exitcode"))
+    report(exitcode == 0, "{} == 0".format(exitcode))
+
+
+main(run_test)
diff --git a/tests/tcg/multiarch/late-attach.c b/tests/tcg/multiarch/late-attach.c
new file mode 100644
index 00000000000..20a364034b5
--- /dev/null
+++ b/tests/tcg/multiarch/late-attach.c
@@ -0,0 +1,41 @@
+/*
+ * Test attaching GDB to a running process.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+static const char *phase = "start";
+
+int main(void)
+{
+    sigset_t set;
+    int sig;
+
+    assert(sigfillset(&set) == 0);
+    assert(sigprocmask(SIG_BLOCK, &set, NULL) == 0);
+
+    /* Let GDB know it can send SIGUSR1. */
+    phase = "sigwait";
+    if (getenv("LATE_ATTACH_PY")) {
+        assert(sigwait(&set, &sig) == 0);
+        if (sig != SIGUSR1) {
+            fprintf(stderr, "Unexpected signal %d\n", sig);
+            return EXIT_FAILURE;
+        }
+    }
+
+    /* Check that the guest does not see host_interrupt_signal. */
+    assert(sigpending(&set) == 0);
+    for (sig = 1; sig < NSIG; sig++) {
+        if (sigismember(&set, sig)) {
+            fprintf(stderr, "Unexpected signal %d\n", sig);
+            return EXIT_FAILURE;
+        }
+    }
+
+    return EXIT_SUCCESS;
+}
-- 
2.47.0