This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: RFA [PATCH] Implement 'catch syscall' for gdbserver
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 19 Sep 2013 02:29:13 -0300
- Subject: Re: RFA [PATCH] Implement 'catch syscall' for gdbserver
- Authentication-results: sourceware.org; auth=none
- References: <1377876388 dot 3041 dot 3 dot camel at soleil>
On Friday, August 30 2013, Philippe Waroquiers wrote:
> This is the 2nd version of the patch implementing 'catch syscalls' for
> gdbserver.
> First version was sent in an RFC (no feedback yet, so here is a completed
> and tested RFA ready version).
> Tested (no regression) on linux amd64, native and gdbserver.
> Manually tested with a patched Valgrind gdbserver.
Thank you very much for this, Philippe :-). I have some comments.
> Here are the changes logs:
> ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * NEWS: Document new QcatchSyscalls packet and its use
> in x86/amd64 linux gdbserver and Valgrind gdbserver.
> * remote.c (PACKET_QCatchSyscalls): New.
> (remote_protocol_features): Add Qcatchsyscalls.
> (remote_set_syscall_catchpoint): New function.
> (remote_parse_stop_reply): New stop reasons syscall_entry
> and syscall_return.
> (init_remote_ops): Registers remote_set_syscall_catchpoint
> and the config commands for [PACKET_QCatchSyscalls.
^
Typo (wrong "[" at the beginning)?
> * common/linux-ptrace.c (linux_check_ptrace_features):
> Detect PTRACE_O_TRACESYSGOOD for gdbserver.
> (ptrace_supports_feature): Initializes ptrace features if needed.
>
> doc/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * gdb.texinfo (General Query Packets): Document new QcatchSyscalls
> packet.
It's QCatchSyscalls (capital "C"), right?
> (Stop Reply Packets): Document new stop reasons syscall_entry and
> syscall_return.
> (Async Records): fixed syscall-return item name.
Capital "F" for "Fixed".
>
> gdbserver/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * target.h (struct target_ops): Add supports_catch_syscall operation.
> * linux-low.h (struct linux_target_ops): Add get_syscall_trapinfo
> operation.
> * linux-low.c (linux_wait_1): Enables, detects and handle
> SYSCALL_SIGTRAP.
I'd rewrite it in the imperative form, i.e., "Enable, detect and handle".
> (gdb_catched_syscall): New function.
> (get_syscall_trapinfo): New function.
> (linux_supports_catch_syscall): New function.
Wrong indentation.
> (struct target_ops linux_target_ops): Set linux_supports_catch_syscall.
> * linux-x86-low.c (x86_get_syscall_trapinfo): New function.
> (struct linux_target_ops the_low_target): Set x86_get_syscall_trapinfo.
> * remote-utils.c (prepare_resume_reply): Handle status kinds
> TARGET_WAITKIND_SYSCALL_ENTRY and TARGET_WAITKIND_SYSCALL_RETURN.
> * server.h: Declare catch_syscalls_p, catched_syscalls_size and
> catched_syscalls.
> * server.c: Define catch_syscalls_p, catched_syscalls_size and
> catched_syscalls.
> (handle_general_set): Handle QCatchSyscalls packet.
> (handle_query): Reports if low target supports QCatchSyscalls.
> * win32-low.c (struct target_ops win32_target_op): Sets NULL
> for supports_catch_syscall.
>
> testsuite/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * gdb.base/catch-syscall.exp: Enables test for x86 and amd64
> gdbserver.
>
> Two specific points that might be worth looking at (after this patch
> probably):
>
> 1. GDB native has a bug in the detection of "syscall entry/return",
> when the catch syscall is disabled when inferior is stopped
> on a syscall entry, and then catch syscall is re-enabled later:
>
> Catchpoint 1 (call to syscall brk), 0x00207ead in brk ()
> from /lib/ld-linux.so.2
> (gdb) break main
> Breakpoint 2 at 0x80486ca: file reach_thread_register.c, line 42.
> (gdb) disa 1
> (gdb) c
> Continuing.
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/libthread_db.so.1".
>
> Breakpoint 2, main () at reach_thread_register.c:42
> 42 pthread_barrier_init(&bar, NULL, 2);
> (gdb) enable
> (gdb) c
> Continuing.
>
> Catchpoint 1 (returned from syscall mmap2), 0x00121416 in __kernel_vsyscall ()
> (gdb) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ should be call to syscall mmap2
> /// after this, all entries are seen as returns, and all returns are seen
> /// as entries.
>
> In GDBSERVER, to differentiate entry from return, I use the
> syscall retcode -ENOSYS.
> This is working properly including for the case above.
> The only problem is that if a syscall is not implemented
> (and so really returns -ENOSYS)
> then GDBSERVER will wrongly report the return of the system call
> as an entry (but subsequent syscall entries/returns will be properly
> reported).
> Maybe there is a better approach, including for fixing
> the 'GDB native catch syscall' ?
Hm, I remember seeing and trying to fix this bug a long time ago. Not
sure if I managed to do it or not (guess not, since I didn't send the
patch, but I remember having something close to the fix). My initial
idea was to record the event (syscall entry or return) inside GDB and
make it decide based on it.
Anyway, I agree it's a bug and that it deserves a bug report :-). Could
you please file it?
> 2. GDB (probably in breakpoint.c) has a bug with catch syscalls
> in remote mode when 'set breakpoint always-inserted on' :
> Disabling all the catch syscalls does not cause
> a call to target_set_syscall_catchpoint with needed = 0,
> which means that QCatchSyscalls:0 is not sent.
> After that, GDBSERVER continues to report various syscalls
> to GDB, that filters them, so no functional bug, only
> efficiency bug.
>
> ----------------- Patch:
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.611
> diff -u -p -r1.611 NEWS
> --- NEWS 27 Aug 2013 05:20:56 -0000 1.611
> +++ NEWS 30 Aug 2013 15:18:08 -0000
> @@ -127,11 +127,21 @@ qXfer:libraries-svr4:read's annex
> necessary for library list updating, resulting in significant
> speedup.
>
> +QCatchSyscalls:1 [;SYSNO]...
> +QCatchSyscalls:0
> + Enable (`QCatchSyscalls:1') or disable (`QCatchSyscalls:0')
> + catching syscalls from the inferior process.
We should not be using ` and ' anymore, now one should use " and ".
> +
> +
> * New features in the GDB remote stub, GDBserver
>
> ** GDBserver now supports target-assisted range stepping. Currently
> enabled on x86/x86_64 GNU/Linux targets.
>
> + ** GDBserver now supports catch syscall catchpoints. Currently
"catch syscall" or "syscall catchpoints", I think.
> + enabled on x86/x86_64 GNU/Linux targets, and in the Valgrind
> + gdbserver.
> +
> ** GDBserver now adds element 'tvar' in the XML in the reply to
> 'qXfer:traceframe-info:read'. It has the id of the collected
> trace state variables.
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.577
> diff -u -p -r1.577 remote.c
> --- remote.c 23 Aug 2013 13:12:17 -0000 1.577
> +++ remote.c 30 Aug 2013 15:18:09 -0000
> @@ -1340,6 +1340,7 @@ enum {
> PACKET_qSupported,
> PACKET_qTStatus,
> PACKET_QPassSignals,
> + PACKET_QCatchSyscalls,
> PACKET_QProgramSignals,
> PACKET_qSearch_memory,
> PACKET_vAttach,
> @@ -1728,6 +1729,93 @@ remote_pass_signals (int numsigs, unsign
> }
> }
>
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> + to report syscalls to GDB. */
I generally prefer to explicitly say which method this function is
implementing. Like, in this case, mention that it is implementing
"target_set_syscall_catchpoint". It makes it easier to find the
definitions of each argument.
> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> + int table_size, int *table)
> +{
> + if (remote_protocol_packets[PACKET_QCatchSyscalls].support != PACKET_DISABLE)
> + {
> + char *catch_packet, *p;
> + enum packet_result result;
> + int n_sysno = 0;
> +
> + if (needed && !any_count)
> + {
> + int i;
> +
> + for (i = 0; i < table_size; i++)
> + if (table[i])
> + n_sysno++;
When would table[i] be 0 in this case? I guess you could use table_size
directly without worrying about n_sysno.
> + }
> +
> + if (remote_debug)
> + fprintf_unfiltered (gdb_stdlog,
> + "remote_set_syscall_catchpoint "
> + "pid %d needed %d any_count %d n_sysno %d\n",
> + pid, needed, any_count, n_sysno);
> + if (needed)
> + {
> + /* Prepare a packet with the sysno list, assuming
> + max 8+1 characters for a sysno. If the resulting
> + packet size is too big, fallback on the non
> + selective packet. */
> + const char *q1 = "QCatchSyscalls:1";
Really nitpicking but you could avoid calling strlen for q1 and replace
it by a size_t variable holding the sizeof (q1) - 1. But this is just
a minor thing.
> + int i;
> + const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
> +
> + catch_packet = xmalloc (maxpktsz);
> + strcpy (catch_packet, q1);
> + if (!any_count)
> + {
> + char *p;
Empty line between variable declaration and code.
> + p = catch_packet;
> + p += strlen(p);
> + for (i = 0; i < table_size; i++)
> + {
> + if (table[i])
Again, I don't see the reason of this check.
> + {
> + xsnprintf(p, catch_packet + maxpktsz - p,
> + ";%x", i);
> + p += strlen(p);
> + }
> + }
> + }
> + if (strlen(catch_packet) > get_remote_packet_size())
> + {
> + /* catch_packet too big. Fallback to less efficient
> + non selective mode, with GDB doing the filtering. */
> + catch_packet[strlen (q1)] = 0;
> + }
Hm, can't you do this check before start filling catch_packet? This
would save some time.
> + }
> + else
> + {
> + catch_packet = xmalloc (strlen ("QCatchSyscalls:0") + 1);
> + strcpy (catch_packet, "QCatchSyscalls:0");
> + }
> +
> + {
> + struct remote_state *rs = get_remote_state ();
> + char *buf = rs->buf;
> +
> + putpkt (catch_packet);
> + getpkt (&rs->buf, &rs->buf_size, 0);
> + result = packet_ok (buf,
> + &remote_protocol_packets[PACKET_QCatchSyscalls]);
> + xfree (catch_packet);
> + if (result == PACKET_OK)
> + return 0;
> + else
> + return -1;
> + }
> + }
> + else
> + return 1; /* not supported */
Would you mind putting this at the beginning of the function? Like:
if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
{
/* Not supported. */
return 1;
}
You would save one level of indentation without losing clarity.
> +}
> +
> +
> /* If 'QProgramSignals' is supported, tell the remote stub what
> signals it should pass through to the inferior when detaching. */
>
> @@ -4016,6 +4104,8 @@ static const struct protocol_feature rem
> PACKET_qXfer_traceframe_info },
> { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
> PACKET_QPassSignals },
> + { "QCatchSyscalls", PACKET_DISABLE, remote_supported_packet,
> + PACKET_QCatchSyscalls },
> { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
> PACKET_QProgramSignals },
> { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
> @@ -5283,7 +5373,8 @@ typedef struct stop_reply
>
> int stopped_by_watchpoint_p;
> CORE_ADDR watch_data_address;
> -
> +
Spurious line with spaces.
> + int syscall;
I know this struct is already messed up, but I would put a comment above
this field to explain its purpose.
I also wonder if all those fields could be turned into bitfields, but
that's another issue not relevant here :-).
> int solibs_changed;
> int replay_event;
>
> @@ -5546,6 +5637,7 @@ remote_parse_stop_reply (char *buf, stru
> event->ptid = null_ptid;
> event->ws.kind = TARGET_WAITKIND_IGNORE;
> event->ws.value.integer = 0;
> + event->syscall = 0;
> event->solibs_changed = 0;
> event->replay_event = 0;
> event->stopped_by_watchpoint_p = 0;
> @@ -5596,6 +5688,22 @@ Packet: '%s'\n"),
> p, buf);
> if (strncmp (p, "thread", p1 - p) == 0)
> event->ptid = read_ptid (++p1, &p);
> + else if (strncmp (p, "syscall_entry", p1 - p) == 0)
> + {
> + ULONGEST sysno;
Empty newline missing.
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + p = unpack_varlen_hex (++p1, &sysno);
> + event->syscall = 1;
> + event->ws.value.syscall_number = (int) sysno;
> + }
> + else if (strncmp (p, "syscall_return", p1 - p) == 0)
> + {
> + ULONGEST sysno;
Empty newline missing.
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
> + p = unpack_varlen_hex (++p1, &sysno);
> + event->syscall = 1;
> + event->ws.value.syscall_number = (int) sysno;
> + }
> else if ((strncmp (p, "watch", p1 - p) == 0)
> || (strncmp (p, "rwatch", p1 - p) == 0)
> || (strncmp (p, "awatch", p1 - p) == 0))
> @@ -5681,6 +5789,11 @@ Packet: '%s'\n"),
> event->ws.kind = TARGET_WAITKIND_LOADED;
> else if (event->replay_event)
> event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
> + else if (event->syscall)
> + {
> + gdb_assert (event->ws.kind == TARGET_WAITKIND_SYSCALL_ENTRY
> + || event->ws.kind == TARGET_WAITKIND_SYSCALL_RETURN);
> + }
No need to open/close brackets.
> else
> {
> event->ws.kind = TARGET_WAITKIND_STOPPED;
> @@ -11452,6 +11565,7 @@ Specify the serial device it is connecte
> remote_ops.to_load = generic_load;
> remote_ops.to_mourn_inferior = remote_mourn;
> remote_ops.to_pass_signals = remote_pass_signals;
> + remote_ops.to_set_syscall_catchpoint = remote_set_syscall_catchpoint;
> remote_ops.to_program_signals = remote_program_signals;
> remote_ops.to_thread_alive = remote_thread_alive;
> remote_ops.to_find_new_threads = remote_threads_info;
> @@ -11946,6 +12060,9 @@ Show the maximum size of the address (in
> add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
> "QPassSignals", "pass-signals", 0);
>
> + add_packet_config_cmd (&remote_protocol_packets[PACKET_QCatchSyscalls],
> + "QCatchSyscalls", "catch-syscalls", 0);
> +
> add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
> "QProgramSignals", "program-signals", 0);
>
> Index: common/linux-ptrace.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 linux-ptrace.c
> --- common/linux-ptrace.c 28 Aug 2013 14:09:31 -0000 1.12
> +++ common/linux-ptrace.c 30 Aug 2013 15:18:09 -0000
> @@ -361,16 +361,17 @@ linux_check_ptrace_features (void)
> return;
> }
>
> -#ifdef GDBSERVER
> - /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
> - PTRACE_O_TRACEVFORKDONE yet. */
> -#else
> - /* Check if the target supports PTRACE_O_TRACESYSGOOD. */
> + /* Check if the target supports PTRACE_O_TRACESYSGOOD, keeping
> + PTRACE_O_TRACEFORK option activated. */
> ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> - (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> + (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> + | PTRACE_O_TRACESYSGOOD));
Any reason why you are keeping PTRACE_O_TRACEFORK set?
> if (ret == 0)
> current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
>
> +#ifdef GDBSERVER
> + /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet. */
> +#else
> /* Check if the target supports PTRACE_O_TRACEVFORKDONE. */
> ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> @@ -474,6 +475,11 @@ linux_enable_event_reporting (pid_t pid)
> static int
> ptrace_supports_feature (int ptrace_options)
> {
> + /* Check if we have initialized the ptrace features for this
> + target. If not, do it now. */
> + if (current_ptrace_options == -1)
> + linux_check_ptrace_features ();
> +
This seems correct to me, but I think you should send it in a separate
patch, because it is not specific to catch syscall IMO.
> gdb_assert (current_ptrace_options >= 0);
>
> return ((current_ptrace_options & ptrace_options) == ptrace_options);
> Index: doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.1105
> diff -u -p -r1.1105 gdb.texinfo
> --- doc/gdb.texinfo 27 Aug 2013 05:20:57 -0000 1.1105
> +++ doc/gdb.texinfo 30 Aug 2013 15:18:15 -0000
> @@ -18681,6 +18681,10 @@ are:
> @tab @code{qSupported}
> @tab Remote communications parameters
>
> +@item @code{catch-syscalls}
> +@tab @code{QCatchSyscalls}
> +@tab @code{catch syscall}
> +
> @item @code{pass-signals}
> @tab @code{QPassSignals}
> @tab @code{handle @var{signal}}
> @@ -38077,6 +38081,11 @@ The currently defined stop reasons are:
> The packet indicates a watchpoint hit, and @var{r} is the data address, in
> hex.
>
> +@item syscall_entry
> +@itemx syscall_return
> +The packet indicates a syscall entry or return, and @var{r} is the
> +syscall number, in hex.
> +
> @cindex shared library events, remote reply
> @item library
> The packet indicates that the loaded libraries have changed.
> @@ -38447,6 +38456,44 @@ by supplying an appropriate @samp{qSuppo
> Use of this packet is controlled by the @code{set non-stop} command;
> @pxref{Non-Stop Mode}.
>
> +@item QCatchSyscalls:1 @r{[};@var{sysno}@r{]}@dots{}
> +@itemx QCatchSyscalls:0
> +@cindex catch syscalls from inferior, remote request
> +@cindex @samp{QCatchSyscalls} packet
> +@anchor{QCatchSyscalls}
> +Enable (@samp{QCatchSyscalls:1}) or disable (@samp{QCatchSyscalls:0})
> +catching syscalls from the inferior process.
> +
> +For @samp{QCatchSyscalls:1}, each listed syscall @var{sysno} (encoded
> +in hex) should be reported to @value{GDBN}. If no syscall @var{sysno}
> +is listed, every system call should be reported.
> +
> +Note that if a syscall not member of the list is reported, @value{GDBN}
> +will filter it if this signal is not catched. It is however more efficient
> +to only report the needed syscalls.
> +
> +Multiple @samp{QCatchSyscalls:1} packets do not
> +combine; any earlier @samp{QCatchSyscalls:1} list is completely replaced by the
> +new list.
> +
> +Reply:
> +@table @samp
> +@item OK
> +The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred. @var{nn} are hex digits.
> +
> +@item @w{}
> +An empty reply indicates that @samp{QCatchSyscalls} is not supported by
> +the stub.
> +@end table
> +
> +Use of this packet is controlled by the @code{set remote catch-syscalls}
> +command (@pxref{Remote Configuration, set remote catch-syscalls}).
> +This packet is not probed by default; the remote stub must request it,
> +by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
> +
> @item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
> @cindex pass signals to inferior, remote request
> @cindex @samp{QPassSignals} packet
> @@ -38810,6 +38857,11 @@ These are the currently defined stub fea
> @tab @samp{-}
> @tab Yes
>
> +@item @samp{QCatchSyscalls}
> +@tab No
> +@tab @samp{-}
> +@tab Yes
> +
> @item @samp{QPassSignals}
> @tab No
> @tab @samp{-}
> @@ -38970,6 +39022,10 @@ packet (@pxref{qXfer fdpic loadmap read}
> The remote stub understands the @samp{QNonStop} packet
> (@pxref{QNonStop}).
>
> +@item QCatchSyscalls
> +The remote stub understands the @samp{QCatchSyscalls} packet
> +(@pxref{QCatchSyscalls}).
> +
> @item QPassSignals
> The remote stub understands the @samp{QPassSignals} packet
> (@pxref{QPassSignals}).
> Index: gdbserver/linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 linux-low.c
> --- gdbserver/linux-low.c 28 Aug 2013 17:40:58 -0000 1.246
> +++ gdbserver/linux-low.c 30 Aug 2013 15:18:16 -0000
> @@ -72,6 +72,11 @@
> #define W_STOPCODE(sig) ((sig) << 8 | 0x7f)
> #endif
>
> +/* Unlike other extended result codes, WSTOPSIG (status) on
> + PTRACE_O_TRACESYSGOOD syscall events doesn't return SIGTRAP, but
> + instead SIGTRAP with bit 7 set. */
> +#define SYSCALL_SIGTRAP (SIGTRAP | 0x80)
> +
> /* This is the kernel's hard limit. Not to be confused with
> SIGRTMIN. */
> #ifndef __SIGRTMIN
> @@ -479,6 +484,33 @@ get_pc (struct lwp_info *lwp)
> return pc;
> }
>
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL. */
Missing explanation of what the function does.
> +
> +static void
> +get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
> +{
> + struct thread_info *saved_inferior;
> + struct regcache *regcache;
> +
> + if (the_low_target.get_syscall_trapinfo == NULL)
> + {
> + *sysno = 0;
> + *sysret = 0;
Aren't you missing a "return" here?
In fact, is there any way for "the_low_target.get_syscall_trapinfo" to
be NULL? It seems to me that this is a necessary condition for this
function to be called (see my comment about putting an assert at
server.c:handle_general_set). So maybe you should put an assert here to
check that the_low_target.get_syscall_trapinfo != NULL. WDYT?
> + }
> +
> + saved_inferior = current_inferior;
> + current_inferior = get_lwp_thread (lwp);
> +
> + regcache = get_thread_regcache (current_inferior, 1);
> + (*the_low_target.get_syscall_trapinfo) (regcache, sysno, sysret);
> +
> + if (debug_threads)
> + fprintf (stderr, "get_syscall_trapinfo sysno %d sysret %d\n",
> + *sysno, *sysret);
> +
> + current_inferior = saved_inferior;
> +}
> +
> /* This function should only be called if LWP got a SIGTRAP.
> The SIGTRAP could mean several things.
>
> @@ -2246,6 +2278,29 @@ linux_stabilize_threads (void)
> }
> }
>
> +/* Returns 1 if GDB is interested in the event_child syscall.
> + Only to be called when stopped reason is SIGTRAP_SYSCALL. */
> +
> +static int
> +gdb_catched_syscall (struct lwp_info *event_child)
s/catched/caught/g ?
Maybe also rename the function to "catch_current_syscall_p"? Or
something to give the idea that we are checking if this specific syscall
is interesting for us.
> +{
> + int i;
> + int sysno, sysret;
> +
> + if (!catch_syscalls_p)
I think you could rename this to "catching_syscalls_p".
> + return 0;
> +
> + if (catched_syscalls_size == 0)
> + return 1;
> +
> + get_syscall_trapinfo (event_child, &sysno, &sysret);
> + for (i = 0; i < catched_syscalls_size; i++)
> + if (catched_syscalls[i] == sysno)
> + return 1;
Also, I think the names of the variables are a bit misleading.
"catched_syscalls" does not represent syscalls that were caught, but
rather syscalls that are to be caught. Maybe rename them to
"syscalls_to_be_caught" or something?
> +
> + return 0;
> +}
> +
> /* Wait for process, returns status. */
>
> static ptid_t
> @@ -2527,6 +2582,19 @@ Check if we're already there.\n",
>
> /* Check whether GDB would be interested in this event. */
>
> + /* Check if GDB is interested in this syscall. */
> + if (WIFSTOPPED (w)
> + && WSTOPSIG (w) == SYSCALL_SIGTRAP
Spurious whitespace at the end. This line could also be joined with the
line above.
> + && !gdb_catched_syscall (event_child))
> + {
> + if (debug_threads)
> + fprintf (stderr, "Ignored syscall for LWP %ld.\n",
> + lwpid_of (event_child));
> + linux_resume_one_lwp (event_child, event_child->stepping,
> + 0, NULL);
> + goto retry;
> + }
> +
> /* If GDB is not interested in this signal, don't stop other
> threads, and don't report it to GDB. Just resume the inferior
> right away. We do this for threading-related signals as well as
> @@ -2705,7 +2773,18 @@ Check if we're already there.\n",
>
> ourstatus->kind = TARGET_WAITKIND_STOPPED;
>
> - if (current_inferior->last_resume_kind == resume_stop
> + if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
> + {
> + int sysret;
> +
> + get_syscall_trapinfo (event_child,
> + &ourstatus->value.syscall_number, &sysret);
> + if (sysret == -ENOSYS)
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + else
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;
It would be good to include a comment here explaining why you're using
-ENOSYS to determine whether it's a syscall entry or return.
> + }
> + else if (current_inferior->last_resume_kind == resume_stop
> && WSTOPSIG (w) == SIGSTOP)
> {
> /* A thread that has been requested to stop by GDB with vCont;t,
> @@ -3267,7 +3346,8 @@ lwp %ld wants to get out of fast tracepo
> lwp->stopped = 0;
> lwp->stopped_by_watchpoint = 0;
> lwp->stepping = step;
> - ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (lwp),
> + ptrace (step ? PTRACE_SINGLESTEP : catch_syscalls_p ? PTRACE_SYSCALL : PTRACE_CONT,
> + lwpid_of (lwp),
The line above is too long. I am also not fond of using
"double-ternary" :-/. Maybe it's just me, but I find them quite
confusing sometimes.
> (PTRACE_TYPE_ARG3) 0,
> /* Coerce to a uintptr_t first to avoid potential gcc warning
> of coercing an 8 byte integer to a 4 byte pointer. */
> @@ -5106,6 +5186,13 @@ linux_process_qsupported (const char *qu
> }
>
> static int
> +linux_supports_catch_syscall (void)
> +{
> + return the_low_target.get_syscall_trapinfo != NULL
> + && linux_supports_tracesysgood();
You could use a parenthesis here to properly indent the check.
> +}
> +
> +static int
> linux_supports_tracepoints (void)
> {
> if (*the_low_target.supports_tracepoints == NULL)
> @@ -5796,6 +5883,7 @@ static struct target_ops linux_target_op
> linux_common_core_of_thread,
> linux_read_loadmap,
> linux_process_qsupported,
> + linux_supports_catch_syscall,
> linux_supports_tracepoints,
> linux_read_pc,
> linux_write_pc,
> Index: gdbserver/linux-low.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
> retrieving revision 1.65
> diff -u -p -r1.65 linux-low.h
> --- gdbserver/linux-low.h 22 Aug 2013 23:46:29 -0000 1.65
> +++ gdbserver/linux-low.h 30 Aug 2013 15:18:16 -0000
> @@ -187,6 +187,12 @@ struct linux_target_ops
> /* Hook to support target specific qSupported. */
> void (*process_qsupported) (const char *);
>
> + /* Fill SYSNO with the syscall nr trapped. Fill SYSRET with the
^
Double space after dot.
> + return code. Only to be called when inferior is stopped
> + due to SYSCALL_SIGTRAP. */
> + void (*get_syscall_trapinfo) (struct regcache *regcache,
> + int *sysno, int *sysret);
> +
> /* Returns true if the low target supports tracepoints. */
> int (*supports_tracepoints) (void);
>
> Index: gdbserver/linux-x86-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-low.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 linux-x86-low.c
> --- gdbserver/linux-x86-low.c 12 Jun 2013 16:05:39 -0000 1.49
> +++ gdbserver/linux-x86-low.c 30 Aug 2013 15:18:16 -0000
> @@ -1472,6 +1472,32 @@ x86_arch_setup (void)
> current_process ()->tdesc = x86_linux_read_description ();
> }
>
> +static void
> +x86_get_syscall_trapinfo (struct regcache *regcache, int *sysno, int *sysret)
> +{
> + int use_64bit = register_size (regcache->tdesc, 0) == 8;
> +
> + if (use_64bit)
> + {
> + long l_sysno;
> + long l_sysret;
Empty newline missing.
> + collect_register_by_name (regcache, "orig_rax", &l_sysno);
> + collect_register_by_name (regcache, "rax", &l_sysret);
> + *sysno = (int) l_sysno;
> + *sysret = (int) l_sysret;
> + }
> + else
> + {
> + int l_sysno;
> + int l_sysret;
Empty newline missing.
> + collect_register_by_name (regcache, "orig_eax", &l_sysno);
> + collect_register_by_name (regcache, "eax", &l_sysret);
> + *sysno = (int) l_sysno;
> + *sysret = (int) l_sysret;
> + }
> +
Spurious newline.
> +}
> +
> static int
> x86_supports_tracepoints (void)
> {
> @@ -3321,6 +3347,7 @@ struct linux_target_ops the_low_target =
> x86_linux_new_thread,
> x86_linux_prepare_to_resume,
> x86_linux_process_qsupported,
> + x86_get_syscall_trapinfo,
> x86_supports_tracepoints,
> x86_get_thread_area,
> x86_install_fast_tracepoint_jump_pad,
> Index: gdbserver/remote-utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 remote-utils.c
> --- gdbserver/remote-utils.c 1 Jul 2013 11:19:27 -0000 1.98
> +++ gdbserver/remote-utils.c 30 Aug 2013 15:18:16 -0000
> @@ -1319,12 +1319,17 @@ prepare_resume_reply (char *buf, ptid_t
> switch (status->kind)
> {
> case TARGET_WAITKIND_STOPPED:
> + case TARGET_WAITKIND_SYSCALL_ENTRY:
> + case TARGET_WAITKIND_SYSCALL_RETURN:
> {
> struct thread_info *saved_inferior;
> const char **regp;
> struct regcache *regcache;
>
> - sprintf (buf, "T%02x", status->value.sig);
> + if (status->kind == TARGET_WAITKIND_STOPPED)
> + sprintf (buf, "T%02x", status->value.sig);
> + else
> + sprintf (buf, "T%02x", SIGTRAP);
Again, maybe it's just me, but would you mind putting a comment here
explainig why you're using SIGTRAP directly if status->kind ==
TARGET_WAITKIND_SYSCALL_{ENTRY,RETURN} ?
> buf += strlen (buf);
>
> saved_inferior = current_inferior;
> @@ -1335,6 +1340,16 @@ prepare_resume_reply (char *buf, ptid_t
>
> regcache = get_thread_regcache (current_inferior, 1);
>
> + if (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> + || status->kind == TARGET_WAITKIND_SYSCALL_RETURN)
> + {
> + sprintf (buf, "%s:%x;",
> + status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> + ? "syscall_entry" : "syscall_return",
> + status->value.syscall_number);
> + buf += strlen (buf);
> + }
> +
> if (the_target->stopped_by_watchpoint != NULL
> && (*the_target->stopped_by_watchpoint) ())
> {
> Index: gdbserver/server.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 server.c
> --- gdbserver/server.c 28 Aug 2013 17:40:58 -0000 1.196
> +++ gdbserver/server.c 30 Aug 2013 15:18:17 -0000
> @@ -71,6 +71,9 @@ int debug_threads;
> int debug_hw_points;
>
> int pass_signals[GDB_SIGNAL_LAST];
> +int catch_syscalls_p;
> +int catched_syscalls_size;
size_t? :-)
(I know, the interface should be changed as well, but...)
> +int *catched_syscalls;
> int program_signals[GDB_SIGNAL_LAST];
> int program_signals_p;
>
> @@ -507,6 +510,46 @@ handle_general_set (char *own_buf)
> return;
> }
>
> + if (strncmp ("QCatchSyscalls:1", own_buf, strlen ("QCatchSyscalls:1")) == 0)
> + {
> + int i;
> + const char *p;
> + CORE_ADDR sysno;
> +
I am not very familiar with the remote protocol, but IMHO you could do
an assert here to check if target_supports_catch_syscall returns 1,
WDYT? IOW, we shouldn't get here if it returns 0, right?
Or is there some sort of mechanism intended to overwrite the initial
probing of features and force unsupported features to be enabled? In
that case, I guess the assert would trigger every time... Not sure what
to do.
> + catch_syscalls_p = 1;
> + if (catched_syscalls != NULL)
> + {
> + free (catched_syscalls);
xfree.
> + catched_syscalls = NULL;
> + }
> + catched_syscalls_size = 0;
> + p = own_buf + strlen("QCatchSyscalls:1");
> + while (*p)
> + {
> + if (*p++ == ';')
> + catched_syscalls_size++;
> + }
> + if (catched_syscalls_size > 0)
> + {
> + catched_syscalls = xmalloc (catched_syscalls_size * sizeof (int));
> + p = strchr(own_buf, ';') + 1;
> + for (i = 0; i < catched_syscalls_size; i++)
> + {
> + p = decode_address_to_semicolon (&sysno, p);
> + catched_syscalls [i] = (int) sysno;
^
Spurious whitespace.
> + }
> + }
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> + if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> + {
> + catch_syscalls_p = 0;
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
> {
> int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1740,6 +1783,9 @@ handle_query (char *own_buf, int packet_
> "PacketSize=%x;QPassSignals+;QProgramSignals+",
> PBUFSIZ - 1);
>
> + if (target_supports_catch_syscall())
Missing space between function name and parenthesis.
> + strcat (own_buf, ";QCatchSyscalls+");
> +
> if (the_target->qxfer_libraries_svr4 != NULL)
> strcat (own_buf, ";qXfer:libraries-svr4:read+"
> ";augmented-libraries-svr4-read+");
> Index: gdbserver/server.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
> retrieving revision 1.109
> diff -u -p -r1.109 server.h
> --- gdbserver/server.h 1 Jul 2013 11:28:30 -0000 1.109
> +++ gdbserver/server.h 30 Aug 2013 15:18:17 -0000
> @@ -230,6 +230,15 @@ extern int server_waiting;
> extern int debug_threads;
> extern int debug_hw_points;
> extern int pass_signals[];
> +
> +/* 1 if some (or all) syscalls are catched. */
I'd rewrite this comment:
/* Set to 1 if we are catching syscalls, zero otherwise. */
> +extern int catch_syscalls_p;
> +/* catched_syscalls is the list of syscalls to report to GDB.
> + If catch_syscalls_p and catched_syscalls == NULL, it means
> + all syscalls must be reported. */
> +extern int catched_syscalls_size;
> +extern int *catched_syscalls;
> +
> extern int program_signals[];
> extern int program_signals_p;
>
> Index: gdbserver/target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
> retrieving revision 1.70
> diff -u -p -r1.70 target.h
> --- gdbserver/target.h 19 Aug 2013 16:54:11 -0000 1.70
> +++ gdbserver/target.h 30 Aug 2013 15:18:17 -0000
> @@ -267,6 +267,10 @@ struct target_ops
> /* Target specific qSupported support. */
> void (*process_qsupported) (const char *);
>
> + /* Return 1 if the target supports catch syscall, 0 (or leave the
> + callback NULL) otherwise. */
> + int (*supports_catch_syscall) (void);
> +
> /* Return 1 if the target supports tracepoints, 0 (or leave the
> callback NULL) otherwise. */
> int (*supports_tracepoints) (void);
> @@ -413,6 +417,10 @@ int kill_inferior (int);
> the_target->process_qsupported (query); \
> } while (0)
>
> +#define target_supports_catch_syscall() \
> + (the_target->supports_catch_syscall ? \
> + (*the_target->supports_catch_syscall) () : 0)
> +
> #define target_supports_tracepoints() \
> (the_target->supports_tracepoints \
> ? (*the_target->supports_tracepoints) () : 0)
> Index: gdbserver/win32-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 win32-low.c
> --- gdbserver/win32-low.c 2 Jul 2013 11:59:24 -0000 1.66
> +++ gdbserver/win32-low.c 30 Aug 2013 15:18:17 -0000
> @@ -1813,6 +1813,7 @@ static struct target_ops win32_target_op
> NULL, /* core_of_thread */
> NULL, /* read_loadmap */
> NULL, /* process_qsupported */
> + NULL, /* supports_catch_syscall */
> NULL, /* supports_tracepoints */
> NULL, /* read_pc */
> NULL, /* write_pc */
> Index: testsuite/gdb.base/catch-syscall.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-syscall.exp,v
> retrieving revision 1.19
> diff -u -p -r1.19 catch-syscall.exp
> --- testsuite/gdb.base/catch-syscall.exp 22 Aug 2013 20:32:54 -0000 1.19
> +++ testsuite/gdb.base/catch-syscall.exp 30 Aug 2013 15:18:17 -0000
> @@ -19,7 +19,7 @@
> # It was written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
> # on September/2008.
>
> -if { [is_remote target] || ![isnative] } then {
> +if { ![isnative] } then {
> continue
> }
>
> @@ -28,6 +28,14 @@ if {![istarget "hppa*-hp-hpux*"] && ![is
> continue
> }
>
> +# This shall be updated whenever QCatchSyscalls packet support is implemented
> +# on some gdbserver architecture.
> +if { [is_remote target]
> + && ![istarget "x86_64-*-linux*"]
> + && ![istarget "i\[34567\]86-*-linux*"] } {
> + continue
> +}
Hm, am I too sleepy or this test does nothing at all? :-).
> +
> # This shall be updated whenever 'catch syscall' is implemented
> # on some architecture.
> #if { ![istarget "i\[34567\]86-*-linux*"]
Otherwise, the patch looks pretty good to me! I have tested it here and
as far as I have seen it is working OK. I hope my comments help you
with getting the patch approved. I am not a maintainer, so you will
need to wait for one to review the patch as well :-).
Thank you very much for working on this.
--
Sergio