On Thu, 2024-02-01 at 17:15 +0000, Alex Bennée wrote:
> 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(-)
[...]
> > @@ -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 = ¶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:
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 = ¶m[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.
>
> > }
>
