Ilya Leoshkevich <i...@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 <i...@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 = ¶m[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 = ¶m[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
