[PATCH v2 0/2] gdbstub: Implement catching syscalls

[PATCH v2 0/2] gdbstub: Implement catching syscalls

[Ilya Leoshkevich]

Based-on: <20240116003551.75168-1-iii@linux.ibm.com>
([PATCH v3 0/3] linux-user: Allow gdbstub to ignore page protection)

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 (2):
  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                                | 104 +++++++++++++++++-
 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 |  52 +++++++++
 9 files changed, 263 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 v2 1/2] 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          |   2 +
 gdbstub/user-target.c        |   5 ++
 gdbstub/user.c               | 104 ++++++++++++++++++++++++++++++++++-
 include/gdbstub/user.h       |  29 +++++++++-
 include/user/syscall-trace.h |   7 ++-
 6 files changed, 151 insertions(+), 5 deletions(-)

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 5c0c725e54c..56b7c13b750 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);
 
@@ -194,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-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;
+}
diff --git a/gdbstub/user.c b/gdbstub/user.c
index dbe1d9b8875..01dd7169258 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,25 @@
 #include "trace.h"
 #include "internals.h"
 
+enum GDBCatchSyscallsState {
+    GDB_CATCH_SYSCALLS_NONE,
+    GDB_CATCH_SYSCALLS_ALL,
+    GDB_CATCH_SYSCALLS_SELECTED,
+};
+#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.
+     */
+    enum GDBCatchSyscallsState catch_syscalls_state;
+    GDBSyscallsMask catch_syscalls_mask;
 } GDBUserState;
 
 static GDBUserState gdbserver_user_state;
@@ -121,7 +136,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 +156,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;
         }
@@ -499,3 +517,87 @@ void gdb_syscall_handling(const char *syscall_packet)
     gdb_put_packet(syscall_packet);
     gdb_handlesig(gdbserver_state.c_cpu, 0);
 }
+
+static bool should_catch_syscall(int num)
+{
+    switch (gdbserver_user_state.catch_syscalls_state) {
+    case GDB_CATCH_SYSCALLS_NONE:
+        return false;
+    case GDB_CATCH_SYSCALLS_ALL:
+        return true;
+    case GDB_CATCH_SYSCALLS_SELECTED:
+        if (num < 0 || num >= GDB_NR_SYSCALLS) {
+            return false;
+        } else {
+            return test_bit(num, gdbserver_user_state.catch_syscalls_mask);
+        }
+    default:
+        g_assert_not_reached();
+    }
+}
+
+void gdb_syscall_entry(CPUState *cs, int num)
+{
+    char reason[32];
+
+    if (should_catch_syscall(num)) {
+        snprintf(reason, sizeof(reason), "syscall_entry:%x;", num);
+        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
+    }
+}
+
+void gdb_syscall_return(CPUState *cs, int num)
+{
+    char reason[32];
+
+    if (should_catch_syscall(num)) {
+        snprintf(reason, sizeof(reason), "syscall_return:%x;", num);
+        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
+    }
+}
+
+void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx)
+{
+    enum GDBCatchSyscallsState catch_syscalls_state;
+    const char *param = get_param(params, 0)->data;
+    GDBSyscallsMask catch_syscalls_mask;
+    bool catch_syscalls_none;
+    unsigned int num;
+    const char *p;
+
+    catch_syscalls_none = strcmp(param, "0") == 0;
+    if (catch_syscalls_none || strcmp(param, "1") == 0) {
+        gdbserver_user_state.catch_syscalls_state =
+            catch_syscalls_none ? GDB_CATCH_SYSCALLS_NONE :
+                                  GDB_CATCH_SYSCALLS_ALL;
+        gdb_put_packet("OK");
+        return;
+    }
+
+    if (param[0] == '1' && param[1] == ';') {
+        catch_syscalls_state = GDB_CATCH_SYSCALLS_SELECTED;
+        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. */
+                catch_syscalls_state = GDB_CATCH_SYSCALLS_ALL;
+            } else {
+                set_bit(num, catch_syscalls_mask);
+            }
+            if (!*p) {
+                break;
+            }
+        }
+        gdbserver_user_state.catch_syscalls_state = catch_syscalls_state;
+        memcpy(gdbserver_user_state.catch_syscalls_mask, catch_syscalls_mask,
+               sizeof(catch_syscalls_mask));
+        gdb_put_packet("OK");
+        return;
+    }
+
+err:
+    gdb_put_packet("E00");
+}
diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
index d392e510c59..68b6534130c 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
@@ -39,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

Re: [PATCH v2 1/2] gdbstub: Implement catching syscalls

[Alex Bennée]

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

> 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          |   2 +
>  gdbstub/user-target.c        |   5 ++
>  gdbstub/user.c               | 104 ++++++++++++++++++++++++++++++++++-
>  include/gdbstub/user.h       |  29 +++++++++-
>  include/user/syscall-trace.h |   7 ++-
>  6 files changed, 151 insertions(+), 5 deletions(-)
>
> 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 5c0c725e54c..56b7c13b750 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);
>  
> @@ -194,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-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;
> +}
> diff --git a/gdbstub/user.c b/gdbstub/user.c
> index dbe1d9b8875..01dd7169258 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,25 @@
>  #include "trace.h"
>  #include "internals.h"
>  
> +enum GDBCatchSyscallsState {
> +    GDB_CATCH_SYSCALLS_NONE,
> +    GDB_CATCH_SYSCALLS_ALL,
> +    GDB_CATCH_SYSCALLS_SELECTED,
> +};
> +#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.
> +     */
> +    enum GDBCatchSyscallsState catch_syscalls_state;
> +    GDBSyscallsMask catch_syscalls_mask;
>  } GDBUserState;
>  
>  static GDBUserState gdbserver_user_state;
> @@ -121,7 +136,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 +156,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;
>          }
> @@ -499,3 +517,87 @@ void gdb_syscall_handling(const char *syscall_packet)
>      gdb_put_packet(syscall_packet);
>      gdb_handlesig(gdbserver_state.c_cpu, 0);
>  }
> +
> +static bool should_catch_syscall(int num)
> +{
> +    switch (gdbserver_user_state.catch_syscalls_state) {
> +    case GDB_CATCH_SYSCALLS_NONE:
> +        return false;
> +    case GDB_CATCH_SYSCALLS_ALL:
> +        return true;
> +    case GDB_CATCH_SYSCALLS_SELECTED:
> +        if (num < 0 || num >= GDB_NR_SYSCALLS) {
> +            return false;
> +        } else {
> +            return test_bit(num, gdbserver_user_state.catch_syscalls_mask);
> +        }
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
> +void gdb_syscall_entry(CPUState *cs, int num)
> +{
> +    char reason[32];
> +
> +    if (should_catch_syscall(num)) {
> +        snprintf(reason, sizeof(reason), "syscall_entry:%x;", num);
> +        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
> +    }
> +}
> +
> +void gdb_syscall_return(CPUState *cs, int num)
> +{
> +    char reason[32];
> +
> +    if (should_catch_syscall(num)) {
> +        snprintf(reason, sizeof(reason), "syscall_return:%x;", num);
> +        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
> +    }
> +}

I'm not super keen on re-introducing snprintf's as we've been slowly
eradicating them from the code base. How about:

    g_autoptr(GString) reason = g_string_printf("syscall_return:%x;", num);
    gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);

> +
> +void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx)
> +{
> +    enum GDBCatchSyscallsState catch_syscalls_state;
> +    const char *param = get_param(params, 0)->data;
> +    GDBSyscallsMask catch_syscalls_mask;
> +    bool catch_syscalls_none;
> +    unsigned int num;
> +    const char *p;
> +

Perhaps a little comment:

  /* terminating with 0/1 to disable/enable all */

> +    catch_syscalls_none = strcmp(param, "0") == 0;
> +    if (catch_syscalls_none || strcmp(param, "1") == 0) {
> +        gdbserver_user_state.catch_syscalls_state =
> +            catch_syscalls_none ? GDB_CATCH_SYSCALLS_NONE :
> +                                  GDB_CATCH_SYSCALLS_ALL;
> +        gdb_put_packet("OK");
> +        return;
> +    }

  /* otherwise decode the following list of syscalls... */

?

> +    if (param[0] == '1' && param[1] == ';') {
> +        catch_syscalls_state = GDB_CATCH_SYSCALLS_SELECTED;
> +        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. */
> +                catch_syscalls_state = GDB_CATCH_SYSCALLS_ALL;

Is this the right thing or maybe we should error because gdb sent us
something strange? In fact you could do:

           if (qemu_strtoui(p, &p, 16, &num) ||
               (*p && *p != ';') ||
               num >= GDB_NR_SYSCALLS) {
               gdb_put_packet("E00");
               return;
           }

and skip the goto err

> +            } else {
> +                set_bit(num, catch_syscalls_mask);
> +            }
> +            if (!*p) {
> +                break;
> +            }

Could this be in the for loop?

  for(p = &param[2]; *p; p++)
  

> +        }
> +        gdbserver_user_state.catch_syscalls_state = catch_syscalls_state;
> +        memcpy(gdbserver_user_state.catch_syscalls_mask, catch_syscalls_mask,
> +               sizeof(catch_syscalls_mask));
> +        gdb_put_packet("OK");
> +        return;
> +    }
> +
> +err:
> +    gdb_put_packet("E00");
> +}
> diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
> index d392e510c59..68b6534130c 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
> @@ -39,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);

This clean-up belongs in a separate patch.

>  }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 1/2] gdbstub: Implement catching syscalls

[Ilya Leoshkevich]

On Thu, 2024-02-01 at 17:15 +0000, Alex Bennée wrote:
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
> 
> > 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          |   2 +
> >  gdbstub/user-target.c        |   5 ++
> >  gdbstub/user.c               | 104
> > ++++++++++++++++++++++++++++++++++-
> >  include/gdbstub/user.h       |  29 +++++++++-
> >  include/user/syscall-trace.h |   7 ++-
> >  6 files changed, 151 insertions(+), 5 deletions(-)

[...]

> > @@ -499,3 +517,87 @@ void gdb_syscall_handling(const char
> > *syscall_packet)
> >      gdb_put_packet(syscall_packet);
> >      gdb_handlesig(gdbserver_state.c_cpu, 0);
> >  }
> > +
> > +static bool should_catch_syscall(int num)
> > +{
> > +    switch (gdbserver_user_state.catch_syscalls_state) {
> > +    case GDB_CATCH_SYSCALLS_NONE:
> > +        return false;
> > +    case GDB_CATCH_SYSCALLS_ALL:
> > +        return true;
> > +    case GDB_CATCH_SYSCALLS_SELECTED:
> > +        if (num < 0 || num >= GDB_NR_SYSCALLS) {
> > +            return false;
> > +        } else {
> > +            return test_bit(num,
> > gdbserver_user_state.catch_syscalls_mask);
> > +        }
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +}
> > +
> > +void gdb_syscall_entry(CPUState *cs, int num)
> > +{
> > +    char reason[32];
> > +
> > +    if (should_catch_syscall(num)) {
> > +        snprintf(reason, sizeof(reason), "syscall_entry:%x;",
> > num);
> > +        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
> > +    }
> > +}
> > +
> > +void gdb_syscall_return(CPUState *cs, int num)
> > +{
> > +    char reason[32];
> > +
> > +    if (should_catch_syscall(num)) {
> > +        snprintf(reason, sizeof(reason), "syscall_return:%x;",
> > num);
> > +        gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);
> > +    }
> > +}
> 
> I'm not super keen on re-introducing snprintf's as we've been slowly
> eradicating them from the code base. How about:
> 
>     g_autoptr(GString) reason = g_string_printf("syscall_return:%x;",
> num);
>     gdb_handlesig_reason(cs, gdb_target_sigtrap(), reason);

Ok.

> > +
> > +void gdb_handle_set_catch_syscalls(GArray *params, void *user_ctx)
> > +{
> > +    enum GDBCatchSyscallsState catch_syscalls_state;
> > +    const char *param = get_param(params, 0)->data;
> > +    GDBSyscallsMask catch_syscalls_mask;
> > +    bool catch_syscalls_none;
> > +    unsigned int num;
> > +    const char *p;
> > +
> 
> Perhaps a little comment:
> 
>   /* terminating with 0/1 to disable/enable all */
> 
> > +    catch_syscalls_none = strcmp(param, "0") == 0;
> > +    if (catch_syscalls_none || strcmp(param, "1") == 0) {
> > +        gdbserver_user_state.catch_syscalls_state =
> > +            catch_syscalls_none ? GDB_CATCH_SYSCALLS_NONE :
> > +                                  GDB_CATCH_SYSCALLS_ALL;
> > +        gdb_put_packet("OK");
> > +        return;
> > +    }
> 
>   /* otherwise decode the following list of syscalls... */
> 
> ?

Ok.

> 
> > +    if (param[0] == '1' && param[1] == ';') {
> > +        catch_syscalls_state = GDB_CATCH_SYSCALLS_SELECTED;
> > +        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. */
> > +                catch_syscalls_state = GDB_CATCH_SYSCALLS_ALL;
> 
> Is this the right thing or maybe we should error because gdb sent us
> something strange? In fact you could do:

I would keep this, because the size of GDBSyscallsMask is chosen
somewhat arbitrarily, and it's nice to have a fallback. The spec allows
this:

    Note that if a syscall not in the list is reported, GDB will still
    filter the event according to its own list from all corresponding
    catch syscall commands. However, it is more efficient to only
    report the requested syscalls. 

>            if (qemu_strtoui(p, &p, 16, &num) ||
>                (*p && *p != ';') ||
>                num >= GDB_NR_SYSCALLS) {
>                gdb_put_packet("E00");
>                return;
>            }
> 
> and skip the goto err
> 
> > +            } else {
> > +                set_bit(num, catch_syscalls_mask);
> > +            }
> > +            if (!*p) {
> > +                break;
> > +            }
> 
> Could this be in the for loop?
> 
>   for(p = &param[2]; *p; p++)

The idea behind this is to catch the empty syscall list, which I
believe is not a valid syntax. I will add a comment.

> > +        }
> > +        gdbserver_user_state.catch_syscalls_state =
> > catch_syscalls_state;
> > +        memcpy(gdbserver_user_state.catch_syscalls_mask,
> > catch_syscalls_mask,
> > +               sizeof(catch_syscalls_mask));
> > +        gdb_put_packet("OK");
> > +        return;
> > +    }
> > +
> > +err:
> > +    gdb_put_packet("E00");
> > +}
> > diff --git a/include/gdbstub/user.h b/include/gdbstub/user.h
> > index d392e510c59..68b6534130c 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
> > @@ -39,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);
> 
> This clean-up belongs in a separate patch.

Ok.

> 
> >  }
> 

[PATCH v2 2/2] tests/tcg: Add the syscall catchpoint gdbstub test

[Ilya Leoshkevich]

Check that adding/removing syscall catchpoints works.

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 | 52 +++++++++++++++++++
 3 files changed, 112 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..8bab12537fc
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/catch-syscalls.py
@@ -0,0 +1,52 @@
+"""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
+    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 v2 2/2] tests/tcg: Add the syscall catchpoint gdbstub test

[Alex Bennée]

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

> Check that adding/removing syscall catchpoints works.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro