[PATCH v3 0/5] gdbstub: Implement catching syscalls

[PATCH v3 0/5] gdbstub: Implement catching syscalls

[Ilya Leoshkevich]

v2: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02980.html
v2 -> v3: Simplify the catchpoint state by making "don't catch" a
          subset of "catch some".
          Factor out several prep patches;
          Don't use snprintf;
          Add some comments (Alex).

v1: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02911.html
v1 -> v2: Avoid touching the system gdbstub.
          Advertise QCatchSyscalls+ only on Linux.

Hi,

I noticed that GDB's "catch syscall" does not work with qemu-user.
This series adds the missing bits in [1/2] and a test in [2/2].
I'm basing this on my other series, since it contains useful gdbstub
test refactorings.

Best regards,
Ilya

Ilya Leoshkevich (5):
  gdbstub: Expose TARGET_SIGTRAP in a target-agnostic way
  gdbstub: Allow specifying a reason in stop packets
  gdbstub: Add syscall entry/return hooks
  gdbstub: Implement catching syscalls
  tests/tcg: Add the syscall catchpoint gdbstub test

 gdbstub/gdbstub.c                             |   9 ++
 gdbstub/internals.h                           |   2 +
 gdbstub/user-target.c                         |   5 +
 gdbstub/user.c                                | 108 +++++++++++++++++-
 include/gdbstub/user.h                        |  29 ++++-
 include/user/syscall-trace.h                  |   7 +-
 tests/tcg/multiarch/Makefile.target           |  10 +-
 tests/tcg/multiarch/catch-syscalls.c          |  51 +++++++++
 tests/tcg/multiarch/gdbstub/catch-syscalls.py |  53 +++++++++
 9 files changed, 268 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/multiarch/catch-syscalls.c
 create mode 100644 tests/tcg/multiarch/gdbstub/catch-syscalls.py

-- 
2.43.0

[PATCH v3 1/5] gdbstub: Expose TARGET_SIGTRAP in a target-agnostic way

[Ilya Leoshkevich]

The upcoming syscall catchpoint support needs to send SIGTRAP stop
packets to GDB. Being able to compile this support only once for all
targets is a good thing, and it requires hiding TARGET_SIGTRAP behind
a function call.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/internals.h   | 1 +
 gdbstub/user-target.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 5c0c725e54c..aeb0d9b5377 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -136,6 +136,7 @@ void gdb_append_thread_id(CPUState *cpu, GString *buf);
 int gdb_get_cpu_index(CPUState *cpu);
 unsigned int gdb_get_max_cpus(void); /* both */
 bool gdb_can_reverse(void); /* softmmu, stub for user */
+int gdb_target_sigtrap(void); /* user */
 
 void gdb_create_default_process(GDBState *s);
 
diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
index c4bba4c72c7..b7d4c37cd81 100644
--- a/gdbstub/user-target.c
+++ b/gdbstub/user-target.c
@@ -418,3 +418,8 @@ void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
                     ts->bprm->filename + offset);
     gdb_put_strbuf();
 }
+
+int gdb_target_sigtrap(void)
+{
+    return TARGET_SIGTRAP;
+}
-- 
2.43.0

[PATCH v3 2/5] gdbstub: Allow specifying a reason in stop packets

[Ilya Leoshkevich]

The upcoming syscall catchpoint support needs to send stop packets with
an associated reason to GDB. Add an extra parameter to gdb_handlesig()
for that, and rename it to gdb_handlesig_reason(). Provide a
compatibility wrapper with an old name.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/user.c         |  5 ++++-
 include/gdbstub/user.h | 16 ++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/gdbstub/user.c b/gdbstub/user.c
index dbe1d9b8875..63edca131ab 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -121,7 +121,7 @@ void gdb_qemu_exit(int code)
     exit(code);
 }
 
-int gdb_handlesig(CPUState *cpu, int sig)
+int gdb_handlesig_reason(CPUState *cpu, int sig, const char *reason)
 {
     char buf[256];
     int n;
@@ -141,6 +141,9 @@ int gdb_handlesig(CPUState *cpu, int sig)
                             "T%02xthread:", gdb_target_signal_to_gdb(sig));
             gdb_append_thread_id(cpu, gdbserver_state.str_buf);
             g_string_append_c(gdbserver_state.str_buf, ';');
+            if (reason) {
+                g_string_append(gdbserver_state.str_buf, reason);
+            }
             gdb_put_strbuf();
             gdbserver_state.allow_stop_reply = false;
         }
diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
index d392e510c59..1fc43e04af5 100644
--- a/include/gdbstub/user.h
+++ b/include/gdbstub/user.h
@@ -10,9 +10,10 @@
 #define GDBSTUB_USER_H
 
 /**
- * gdb_handlesig() - yield control to gdb
+ * gdb_handlesig_reason() - yield control to gdb
  * @cpu: CPU
  * @sig: if non-zero, the signal number which caused us to stop
+ * @reason: stop reason for stop reply packet or NULL
  *
  * This function yields control to gdb, when a user-mode-only target
  * needs to stop execution. If @sig is non-zero, then we will send a
@@ -24,7 +25,18 @@
  * or 0 if no signal should be delivered, ie the signal that caused
  * us to stop should be ignored.
  */
-int gdb_handlesig(CPUState *, int);
+int gdb_handlesig_reason(CPUState *, int, const char *);
+
+/**
+ * gdb_handlesig() - yield control to gdb
+ * @cpu CPU
+ * @sig: if non-zero, the signal number which caused us to stop
+ * @see gdb_handlesig_reason()
+ */
+static inline int gdb_handlesig(CPUState *cpu, int sig)
+{
+    return gdb_handlesig_reason(cpu, sig, NULL);
+}
 
 /**
  * gdb_signalled() - inform remote gdb of sig exit
-- 
2.43.0

[PATCH v3 3/5] gdbstub: Add syscall entry/return hooks

[Ilya Leoshkevich]

The upcoming syscall catchpoint support needs to get control on syscall
entry and return. Provide the necessary hooks for that, which are
no-ops for now.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/user.c               |  8 ++++++++
 include/gdbstub/user.h       | 13 +++++++++++++
 include/user/syscall-trace.h |  7 +++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/gdbstub/user.c b/gdbstub/user.c
index 63edca131ab..2ba01c17faf 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -502,3 +502,11 @@ void gdb_syscall_handling(const char *syscall_packet)
     gdb_put_packet(syscall_packet);
     gdb_handlesig(gdbserver_state.c_cpu, 0);
 }
+
+void gdb_syscall_entry(CPUState *cs, int num)
+{
+}
+
+void gdb_syscall_return(CPUState *cs, int num)
+{
+}
diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
index 1fc43e04af5..68b6534130c 100644
--- a/include/gdbstub/user.h
+++ b/include/gdbstub/user.h
@@ -51,5 +51,18 @@ void gdb_signalled(CPUArchState *as, int sig);
  */
 void gdbserver_fork(CPUState *cs);
 
+/**
+ * gdb_syscall_entry() - inform gdb of syscall entry and yield control to it
+ * @cs: CPU
+ * @num: syscall number
+ */
+void gdb_syscall_entry(CPUState *cs, int num);
+
+/**
+ * gdb_syscall_entry() - inform gdb of syscall return and yield control to it
+ * @cs: CPU
+ * @num: syscall number
+ */
+void gdb_syscall_return(CPUState *cs, int num);
 
 #endif /* GDBSTUB_USER_H */
diff --git a/include/user/syscall-trace.h b/include/user/syscall-trace.h
index 557f881a79b..b48b2b2d0ae 100644
--- a/include/user/syscall-trace.h
+++ b/include/user/syscall-trace.h
@@ -11,6 +11,7 @@
 #define SYSCALL_TRACE_H
 
 #include "exec/user/abitypes.h"
+#include "gdbstub/user.h"
 #include "qemu/plugin.h"
 #include "trace/trace-root.h"
 
@@ -20,7 +21,7 @@
  * could potentially unify the -strace code here as well.
  */
 
-static inline void record_syscall_start(void *cpu, int num,
+static inline void record_syscall_start(CPUState *cpu, int num,
                                         abi_long arg1, abi_long arg2,
                                         abi_long arg3, abi_long arg4,
                                         abi_long arg5, abi_long arg6,
@@ -29,11 +30,13 @@ static inline void record_syscall_start(void *cpu, int num,
     qemu_plugin_vcpu_syscall(cpu, num,
                              arg1, arg2, arg3, arg4,
                              arg5, arg6, arg7, arg8);
+    gdb_syscall_entry(cpu, num);
 }
 
-static inline void record_syscall_return(void *cpu, int num, abi_long ret)
+static inline void record_syscall_return(CPUState *cpu, int num, abi_long ret)
 {
     qemu_plugin_vcpu_syscall_ret(cpu, num, ret);
+    gdb_syscall_return(cpu, num);
 }
 
 
-- 
2.43.0

[PATCH v3 4/5] gdbstub: Implement catching syscalls

[Ilya Leoshkevich]

GDB supports stopping on syscall entry and exit using the "catch
syscall" command. It relies on 3 packets, which are currently not
supported by QEMU:

* qSupported:QCatchSyscalls+ [1]
* QCatchSyscalls: [2]
* T05syscall_entry: and T05syscall_return: [3]

Implement generation and handling of these packets.

[1] https://sourceware.org/gdb/current/onlinedocs/gdb.html/General-Query-Packets.html#qSupported
[2] https://sourceware.org/gdb/current/onlinedocs/gdb.html/General-Query-Packets.html#QCatchSyscalls
[3] https://sourceware.org/gdb/current/onlinedocs/gdb.html/Stop-Reply-Packets.html

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/gdbstub.c   |  9 +++++
 gdbstub/internals.h |  1 +
 gdbstub/user.c      | 95 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 46d752bbc2c..7e73e916bdc 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1617,6 +1617,7 @@ static void handle_query_supported(GArray *params, void *user_ctx)
     if (gdbserver_state.c_cpu->opaque) {
         g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
     }
+    g_string_append(gdbserver_state.str_buf, ";QCatchSyscalls+");
 #endif
     g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
 #endif
@@ -1810,6 +1811,14 @@ static const GdbCmdParseEntry gdb_gen_set_table[] = {
         .schema = "l0"
     },
 #endif
+#if defined(CONFIG_USER_ONLY)
+    {
+        .handler = gdb_handle_set_catch_syscalls,
+        .cmd = "CatchSyscalls:",
+        .cmd_startswith = 1,
+        .schema = "s0",
+    },
+#endif
 };
 
 static void handle_gen_query(GArray *params, void *user_ctx)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index aeb0d9b5377..56b7c13b750 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -195,6 +195,7 @@ void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
 void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
 void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
 void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* user */
+void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx); /* user */
 
 void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
 
diff --git a/gdbstub/user.c b/gdbstub/user.c
index 2ba01c17faf..8f3affbad47 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -10,6 +10,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/bitops.h"
 #include "qemu/cutils.h"
 #include "qemu/sockets.h"
 #include "exec/hwaddr.h"
@@ -21,11 +22,20 @@
 #include "trace.h"
 #include "internals.h"
 
+#define GDB_NR_SYSCALLS 1024
+typedef unsigned long GDBSyscallsMask[BITS_TO_LONGS(GDB_NR_SYSCALLS)];
+
 /* User-mode specific state */
 typedef struct {
     int fd;
     char *socket_path;
     int running_state;
+    /*
+     * Store syscalls mask without memory allocation in order to avoid
+     * implementing synchronization.
+     */
+    bool catch_all_syscalls;
+    GDBSyscallsMask catch_syscalls_mask;
 } GDBUserState;
 
 static GDBUserState gdbserver_user_state;
@@ -503,10 +513,95 @@ void gdb_syscall_handling(const char *syscall_packet)
     gdb_handlesig(gdbserver_state.c_cpu, 0);
 }
 
+static bool should_catch_syscall(int num)
+{
+    if (gdbserver_user_state.catch_all_syscalls) {
+        return true;
+    }
+    if (num < 0 || num >= GDB_NR_SYSCALLS) {
+        return false;
+    }
+    return test_bit(num, gdbserver_user_state.catch_syscalls_mask);
+}
+
 void gdb_syscall_entry(CPUState *cs, int num)
 {
+    if (should_catch_syscall(num)) {
+        g_autoptr(GString) reason = g_string_sized_new(24);
+
+        g_string_printf(reason, "syscall_entry:%x;", num);
+        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason->str);
+    }
 }
 
 void gdb_syscall_return(CPUState *cs, int num)
 {
+    if (should_catch_syscall(num)) {
+        g_autoptr(GString) reason = g_string_sized_new(24);
+
+        g_string_printf(reason, "syscall_return:%x;", num);
+        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason->str);
+    }
+}
+
+void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx)
+{
+    const char *param = get_param(params, 0)->data;
+    GDBSyscallsMask catch_syscalls_mask;
+    bool catch_all_syscalls;
+    unsigned int num;
+    const char *p;
+
+    /* "0" means not catching any syscalls. */
+    if (strcmp(param, "0") == 0) {
+        gdbserver_user_state.catch_all_syscalls = false;
+        memset(gdbserver_user_state.catch_syscalls_mask, 0,
+               sizeof(gdbserver_user_state.catch_syscalls_mask));
+        gdb_put_packet("OK");
+        return;
+    }
+
+    /* "1" means catching all syscalls. */
+    if (strcmp(param, "1") == 0) {
+        gdbserver_user_state.catch_all_syscalls = true;
+        gdb_put_packet("OK");
+        return;
+    }
+
+    /*
+     * "1;..." means catching only the specified syscalls.
+     * The syscall list must not be empty.
+     */
+    if (param[0] == '1' && param[1] == ';') {
+        catch_all_syscalls = false;
+        memset(catch_syscalls_mask, 0, sizeof(catch_syscalls_mask));
+        for (p = &param[2];; p++) {
+            if (qemu_strtoui(p, &p, 16, &num) || (*p && *p != ';')) {
+                goto err;
+            }
+            if (num >= GDB_NR_SYSCALLS) {
+                /*
+                 * Fall back to reporting all syscalls. Reporting extra
+                 * syscalls is inefficient, but the spec explicitly allows it.
+                 * Keep parsing in case there is a syntax error ahead.
+                 */
+                catch_all_syscalls = true;
+            } else {
+                set_bit(num, catch_syscalls_mask);
+            }
+            if (!*p) {
+                break;
+            }
+        }
+        gdbserver_user_state.catch_all_syscalls = catch_all_syscalls;
+        if (!catch_all_syscalls) {
+            memcpy(gdbserver_user_state.catch_syscalls_mask,
+                   catch_syscalls_mask, sizeof(catch_syscalls_mask));
+        }
+        gdb_put_packet("OK");
+        return;
+    }
+
+err:
+    gdb_put_packet("E00");
 }
-- 
2.43.0

[PATCH v3 5/5] tests/tcg: Add the syscall catchpoint gdbstub test

[Ilya Leoshkevich]

Check that adding/removing syscall catchpoints works.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/Makefile.target           | 10 +++-
 tests/tcg/multiarch/catch-syscalls.c          | 51 ++++++++++++++++++
 tests/tcg/multiarch/gdbstub/catch-syscalls.py | 53 +++++++++++++++++++
 3 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/multiarch/catch-syscalls.c
 create mode 100644 tests/tcg/multiarch/gdbstub/catch-syscalls.py

diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 315a2e13588..e10951a8016 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -108,13 +108,21 @@ run-gdbstub-prot-none: prot-none
 		--bin $< --test $(MULTIARCH_SRC)/gdbstub/prot-none.py, \
 	accessing PROT_NONE memory)
 
+run-gdbstub-catch-syscalls: catch-syscalls
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(GDB) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin $< --test $(MULTIARCH_SRC)/gdbstub/catch-syscalls.py, \
+	hitting a syscall catchpoint)
+
 else
 run-gdbstub-%:
 	$(call skip-test, "gdbstub test $*", "need working gdb with $(patsubst -%,,$(TARGET_NAME)) support")
 endif
 EXTRA_RUNS += run-gdbstub-sha1 run-gdbstub-qxfer-auxv-read \
 	      run-gdbstub-proc-mappings run-gdbstub-thread-breakpoint \
-	      run-gdbstub-registers run-gdbstub-prot-none
+	      run-gdbstub-registers run-gdbstub-prot-none \
+	      run-gdbstub-catch-syscalls
 
 # ARM Compatible Semi Hosting Tests
 #
diff --git a/tests/tcg/multiarch/catch-syscalls.c b/tests/tcg/multiarch/catch-syscalls.c
new file mode 100644
index 00000000000..d1ff1936a7a
--- /dev/null
+++ b/tests/tcg/multiarch/catch-syscalls.c
@@ -0,0 +1,51 @@
+/*
+ * Test GDB syscall catchpoints.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#define _GNU_SOURCE
+#include <stdlib.h>
+#include <unistd.h>
+
+const char *catch_syscalls_state = "start";
+
+void end_of_main(void)
+{
+}
+
+int main(void)
+{
+    int ret = EXIT_FAILURE;
+    char c0 = 'A', c1;
+    int fd[2];
+
+    catch_syscalls_state = "pipe2";
+    if (pipe2(fd, 0)) {
+        goto out;
+    }
+
+    catch_syscalls_state = "write";
+    if (write(fd[1], &c0, sizeof(c0)) != sizeof(c0)) {
+        goto out_close;
+    }
+
+    catch_syscalls_state = "read";
+    if (read(fd[0], &c1, sizeof(c1)) != sizeof(c1)) {
+        goto out_close;
+    }
+
+    catch_syscalls_state = "check";
+    if (c0 == c1) {
+        ret = EXIT_SUCCESS;
+    }
+
+out_close:
+    catch_syscalls_state = "close";
+    close(fd[0]);
+    close(fd[1]);
+
+out:
+    catch_syscalls_state = "end";
+    end_of_main();
+    return ret;
+}
diff --git a/tests/tcg/multiarch/gdbstub/catch-syscalls.py b/tests/tcg/multiarch/gdbstub/catch-syscalls.py
new file mode 100644
index 00000000000..ccce35902fb
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/catch-syscalls.py
@@ -0,0 +1,53 @@
+"""Test GDB syscall catchpoints.
+
+SPDX-License-Identifier: GPL-2.0-or-later
+"""
+from test_gdbstub import main, report
+
+
+def check_state(expected):
+    """Check the catch_syscalls_state value"""
+    actual = gdb.parse_and_eval("catch_syscalls_state").string()
+    report(actual == expected, "{} == {}".format(actual, expected))
+
+
+def run_test():
+    """Run through the tests one by one"""
+    gdb.Breakpoint("main")
+    gdb.execute("continue")
+
+    # Check that GDB stops for pipe2/read calls/returns, but not for write.
+    gdb.execute("delete")
+    try:
+        gdb.execute("catch syscall pipe2 read")
+    except gdb.error as exc:
+        exc_str = str(exc)
+        if "not supported on this architecture" in exc_str:
+            print("SKIP: {}".format(exc_str))
+            return
+        raise
+    for _ in range(2):
+        gdb.execute("continue")
+        check_state("pipe2")
+    for _ in range(2):
+        gdb.execute("continue")
+        check_state("read")
+
+    # Check that deletion works.
+    gdb.execute("delete")
+    gdb.Breakpoint("end_of_main")
+    gdb.execute("continue")
+    check_state("end")
+
+    # Check that catch-all works (libc should at least call exit).
+    gdb.execute("delete")
+    gdb.execute("catch syscall")
+    gdb.execute("continue")
+    gdb.execute("delete")
+    gdb.execute("continue")
+
+    exitcode = int(gdb.parse_and_eval("$_exitcode"))
+    report(exitcode == 0, "{} == 0".format(exitcode))
+
+
+main(run_test)
-- 
2.43.0

Re: [PATCH v3 0/5] gdbstub: Implement catching syscalls

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> v2: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02980.html
> v2 -> v3: Simplify the catchpoint state by making "don't catch" a
>           subset of "catch some".
>           Factor out several prep patches;
>           Don't use snprintf;
>           Add some comments (Alex).
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02911.html
> v1 -> v2: Avoid touching the system gdbstub.
>           Advertise QCatchSyscalls+ only on Linux.
>
> Hi,
>
> I noticed that GDB's "catch syscall" does not work with qemu-user.
> This series adds the missing bits in [1/2] and a test in [2/2].
> I'm basing this on my other series, since it contains useful gdbstub
> test refactorings.

Queued to gdbstub/next, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro