* RFA [PATCH v3] Implement 'catch syscall' for gdbserver
@ 2013-09-21 20:55 Philippe Waroquiers
2013-09-21 21:03 ` Eli Zaretskii
` (4 more replies)
0 siblings, 5 replies; 30+ messages in thread
From: Philippe Waroquiers @ 2013-09-21 20:55 UTC (permalink / raw)
To: gdb-patches
This is version 3 of the patch implementing catch syscall for gdbserver.
This integrates the review comments given by Eli Zaretskii
and Sergio Durigan Junior.
Difference with version 2 is doc and code cleanup, renamed various
variables and functions, added some clarification comments,
avoid use of catched. No functional change.
No regression on linux/x86-64, native and gdbserver mode.
Manually tested with a (patched) Valgrind gdbserver.
Ok to commit ?
Thanks
Philippe
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.
* 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.
(Stop Reply Packets): Document new stop reasons syscall_entry and
syscall_return.
(Async Records): Fixed syscall-return item name.
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): Enable, detect and handle
SYSCALL_SIGTRAP.
(gdb_catch_this_syscall_p): New function.
(get_syscall_trapinfo): New function.
(linux_supports_catch_syscall): New function.
(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 catching_syscalls_p, syscalls_to_catch_size and
syscalls_to_catch.
* server.c: Define catching_syscalls_p, syscalls_to_catch_size and
syscalls_to_catch.
(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.
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.614
diff -u -p -r1.614 NEWS
--- NEWS 16 Sep 2013 17:47:29 -0000 1.614
+++ NEWS 21 Sep 2013 20:38:09 -0000
@@ -139,11 +139,20 @@ 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.
+
+
* 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. Currently 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 21 Sep 2013 20:38:10 -0000
@@ -1340,6 +1340,7 @@ enum {
PACKET_qSupported,
PACKET_qTStatus,
PACKET_QPassSignals,
+ PACKET_QCatchSyscalls,
PACKET_QProgramSignals,
PACKET_qSearch_memory,
PACKET_vAttach,
@@ -1728,6 +1729,96 @@ remote_pass_signals (int numsigs, unsign
}
}
+/* If 'QCatchSyscalls' is supported, tell the remote stub
+ to report syscalls to GDB. */
+
+static int
+remote_set_syscall_catchpoint (int pid, int needed, int any_count,
+ int table_size, int *table)
+{
+ char *catch_packet, *p;
+ enum packet_result result;
+ int n_sysno = 0;
+
+ if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
+ return 1; /* not supported */
+
+ if (needed && !any_count)
+ {
+ int i;
+
+ /* Count how many syscalls are to be caught (table[sysno] != 0). */
+ for (i = 0; i < table_size; i++)
+ if (table[i])
+ 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";
+ int i;
+ const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
+
+ catch_packet = xmalloc (maxpktsz);
+ strcpy (catch_packet, q1);
+ if (!any_count)
+ {
+ char *p;
+
+ p = catch_packet;
+ p += strlen(p);
+
+ /* Add in catch_packet each syscall to be caught (table[i] != 0). */
+ for (i = 0; i < table_size; i++)
+ {
+ if (table[i])
+ {
+ 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;
+ }
+ }
+ 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;
+ }
+}
+
+
+
/* If 'QProgramSignals' is supported, tell the remote stub what
signals it should pass through to the inferior when detaching. */
@@ -4016,6 +4107,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,
@@ -5284,6 +5377,11 @@ typedef struct stop_reply
int stopped_by_watchpoint_p;
CORE_ADDR watch_data_address;
+ /* Set to 1 if the stop_reply is for a syscall entry or
+ return. The field ws.value.syscall_number then identifies
+ the syscall. */
+ int syscall;
+
int solibs_changed;
int replay_event;
@@ -5546,6 +5644,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 +5695,24 @@ 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;
+
+ 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;
+
+ 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 +5798,9 @@ 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);
else
{
event->ws.kind = TARGET_WAITKIND_STOPPED;
@@ -11452,6 +11572,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 +12067,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 21 Sep 2013 20:38:10 -0000
@@ -361,16 +361,18 @@ 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.
+ We keep PTRACE_O_TRACEFORK option activated as a fork
+ event notification is expected after my_waitpid below. */
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));
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 +476,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 ();
+
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.1112
diff -u -p -r1.1112 gdb.texinfo
--- doc/gdb.texinfo 16 Sep 2013 18:00:34 -0000 1.1112
+++ doc/gdb.texinfo 21 Sep 2013 20:38:14 -0000
@@ -18752,6 +18752,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}}
@@ -38147,6 +38151,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.
@@ -38517,6 +38526,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 syscall is not caught. 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
@@ -38880,6 +38927,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{-}
@@ -39040,6 +39092,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.248
diff -u -p -r1.248 linux-low.c
--- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
+++ gdbserver/linux-low.c 21 Sep 2013 20:38:15 -0000
@@ -74,6 +74,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
@@ -481,6 +486,36 @@ get_pc (struct lwp_info *lwp)
return pc;
}
+/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
+ Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
+ return code. */
+
+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;
+ return;
+ }
+
+ 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.
@@ -2248,6 +2283,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_catch_this_syscall_p (struct lwp_info *event_child)
+{
+ int i;
+ int sysno, sysret;
+
+ if (!catching_syscalls_p)
+ return 0;
+
+ if (syscalls_to_catch_size == 0)
+ return 1;
+
+ get_syscall_trapinfo (event_child, &sysno, &sysret);
+ for (i = 0; i < syscalls_to_catch_size; i++)
+ if (syscalls_to_catch[i] == sysno)
+ return 1;
+
+ return 0;
+}
+
/* Wait for process, returns status. */
static ptid_t
@@ -2529,6 +2587,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
+ && !gdb_catch_this_syscall_p (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
@@ -2707,7 +2778,24 @@ 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);
+ /* Differentiate entry from return using the return code
+ -ENOSYS. This is not ideal as a syscall return from a not
+ implemented syscall will be seen as an entry. However, this
+ resists well to enabling/disabling catch syscall at various
+ moment. A better way to differentiate entry/return in GDB
+ and GDBSERVER would be nice. */
+ if (sysret == -ENOSYS)
+ ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
+ else
+ ourstatus->kind = TARGET_WAITKIND_SYSCALL_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,
@@ -3269,7 +3357,10 @@ 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
+ : catching_syscalls_p ? PTRACE_SYSCALL
+ : PTRACE_CONT,
+ lwpid_of (lwp),
(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. */
@@ -5108,6 +5199,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());
+}
+
+static int
linux_supports_tracepoints (void)
{
if (*the_low_target.supports_tracepoints == NULL)
@@ -5798,6 +5896,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 21 Sep 2013 20:38:15 -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
+ 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.51
diff -u -p -r1.51 linux-x86-low.c
--- gdbserver/linux-x86-low.c 5 Sep 2013 20:40:58 -0000 1.51
+++ gdbserver/linux-x86-low.c 21 Sep 2013 20:38:15 -0000
@@ -1474,6 +1474,33 @@ 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;
+
+ 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;
+
+ 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;
+ }
+}
+
static int
x86_supports_tracepoints (void)
{
@@ -3323,6 +3350,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.99
diff -u -p -r1.99 remote-utils.c
--- gdbserver/remote-utils.c 5 Sep 2013 20:41:55 -0000 1.99
+++ gdbserver/remote-utils.c 21 Sep 2013 20:38:15 -0000
@@ -1321,12 +1321,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);
buf += strlen (buf);
saved_inferior = current_inferior;
@@ -1337,6 +1342,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.201
diff -u -p -r1.201 server.c
--- gdbserver/server.c 18 Sep 2013 01:59:59 -0000 1.201
+++ gdbserver/server.c 21 Sep 2013 20:38:15 -0000
@@ -74,6 +74,9 @@ int debug_threads;
int debug_hw_points;
int pass_signals[GDB_SIGNAL_LAST];
+int catching_syscalls_p;
+int syscalls_to_catch_size;
+int *syscalls_to_catch;
int program_signals[GDB_SIGNAL_LAST];
int program_signals_p;
@@ -511,6 +514,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;
+
+ catching_syscalls_p = 1;
+ if (syscalls_to_catch != NULL)
+ {
+ xfree (syscalls_to_catch);
+ syscalls_to_catch = NULL;
+ }
+ syscalls_to_catch_size = 0;
+ p = own_buf + strlen("QCatchSyscalls:1");
+ while (*p)
+ {
+ if (*p++ == ';')
+ syscalls_to_catch_size++;
+ }
+ if (syscalls_to_catch_size > 0)
+ {
+ syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
+ p = strchr(own_buf, ';') + 1;
+ for (i = 0; i < syscalls_to_catch_size; i++)
+ {
+ p = decode_address_to_semicolon (&sysno, p);
+ syscalls_to_catch[i] = (int) sysno;
+ }
+ }
+ strcpy (own_buf, "OK");
+ return;
+ }
+
+ if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
+ {
+ catching_syscalls_p = 0;
+ strcpy (own_buf, "OK");
+ return;
+ }
+
if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
{
int numsigs = (int) GDB_SIGNAL_LAST, i;
@@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
"PacketSize=%x;QPassSignals+;QProgramSignals+",
PBUFSIZ - 1);
+ if (target_supports_catch_syscall ())
+ 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.118
diff -u -p -r1.118 server.h
--- gdbserver/server.h 5 Sep 2013 20:45:39 -0000 1.118
+++ gdbserver/server.h 21 Sep 2013 20:38:15 -0000
@@ -115,6 +115,15 @@ extern int server_waiting;
extern int debug_threads;
extern int debug_hw_points;
extern int pass_signals[];
+
+/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise. */
+extern int catching_syscalls_p;
+/* syscalls_to_catch is the list of syscalls to report to GDB.
+ If catching_syscalls_p and syscalls_to_catch == NULL, it means
+ all syscalls must be reported. */
+extern int syscalls_to_catch_size;
+extern int *syscalls_to_catch;
+
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.71
diff -u -p -r1.71 target.h
--- gdbserver/target.h 5 Sep 2013 20:41:22 -0000 1.71
+++ gdbserver/target.h 21 Sep 2013 20:38:15 -0000
@@ -268,6 +268,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);
@@ -414,6 +418,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.69
diff -u -p -r1.69 win32-low.c
--- gdbserver/win32-low.c 5 Sep 2013 20:45:39 -0000 1.69
+++ gdbserver/win32-low.c 21 Sep 2013 20:38:16 -0000
@@ -1824,6 +1824,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 21 Sep 2013 20:38:16 -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
+}
+
# This shall be updated whenever 'catch syscall' is implemented
# on some architecture.
#if { ![istarget "i\[34567\]86-*-linux*"]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
@ 2013-09-21 21:03 ` Eli Zaretskii
2013-09-23 11:51 ` Agovic, Sanimir
` (3 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Eli Zaretskii @ 2013-09-21 21:03 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Sat, 21 Sep 2013 22:55:07 +0200
>
> This is version 3 of the patch implementing catch syscall for gdbserver.
> This integrates the review comments given by Eli Zaretskii
> and Sergio Durigan Junior.
>
> Difference with version 2 is doc and code cleanup, renamed various
> variables and functions, added some clarification comments,
> avoid use of catched. No functional change.
>
> No regression on linux/x86-64, native and gdbserver mode.
> Manually tested with a (patched) Valgrind gdbserver.
>
> Ok to commit ?
OK for the documentation part.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
2013-09-21 21:03 ` Eli Zaretskii
@ 2013-09-23 11:51 ` Agovic, Sanimir
2013-09-23 19:32 ` Philippe Waroquiers
2013-09-27 13:25 ` [COMMIT PATCH] remote.c: Remove unnecessary fields from 'struct stop_reply'. (was: Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
` (2 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Agovic, Sanimir @ 2013-09-23 11:51 UTC (permalink / raw)
To: 'Philippe Waroquiers'; +Cc: gdb-patches
Hello Philippe,
you may want to update the wiki [1] page about your progress wrt local/remote feature parity.
A minor comment below.
[1] https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity
-Sanimir
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf
> Of Philippe Waroquiers
> Sent: Saturday, September 21, 2013 10:55 PM
> To: gdb-patches@sourceware.org
> Subject: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
>
> Index: gdbserver/linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 linux-low.c
> --- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
> +++ gdbserver/linux-low.c 21 Sep 2013 20:38:15 -0000
> @@ -74,6 +74,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
> @@ -481,6 +486,36 @@ get_pc (struct lwp_info *lwp)
> return pc;
> }
>
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
> + Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
> + return code. */
> +
> +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;
> + return;
> + }
>
Is it sufficient to assign sysno/sysret to 0 to indicate missing 'catch syscall'
functionality? Both values seem legal to me.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-23 11:51 ` Agovic, Sanimir
@ 2013-09-23 19:32 ` Philippe Waroquiers
2013-09-24 7:07 ` Agovic, Sanimir
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Waroquiers @ 2013-09-23 19:32 UTC (permalink / raw)
To: Agovic, Sanimir; +Cc: gdb-patches
On Mon, 2013-09-23 at 11:50 +0000, Agovic, Sanimir wrote:
> Hello Philippe,
>
> > + if (the_low_target.get_syscall_trapinfo == NULL)
> > + {
> > + *sysno = 0;
> > + *sysret = 0;
> > + return;
> > + }
> >
> Is it sufficient to assign sysno/sysret to 0 to indicate missing 'catch syscall'
> functionality? Both values seem legal to me.
The idea is that will be used in case the user forces the use of the
QCatchSyscalls packet. So, I think it is better to return "valid" (but
not used) values : I do not think there is a syscall nr 0.
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-23 19:32 ` Philippe Waroquiers
@ 2013-09-24 7:07 ` Agovic, Sanimir
2013-09-25 16:55 ` Sergio Durigan Junior
0 siblings, 1 reply; 30+ messages in thread
From: Agovic, Sanimir @ 2013-09-24 7:07 UTC (permalink / raw)
To: 'Philippe Waroquiers'; +Cc: gdb-patches
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf
> Of Philippe Waroquiers
> Sent: Monday, September 23, 2013 09:33 PM
> To: Agovic, Sanimir
> Cc: gdb-patches@sourceware.org
> Subject: RE: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
>
> On Mon, 2013-09-23 at 11:50 +0000, Agovic, Sanimir wrote:
> > Hello Philippe,
> >
>
> > > + if (the_low_target.get_syscall_trapinfo == NULL)
> > > + {
> > > + *sysno = 0;
> > > + *sysret = 0;
> > > + return;
> > > + }
> > >
> > Is it sufficient to assign sysno/sysret to 0 to indicate missing 'catch syscall'
> > functionality? Both values seem legal to me.
> The idea is that will be used in case the user forces the use of the
> QCatchSyscalls packet. So, I think it is better to return "valid" (but
> not used) values : I do not think there is a syscall nr 0.
>
gdb -batch -ex 'set architecture i386:x86-64' -ex 'catch syscall 0'
The target architecture is assumed to be i386:x86-64
Catchpoint 1 (syscall 'read' [0])
I just wanted to make sure that we do not report any false syscalls.
Thus I checked syscall 0 in gdb on amd64 which is mapped to 'read'.
-Sanimir
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-24 7:07 ` Agovic, Sanimir
@ 2013-09-25 16:55 ` Sergio Durigan Junior
2013-09-25 22:55 ` Philippe Waroquiers
0 siblings, 1 reply; 30+ messages in thread
From: Sergio Durigan Junior @ 2013-09-25 16:55 UTC (permalink / raw)
To: Agovic, Sanimir; +Cc: 'Philippe Waroquiers', gdb-patches
On Tuesday, September 24 2013, Sanimir Agovic wrote:
>> > Is it sufficient to assign sysno/sysret to 0 to indicate missing 'catch syscall'
>> > functionality? Both values seem legal to me.
>> The idea is that will be used in case the user forces the use of the
>> QCatchSyscalls packet. So, I think it is better to return "valid" (but
>> not used) values : I do not think there is a syscall nr 0.
>>
> gdb -batch -ex 'set architecture i386:x86-64' -ex 'catch syscall 0'
> The target architecture is assumed to be i386:x86-64
> Catchpoint 1 (syscall 'read' [0])
>
> I just wanted to make sure that we do not report any false syscalls.
> Thus I checked syscall 0 in gdb on amd64 which is mapped to 'read'.
Thanks for catching that. I guess I had to solve the same problem for
the native "catch syscall". If you look at the UNKNOWN_SYSCALL define
(it's on gdbarch.h), you'll see that I have chosen the value of -1. I
guess you could use the same trick, Philippe. WDYT?
--
Sergio
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-25 16:55 ` Sergio Durigan Junior
@ 2013-09-25 22:55 ` Philippe Waroquiers
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Waroquiers @ 2013-09-25 22:55 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Agovic, Sanimir, gdb-patches
On Wed, 2013-09-25 at 13:55 -0300, Sergio Durigan Junior wrote:
> On Tuesday, September 24 2013, Sanimir Agovic wrote:
>
> >> > Is it sufficient to assign sysno/sysret to 0 to indicate missing 'catch syscall'
> >> > functionality? Both values seem legal to me.
> >> The idea is that will be used in case the user forces the use of the
> >> QCatchSyscalls packet. So, I think it is better to return "valid" (but
> >> not used) values : I do not think there is a syscall nr 0.
> >>
> > gdb -batch -ex 'set architecture i386:x86-64' -ex 'catch syscall 0'
> > The target architecture is assumed to be i386:x86-64
> > Catchpoint 1 (syscall 'read' [0])
> >
> > I just wanted to make sure that we do not report any false syscalls.
> > Thus I checked syscall 0 in gdb on amd64 which is mapped to 'read'.
>
> Thanks for catching that. I guess I had to solve the same problem for
> the native "catch syscall". If you look at the UNKNOWN_SYSCALL define
> (it's on gdbarch.h), you'll see that I have chosen the value of -1. I
> guess you could use the same trick, Philippe. WDYT?
I tested with the 0 value, and effectively that gives a "read" syscall
on x86-64, which is not ideal, even if that would happen only when
the user does something strange (forcing to activate a packet with
a gdbserver which has not reported it can handle such packet).
-1 looks a good "bad" value, assuming that the gdb+protocol+gdbserver
will be able to "correctly" handle negative numbers.
I will test this behaviour (probably this WE).
Also, ping ... :
if before this WE, a maintainer reviews the patch, I will be more
than happy to handle the comments :)
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
* [COMMIT PATCH] remote.c: Remove unnecessary fields from 'struct stop_reply'. (was: Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
2013-09-21 21:03 ` Eli Zaretskii
2013-09-23 11:51 ` Agovic, Sanimir
@ 2013-09-27 13:25 ` Pedro Alves
2013-09-27 19:30 ` RFA [PATCH v3] Implement 'catch syscall' for gdbserver Pedro Alves
2013-09-27 20:55 ` Sergio Durigan Junior
4 siblings, 0 replies; 30+ messages in thread
From: Pedro Alves @ 2013-09-27 13:25 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On 09/21/2013 09:55 PM, Philippe Waroquiers wrote:
> @@ -5284,6 +5377,11 @@ typedef struct stop_reply
> int stopped_by_watchpoint_p;
> CORE_ADDR watch_data_address;
>
> + /* Set to 1 if the stop_reply is for a syscall entry or
> + return. The field ws.value.syscall_number then identifies
> + the syscall. */
> + int syscall;
> +
> int solibs_changed;
> int replay_event;
While reviewing this patch, I wondered why we actually need this
field. Turns out we don't. I've applied the patch below.
----------
Subject: [PATCH] remote.c: Remove unnecessary fields from 'struct
stop_reply'.
I noticed these fields aren't really necessary -- if the T stop reply
indicated any we have any special event, the fallthrough doesn't
really do anything.
Tested on x86_64 Fedora 17 w/ local gdbserver, and also confirmed
"catch load" against a Windows gdbserver running under Wine, which
exercises TARGET_WAITKIND_LOADED, still works as expected.
gdb/
2013-09-27 Pedro Alves <palves@redhat.com>
* remote.c (struct stop_reply) <solibs_changed, replay_event>:
Delete fields.
(remote_parse_stop_reply): Adjust, setting event->ws.kind
directly.
---
gdb/remote.c | 31 +++++++++++--------------------
1 file changed, 11 insertions(+), 20 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 2e116d9..0fa1e2b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5284,9 +5284,6 @@ typedef struct stop_reply
int stopped_by_watchpoint_p;
CORE_ADDR watch_data_address;
- int solibs_changed;
- int replay_event;
-
int core;
} *stop_reply_p;
@@ -5546,8 +5543,6 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
event->ptid = null_ptid;
event->ws.kind = TARGET_WAITKIND_IGNORE;
event->ws.value.integer = 0;
- event->solibs_changed = 0;
- event->replay_event = 0;
event->stopped_by_watchpoint_p = 0;
event->regcache = NULL;
event->core = -1;
@@ -5611,15 +5606,14 @@ Packet: '%s'\n"),
while (*p_temp && *p_temp != ';')
p_temp++;
- event->solibs_changed = 1;
+ event->ws.kind = TARGET_WAITKIND_LOADED;
p = p_temp;
}
else if (strncmp (p, "replaylog", p1 - p) == 0)
{
- /* NO_HISTORY event.
- p1 will indicate "begin" or "end", but
- it makes no difference for now, so ignore it. */
- event->replay_event = 1;
+ event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
+ /* p1 will indicate "begin" or "end", but it makes
+ no difference for now, so ignore it. */
p_temp = strchr (p1 + 1, ';');
if (p_temp)
p = p_temp;
@@ -5675,18 +5669,15 @@ Packet: '%s'\n"),
buf, p);
++p;
}
+
+ if (event->ws.kind != TARGET_WAITKIND_IGNORE)
+ break;
+
/* fall through */
case 'S': /* Old style status, just signal only. */
- if (event->solibs_changed)
- event->ws.kind = TARGET_WAITKIND_LOADED;
- else if (event->replay_event)
- event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
- else
- {
- event->ws.kind = TARGET_WAITKIND_STOPPED;
- event->ws.value.sig = (enum gdb_signal)
- (((fromhex (buf[1])) << 4) + (fromhex (buf[2])));
- }
+ event->ws.kind = TARGET_WAITKIND_STOPPED;
+ event->ws.value.sig = (enum gdb_signal)
+ (((fromhex (buf[1])) << 4) + (fromhex (buf[2])));
break;
case 'W': /* Target exited. */
case 'X':
--
1.7.11.7
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
` (2 preceding siblings ...)
2013-09-27 13:25 ` [COMMIT PATCH] remote.c: Remove unnecessary fields from 'struct stop_reply'. (was: Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
@ 2013-09-27 19:30 ` Pedro Alves
2013-09-27 20:13 ` Philippe Waroquiers
2013-09-27 20:55 ` Sergio Durigan Junior
4 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2013-09-27 19:30 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On 09/21/2013 09:55 PM, Philippe Waroquiers wrote:
> No regression on linux/x86-64, native and gdbserver mode.
> Manually tested with a (patched) Valgrind gdbserver.
I guess you've been testing with the native-extended-gdbserver.exp board file?
I needed these patches to make catch-syscall.exp pass with native-gdbserver.exp
("target remote"):
https://sourceware.org/ml/gdb-patches/2013-09/msg00968.html
(I'm still playing with and reviewing the code itself.)
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-27 19:30 ` RFA [PATCH v3] Implement 'catch syscall' for gdbserver Pedro Alves
@ 2013-09-27 20:13 ` Philippe Waroquiers
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Waroquiers @ 2013-09-27 20:13 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Fri, 2013-09-27 at 20:30 +0100, Pedro Alves wrote:
> On 09/21/2013 09:55 PM, Philippe Waroquiers wrote:
>
> > No regression on linux/x86-64, native and gdbserver mode.
> > Manually tested with a (patched) Valgrind gdbserver.
>
> I guess you've been testing with the native-extended-gdbserver.exp board file?
> I needed these patches to make catch-syscall.exp pass with native-gdbserver.exp
> ("target remote"):
>
> https://sourceware.org/ml/gdb-patches/2013-09/msg00968.html
>
> (I'm still playing with and reviewing the code itself.)
>
As discussed on IRC, I did the tests with native-gdbserver.exp
but got no failure, as my gdb is compiled without libexpat,
and the failures are only triggered where there is xml support.
Philippe (that will install libexpat one of these days ...)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
` (3 preceding siblings ...)
2013-09-27 19:30 ` RFA [PATCH v3] Implement 'catch syscall' for gdbserver Pedro Alves
@ 2013-09-27 20:55 ` Sergio Durigan Junior
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
4 siblings, 1 reply; 30+ messages in thread
From: Sergio Durigan Junior @ 2013-09-27 20:55 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On Saturday, September 21 2013, Philippe Waroquiers wrote:
> This is version 3 of the patch implementing catch syscall for gdbserver.
> This integrates the review comments given by Eli Zaretskii
> and Sergio Durigan Junior.
Thanks!
> Difference with version 2 is doc and code cleanup, renamed various
> variables and functions, added some clarification comments,
> avoid use of catched. No functional change.
Still have some formatting issues, as pointed by Tom on IRC.
> No regression on linux/x86-64, native and gdbserver mode.
> Manually tested with a (patched) Valgrind gdbserver.
>
> Ok to commit ?
>
> Thanks
>
> Philippe
>
> 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.
> * 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.
> (Stop Reply Packets): Document new stop reasons syscall_entry and
> syscall_return.
> (Async Records): Fixed syscall-return item name.
>
> 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): Enable, detect and handle
> SYSCALL_SIGTRAP.
> (gdb_catch_this_syscall_p): New function.
> (get_syscall_trapinfo): New function.
> (linux_supports_catch_syscall): New function.
> (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 catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
> * server.c: Define catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
> (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.
>
>
>
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.614
> diff -u -p -r1.614 NEWS
> --- NEWS 16 Sep 2013 17:47:29 -0000 1.614
> +++ NEWS 21 Sep 2013 20:38:09 -0000
> @@ -139,11 +139,20 @@ 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.
> +
> +
> * 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. Currently 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 21 Sep 2013 20:38:10 -0000
> @@ -1340,6 +1340,7 @@ enum {
> PACKET_qSupported,
> PACKET_qTStatus,
> PACKET_QPassSignals,
> + PACKET_QCatchSyscalls,
> PACKET_QProgramSignals,
> PACKET_qSearch_memory,
> PACKET_vAttach,
> @@ -1728,6 +1729,96 @@ remote_pass_signals (int numsigs, unsign
> }
> }
>
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> + to report syscalls to GDB. */
> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> + int table_size, int *table)
> +{
> + char *catch_packet, *p;
> + enum packet_result result;
> + int n_sysno = 0;
> +
> + if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
> + return 1; /* not supported */
> +
There is excessive whitespaces in the line after "return 1". Also, we
don't put comments in the same line. It would be better to write:
if (condition)
{
/* Not supported. */
return 1;
}
> + if (needed && !any_count)
> + {
> + int i;
> +
> + /* Count how many syscalls are to be caught (table[sysno] != 0). */
> + for (i = 0; i < table_size; i++)
> + if (table[i])
> + 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. */
This comment could be expanded to use more characters in each line.
> + const char *q1 = "QCatchSyscalls:1";
> + int i;
> + const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
> +
> + catch_packet = xmalloc (maxpktsz);
> + strcpy (catch_packet, q1);
> + if (!any_count)
> + {
> + char *p;
> +
> + p = catch_packet;
> + p += strlen(p);
Space between function name and paren.
> +
> + /* Add in catch_packet each syscall to be caught (table[i] != 0). */
> + for (i = 0; i < table_size; i++)
> + {
> + if (table[i])
> + {
> + xsnprintf(p, catch_packet + maxpktsz - p,
Likewise.
> + ";%x", i);
> + p += strlen(p);
Likewise.
> + }
> + }
> + }
> + if (strlen(catch_packet) > get_remote_packet_size())
Likewise.
> + {
> + /* catch_packet too big. Fallback to less efficient
> + non selective mode, with GDB doing the filtering. */
> + catch_packet[strlen (q1)] = 0;
> + }
> + }
> + 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;
> + }
> +}
> +
> +
> +
> /* If 'QProgramSignals' is supported, tell the remote stub what
> signals it should pass through to the inferior when detaching. */
>
> @@ -4016,6 +4107,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,
> @@ -5284,6 +5377,11 @@ typedef struct stop_reply
> int stopped_by_watchpoint_p;
> CORE_ADDR watch_data_address;
>
> + /* Set to 1 if the stop_reply is for a syscall entry or
> + return. The field ws.value.syscall_number then identifies
> + the syscall. */
> + int syscall;
> +
This one can be removed now with Pedro's patch.
> int solibs_changed;
> int replay_event;
>
> @@ -5546,6 +5644,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;
Likewise.
> event->solibs_changed = 0;
> event->replay_event = 0;
> event->stopped_by_watchpoint_p = 0;
> @@ -5596,6 +5695,24 @@ 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;
> +
> + 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;
> +
> + 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 +5798,9 @@ 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);
> else
> {
> event->ws.kind = TARGET_WAITKIND_STOPPED;
> @@ -11452,6 +11572,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 +12067,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 21 Sep 2013 20:38:10 -0000
> @@ -361,16 +361,18 @@ 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.
> + We keep PTRACE_O_TRACEFORK option activated as a fork
> + event notification is expected after my_waitpid below. */
> 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));
> 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 +476,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 ();
> +
> 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.1112
> diff -u -p -r1.1112 gdb.texinfo
> --- doc/gdb.texinfo 16 Sep 2013 18:00:34 -0000 1.1112
> +++ doc/gdb.texinfo 21 Sep 2013 20:38:14 -0000
> @@ -18752,6 +18752,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}}
> @@ -38147,6 +38151,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.
> @@ -38517,6 +38526,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 syscall is not caught. 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
> @@ -38880,6 +38927,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{-}
> @@ -39040,6 +39092,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.248
> diff -u -p -r1.248 linux-low.c
> --- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
> +++ gdbserver/linux-low.c 21 Sep 2013 20:38:15 -0000
> @@ -74,6 +74,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
> @@ -481,6 +486,36 @@ get_pc (struct lwp_info *lwp)
> return pc;
> }
>
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
> + Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
> + return code. */
> +
> +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;
> + return;
> + }
Just a reminder that you were going to check if -1 is a better value to
represent this case. And if it is, then it should be documented, of course.
> +
> + 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.
>
> @@ -2248,6 +2283,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_catch_this_syscall_p (struct lwp_info *event_child)
> +{
> + int i;
> + int sysno, sysret;
> +
> + if (!catching_syscalls_p)
> + return 0;
> +
> + if (syscalls_to_catch_size == 0)
> + return 1;
> +
> + get_syscall_trapinfo (event_child, &sysno, &sysret);
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + if (syscalls_to_catch[i] == sysno)
> + return 1;
> +
> + return 0;
> +}
> +
> /* Wait for process, returns status. */
>
> static ptid_t
> @@ -2529,6 +2587,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
> + && !gdb_catch_this_syscall_p (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
> @@ -2707,7 +2778,24 @@ 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);
> + /* Differentiate entry from return using the return code
> + -ENOSYS. This is not ideal as a syscall return from a not
> + implemented syscall will be seen as an entry. However, this
> + resists well to enabling/disabling catch syscall at various
> + moment. A better way to differentiate entry/return in GDB
> + and GDBSERVER would be nice. */
> + if (sysret == -ENOSYS)
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + else
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_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,
> @@ -3269,7 +3357,10 @@ 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
> + : catching_syscalls_p ? PTRACE_SYSCALL
> + : PTRACE_CONT,
> + lwpid_of (lwp),
Just out of curiosity, have you thought about what I said in the other
patch, i.e., rewriting this piece to avoid the "double-ternary"?
> (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. */
> @@ -5108,6 +5199,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());
> +}
> +
> +static int
> linux_supports_tracepoints (void)
> {
> if (*the_low_target.supports_tracepoints == NULL)
> @@ -5798,6 +5896,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 21 Sep 2013 20:38:15 -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
> + 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.51
> diff -u -p -r1.51 linux-x86-low.c
> --- gdbserver/linux-x86-low.c 5 Sep 2013 20:40:58 -0000 1.51
> +++ gdbserver/linux-x86-low.c 21 Sep 2013 20:38:15 -0000
> @@ -1474,6 +1474,33 @@ x86_arch_setup (void)
> current_process ()->tdesc = x86_linux_read_description ();
> }
>
> +static void
> +x86_get_syscall_trapinfo (struct regcache *regcache, int *sysno, int *sysret)
Oh, I forgot in the other review, but this function needs a comment.
> +{
> + int use_64bit = register_size (regcache->tdesc, 0) == 8;
> +
> + if (use_64bit)
> + {
> + long l_sysno;
> + long l_sysret;
> +
> + 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;
> +
> + 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;
> + }
> +}
> +
> static int
> x86_supports_tracepoints (void)
> {
> @@ -3323,6 +3350,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.99
> diff -u -p -r1.99 remote-utils.c
> --- gdbserver/remote-utils.c 5 Sep 2013 20:41:55 -0000 1.99
> +++ gdbserver/remote-utils.c 21 Sep 2013 20:38:15 -0000
> @@ -1321,12 +1321,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);
> buf += strlen (buf);
>
> saved_inferior = current_inferior;
> @@ -1337,6 +1342,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.201
> diff -u -p -r1.201 server.c
> --- gdbserver/server.c 18 Sep 2013 01:59:59 -0000 1.201
> +++ gdbserver/server.c 21 Sep 2013 20:38:15 -0000
> @@ -74,6 +74,9 @@ int debug_threads;
> int debug_hw_points;
>
> int pass_signals[GDB_SIGNAL_LAST];
> +int catching_syscalls_p;
> +int syscalls_to_catch_size;
> +int *syscalls_to_catch;
> int program_signals[GDB_SIGNAL_LAST];
> int program_signals_p;
>
> @@ -511,6 +514,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;
> +
> + catching_syscalls_p = 1;
> + if (syscalls_to_catch != NULL)
> + {
> + xfree (syscalls_to_catch);
> + syscalls_to_catch = NULL;
> + }
> + syscalls_to_catch_size = 0;
> + p = own_buf + strlen("QCatchSyscalls:1");
> + while (*p)
> + {
> + if (*p++ == ';')
> + syscalls_to_catch_size++;
> + }
> + if (syscalls_to_catch_size > 0)
> + {
> + syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
> + p = strchr(own_buf, ';') + 1;
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + {
> + p = decode_address_to_semicolon (&sysno, p);
> + syscalls_to_catch[i] = (int) sysno;
> + }
> + }
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> + if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> + {
> + catching_syscalls_p = 0;
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
> {
> int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
> "PacketSize=%x;QPassSignals+;QProgramSignals+",
> PBUFSIZ - 1);
>
> + if (target_supports_catch_syscall ())
> + 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.118
> diff -u -p -r1.118 server.h
> --- gdbserver/server.h 5 Sep 2013 20:45:39 -0000 1.118
> +++ gdbserver/server.h 21 Sep 2013 20:38:15 -0000
> @@ -115,6 +115,15 @@ extern int server_waiting;
> extern int debug_threads;
> extern int debug_hw_points;
> extern int pass_signals[];
> +
> +/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise. */
> +extern int catching_syscalls_p;
> +/* syscalls_to_catch is the list of syscalls to report to GDB.
> + If catching_syscalls_p and syscalls_to_catch == NULL, it means
> + all syscalls must be reported. */
> +extern int syscalls_to_catch_size;
> +extern int *syscalls_to_catch;
> +
> 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.71
> diff -u -p -r1.71 target.h
> --- gdbserver/target.h 5 Sep 2013 20:41:22 -0000 1.71
> +++ gdbserver/target.h 21 Sep 2013 20:38:15 -0000
> @@ -268,6 +268,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);
> @@ -414,6 +418,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.69
> diff -u -p -r1.69 win32-low.c
> --- gdbserver/win32-low.c 5 Sep 2013 20:45:39 -0000 1.69
> +++ gdbserver/win32-low.c 21 Sep 2013 20:38:16 -0000
> @@ -1824,6 +1824,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 21 Sep 2013 20:38:16 -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
> +}
> +
> # This shall be updated whenever 'catch syscall' is implemented
> # on some architecture.
> #if { ![istarget "i\[34567\]86-*-linux*"]
--
Sergio
^ permalink raw reply [flat|nested] 30+ messages in thread
* RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-09-27 20:55 ` Sergio Durigan Junior
@ 2013-09-29 15:04 ` Philippe Waroquiers
2013-10-01 5:16 ` Sergio Durigan Junior
` (3 more replies)
0 siblings, 4 replies; 30+ messages in thread
From: Philippe Waroquiers @ 2013-09-29 15:04 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: gdb-patches
On Fri, 2013-09-27 at 17:55 -0300, Sergio Durigan Junior wrote:
> Still have some formatting issues, as pointed by Tom on IRC.
Thanks, handled all comments (formatting and others),
just a question below about the -1 syscall nr.
> > + if (the_low_target.get_syscall_trapinfo == NULL)
> > + {
> > + *sysno = 0;
> > + *sysret = 0;
> > + return;
> > + }
>
> Just a reminder that you were going to check if -1 is a better value to
> represent this case. And if it is, then it should be documented, of course.
Tested with -1 (on x86_64), and it works.
I would have liked to re-use UNKNOWN_SYSCALL defined in gdbarch.h, but
this is not included in gdbserver, and it sounds strange to me to
include it to just get this constant.
So, for the moment, I have just used -1 directly.
Is there a correct place to define this constant ?
Also, not sure what is meant by 'should be documented'.
Do you mean add a sentence in the protocol documentation such as:
"In case the remote stub cannot determine the syscall number,
the remote stub must send -1 as syscall nr (encoded in hex,
e.g. ffffffff)."
(do we really want that to be part of the protocol spec ?
This is really a very special case caused by a user "strange"
manipulation of forcing to use the packet QCatchSyscalls).
In the meantime, find below the version 4 of the patch implementing
catch syscall for gdbserver.
The changes compared to v3 are:
* handles the additional review comments given by Sergio Durigan Junior
In particular, in case the syscall nr cannot be fetched from the low
linux target, returns -1 syscall nr.
* removes the useless "syscall" field in the stop_reply (i.e. similar
to the removal of fields solibs_changes/replay_event done by Pedro Alves).
* formatting changes (I hope I got the formatting all correct this time).
Tested on x86_64, using:
for t in "" "--target_board native-extended-gdbserver" "--target_board native-gdbserver"
do
make check RUNTESTFLAGS="$t"
....
done
(still without libexpat, waiting for Pedro's patch to be committed).
Thanks ...
Philippe
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.
* 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.
(Stop Reply Packets): Document new stop reasons syscall_entry and
syscall_return.
(Async Records): Fixed syscall-return item name.
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): Enable, detect and handle
SYSCALL_SIGTRAP.
(gdb_catch_this_syscall_p): New function.
(get_syscall_trapinfo): New function.
(linux_supports_catch_syscall): New function.
(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 catching_syscalls_p, syscalls_to_catch_size and
syscalls_to_catch.
* server.c: Define catching_syscalls_p, syscalls_to_catch_size and
syscalls_to_catch.
(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.
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.616
diff -u -p -r1.616 NEWS
--- NEWS 25 Sep 2013 23:17:11 -0000 1.616
+++ NEWS 29 Sep 2013 14:20:27 -0000
@@ -157,11 +157,20 @@ 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.
+
+
* 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. Currently 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.579
diff -u -p -r1.579 remote.c
--- remote.c 27 Sep 2013 15:29:06 -0000 1.579
+++ remote.c 29 Sep 2013 14:20:29 -0000
@@ -1340,6 +1340,7 @@ enum {
PACKET_qSupported,
PACKET_qTStatus,
PACKET_QPassSignals,
+ PACKET_QCatchSyscalls,
PACKET_QProgramSignals,
PACKET_qSearch_memory,
PACKET_vAttach,
@@ -1728,6 +1729,98 @@ remote_pass_signals (int numsigs, unsign
}
}
+/* If 'QCatchSyscalls' is supported, tell the remote stub
+ to report syscalls to GDB. */
+
+static int
+remote_set_syscall_catchpoint (int pid, int needed, int any_count,
+ int table_size, int *table)
+{
+ char *catch_packet, *p;
+ enum packet_result result;
+ int n_sysno = 0;
+
+ if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
+ {
+ /* Not supported. */
+ return 1;
+ }
+
+ if (needed && !any_count)
+ {
+ int i;
+
+ /* Count how many syscalls are to be caught (table[sysno] != 0). */
+ for (i = 0; i < table_size; i++)
+ if (table[i])
+ 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";
+ int i;
+ const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
+
+ catch_packet = xmalloc (maxpktsz);
+ strcpy (catch_packet, q1);
+ if (!any_count)
+ {
+ char *p;
+
+ p = catch_packet;
+ p += strlen (p);
+
+ /* Add in catch_packet each syscall to be caught (table[i] != 0). */
+ for (i = 0; i < table_size; i++)
+ {
+ if (table[i])
+ {
+ 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;
+ }
+ }
+ 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;
+ }
+}
+
+
+
/* If 'QProgramSignals' is supported, tell the remote stub what
signals it should pass through to the inferior when detaching. */
@@ -4016,6 +4109,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,
@@ -5591,6 +5686,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;
+
+ event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
+ p = unpack_varlen_hex (++p1, &sysno);
+ event->ws.value.syscall_number = (int) sysno;
+ }
+ else if (strncmp (p, "syscall_return", p1 - p) == 0)
+ {
+ ULONGEST sysno;
+
+ event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
+ p = unpack_varlen_hex (++p1, &sysno);
+ 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))
@@ -11445,6 +11556,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;
@@ -11939,6 +12051,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 29 Sep 2013 14:20:29 -0000
@@ -361,16 +361,18 @@ 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.
+ We keep PTRACE_O_TRACEFORK option activated as a fork
+ event notification is expected after my_waitpid below. */
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));
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 +476,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 ();
+
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.1113
diff -u -p -r1.1113 gdb.texinfo
--- doc/gdb.texinfo 25 Sep 2013 23:17:12 -0000 1.1113
+++ doc/gdb.texinfo 29 Sep 2013 14:20:34 -0000
@@ -18752,6 +18752,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}}
@@ -38153,6 +38157,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.
@@ -38523,6 +38532,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 syscall is not caught. 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
@@ -38886,6 +38933,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{-}
@@ -39046,6 +39098,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.248
diff -u -p -r1.248 linux-low.c
--- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
+++ gdbserver/linux-low.c 29 Sep 2013 14:20:35 -0000
@@ -74,6 +74,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
@@ -481,6 +486,38 @@ get_pc (struct lwp_info *lwp)
return pc;
}
+/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
+ Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
+ return code. */
+
+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)
+ {
+ /* If we cannot get the syscall trapinfo, report an unknown
+ system call using -1 values. */
+ *sysno = -1;
+ *sysret = -1;
+ return;
+ }
+
+ 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.
@@ -2248,6 +2285,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_catch_this_syscall_p (struct lwp_info *event_child)
+{
+ int i;
+ int sysno, sysret;
+
+ if (!catching_syscalls_p)
+ return 0;
+
+ if (syscalls_to_catch_size == 0)
+ return 1;
+
+ get_syscall_trapinfo (event_child, &sysno, &sysret);
+ for (i = 0; i < syscalls_to_catch_size; i++)
+ if (syscalls_to_catch[i] == sysno)
+ return 1;
+
+ return 0;
+}
+
/* Wait for process, returns status. */
static ptid_t
@@ -2529,6 +2589,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
+ && !gdb_catch_this_syscall_p (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
@@ -2707,7 +2780,24 @@ 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);
+ /* Differentiate entry from return using the return code
+ -ENOSYS. This is not ideal as a syscall return from a not
+ implemented syscall will be seen as an entry. However, this
+ resists well to enabling/disabling catch syscall at various
+ moment. A better way to differentiate entry/return in GDB
+ and GDBSERVER would be nice. */
+ if (sysret == -ENOSYS)
+ ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
+ else
+ ourstatus->kind = TARGET_WAITKIND_SYSCALL_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,
@@ -3098,6 +3188,7 @@ linux_resume_one_lwp (struct lwp_info *l
{
struct thread_info *saved_inferior;
int fast_tp_collecting;
+ int ptrace_request;
if (lwp->stopped == 0)
return;
@@ -3269,7 +3360,14 @@ 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),
+ if (step)
+ ptrace_request = PTRACE_SINGLESTEP;
+ else if (catching_syscalls_p)
+ ptrace_request = PTRACE_SYSCALL;
+ else
+ ptrace_request = PTRACE_CONT;
+ ptrace (ptrace_request,
+ lwpid_of (lwp),
(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. */
@@ -5108,6 +5206,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());
+}
+
+static int
linux_supports_tracepoints (void)
{
if (*the_low_target.supports_tracepoints == NULL)
@@ -5798,6 +5903,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 29 Sep 2013 14:20:35 -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
+ 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.51
diff -u -p -r1.51 linux-x86-low.c
--- gdbserver/linux-x86-low.c 5 Sep 2013 20:40:58 -0000 1.51
+++ gdbserver/linux-x86-low.c 29 Sep 2013 14:20:35 -0000
@@ -1474,6 +1474,36 @@ x86_arch_setup (void)
current_process ()->tdesc = x86_linux_read_description ();
}
+/* Fill *SYSNO and *SYSRET with the syscall nr trapped and the syscall return
+ code. This should only be called if LWP got a SIGTRAP_SYSCALL. */
+
+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;
+
+ 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;
+
+ 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;
+ }
+}
+
static int
x86_supports_tracepoints (void)
{
@@ -3323,6 +3353,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.99
diff -u -p -r1.99 remote-utils.c
--- gdbserver/remote-utils.c 5 Sep 2013 20:41:55 -0000 1.99
+++ gdbserver/remote-utils.c 29 Sep 2013 14:20:35 -0000
@@ -1321,12 +1321,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);
buf += strlen (buf);
saved_inferior = current_inferior;
@@ -1337,6 +1342,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.201
diff -u -p -r1.201 server.c
--- gdbserver/server.c 18 Sep 2013 01:59:59 -0000 1.201
+++ gdbserver/server.c 29 Sep 2013 14:20:35 -0000
@@ -74,6 +74,9 @@ int debug_threads;
int debug_hw_points;
int pass_signals[GDB_SIGNAL_LAST];
+int catching_syscalls_p;
+int syscalls_to_catch_size;
+int *syscalls_to_catch;
int program_signals[GDB_SIGNAL_LAST];
int program_signals_p;
@@ -511,6 +514,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;
+
+ catching_syscalls_p = 1;
+ if (syscalls_to_catch != NULL)
+ {
+ xfree (syscalls_to_catch);
+ syscalls_to_catch = NULL;
+ }
+ syscalls_to_catch_size = 0;
+ p = own_buf + strlen("QCatchSyscalls:1");
+ while (*p)
+ {
+ if (*p++ == ';')
+ syscalls_to_catch_size++;
+ }
+ if (syscalls_to_catch_size > 0)
+ {
+ syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
+ p = strchr(own_buf, ';') + 1;
+ for (i = 0; i < syscalls_to_catch_size; i++)
+ {
+ p = decode_address_to_semicolon (&sysno, p);
+ syscalls_to_catch[i] = (int) sysno;
+ }
+ }
+ strcpy (own_buf, "OK");
+ return;
+ }
+
+ if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
+ {
+ catching_syscalls_p = 0;
+ strcpy (own_buf, "OK");
+ return;
+ }
+
if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
{
int numsigs = (int) GDB_SIGNAL_LAST, i;
@@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
"PacketSize=%x;QPassSignals+;QProgramSignals+",
PBUFSIZ - 1);
+ if (target_supports_catch_syscall ())
+ 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.118
diff -u -p -r1.118 server.h
--- gdbserver/server.h 5 Sep 2013 20:45:39 -0000 1.118
+++ gdbserver/server.h 29 Sep 2013 14:20:35 -0000
@@ -115,6 +115,15 @@ extern int server_waiting;
extern int debug_threads;
extern int debug_hw_points;
extern int pass_signals[];
+
+/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise. */
+extern int catching_syscalls_p;
+/* syscalls_to_catch is the list of syscalls to report to GDB.
+ If catching_syscalls_p and syscalls_to_catch == NULL, it means
+ all syscalls must be reported. */
+extern int syscalls_to_catch_size;
+extern int *syscalls_to_catch;
+
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.71
diff -u -p -r1.71 target.h
--- gdbserver/target.h 5 Sep 2013 20:41:22 -0000 1.71
+++ gdbserver/target.h 29 Sep 2013 14:20:35 -0000
@@ -268,6 +268,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);
@@ -414,6 +418,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.69
diff -u -p -r1.69 win32-low.c
--- gdbserver/win32-low.c 5 Sep 2013 20:45:39 -0000 1.69
+++ gdbserver/win32-low.c 29 Sep 2013 14:20:35 -0000
@@ -1824,6 +1824,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 29 Sep 2013 14:20:36 -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
+}
+
# This shall be updated whenever 'catch syscall' is implemented
# on some architecture.
#if { ![istarget "i\[34567\]86-*-linux*"]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
@ 2013-10-01 5:16 ` Sergio Durigan Junior
2013-10-02 21:02 ` Sergio Durigan Junior
2013-10-02 19:41 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] " Pedro Alves
` (2 subsequent siblings)
3 siblings, 1 reply; 30+ messages in thread
From: Sergio Durigan Junior @ 2013-10-01 5:16 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On Sunday, September 29 2013, Philippe Waroquiers wrote:
> On Fri, 2013-09-27 at 17:55 -0300, Sergio Durigan Junior wrote:
>> Still have some formatting issues, as pointed by Tom on IRC.
> Thanks, handled all comments (formatting and others),
> just a question below about the -1 syscall nr.
Thanks.
>> > + if (the_low_target.get_syscall_trapinfo == NULL)
>> > + {
>> > + *sysno = 0;
>> > + *sysret = 0;
>> > + return;
>> > + }
>>
>> Just a reminder that you were going to check if -1 is a better value to
>> represent this case. And if it is, then it should be documented, of course.
> Tested with -1 (on x86_64), and it works.
Nice.
> I would have liked to re-use UNKNOWN_SYSCALL defined in gdbarch.h, but
> this is not included in gdbserver, and it sounds strange to me to
> include it to just get this constant.
> So, for the moment, I have just used -1 directly.
> Is there a correct place to define this constant ?
You're only using it in gdbserver/linux-low.c, so I'd just put it
there. No need to pollute the namespace, I think.
> Also, not sure what is meant by 'should be documented'.
> Do you mean add a sentence in the protocol documentation such as:
> "In case the remote stub cannot determine the syscall number,
> the remote stub must send -1 as syscall nr (encoded in hex,
> e.g. ffffffff)."
> (do we really want that to be part of the protocol spec ?
> This is really a very special case caused by a user "strange"
> manipulation of forcing to use the packet QCatchSyscalls).
I mean that it is good to add a comment above the #define explaining why
we need it, and why it needs to be -1, etc. I wasn't thinking about
putting anything in the docs because of the reasons you mentioned.
Maybe some maintainer has a different opinion, though.
> In the meantime, find below the version 4 of the patch implementing
> catch syscall for gdbserver.
> The changes compared to v3 are:
> * handles the additional review comments given by Sergio Durigan Junior
> In particular, in case the syscall nr cannot be fetched from the low
> linux target, returns -1 syscall nr.
> * removes the useless "syscall" field in the stop_reply (i.e. similar
> to the removal of fields solibs_changes/replay_event done by Pedro Alves).
> * formatting changes (I hope I got the formatting all correct this time).
>
> Tested on x86_64, using:
> for t in "" "--target_board native-extended-gdbserver" "--target_board native-gdbserver"
> do
> make check RUNTESTFLAGS="$t"
> ....
> done
> (still without libexpat, waiting for Pedro's patch to be committed).
OK, I didn't test it now, but I will tomorrow. If I find something
strange, I will write another message. For now, thanks a lot for the
patch!
> Thanks ...
>
> Philippe
>
>
> 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.
> * 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.
> (Stop Reply Packets): Document new stop reasons syscall_entry and
> syscall_return.
> (Async Records): Fixed syscall-return item name.
>
> 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): Enable, detect and handle
> SYSCALL_SIGTRAP.
> (gdb_catch_this_syscall_p): New function.
> (get_syscall_trapinfo): New function.
> (linux_supports_catch_syscall): New function.
> (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 catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
> * server.c: Define catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
> (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.
>
>
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.616
> diff -u -p -r1.616 NEWS
> --- NEWS 25 Sep 2013 23:17:11 -0000 1.616
> +++ NEWS 29 Sep 2013 14:20:27 -0000
> @@ -157,11 +157,20 @@ 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.
> +
> +
> * 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. Currently 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.579
> diff -u -p -r1.579 remote.c
> --- remote.c 27 Sep 2013 15:29:06 -0000 1.579
> +++ remote.c 29 Sep 2013 14:20:29 -0000
> @@ -1340,6 +1340,7 @@ enum {
> PACKET_qSupported,
> PACKET_qTStatus,
> PACKET_QPassSignals,
> + PACKET_QCatchSyscalls,
> PACKET_QProgramSignals,
> PACKET_qSearch_memory,
> PACKET_vAttach,
> @@ -1728,6 +1729,98 @@ remote_pass_signals (int numsigs, unsign
> }
> }
>
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> + to report syscalls to GDB. */
> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> + int table_size, int *table)
> +{
> + char *catch_packet, *p;
> + enum packet_result result;
> + int n_sysno = 0;
> +
> + if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
> + {
> + /* Not supported. */
> + return 1;
> + }
> +
> + if (needed && !any_count)
> + {
> + int i;
> +
> + /* Count how many syscalls are to be caught (table[sysno] != 0). */
> + for (i = 0; i < table_size; i++)
> + if (table[i])
> + 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";
> + int i;
> + const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
> +
> + catch_packet = xmalloc (maxpktsz);
> + strcpy (catch_packet, q1);
> + if (!any_count)
> + {
> + char *p;
> +
> + p = catch_packet;
> + p += strlen (p);
> +
> + /* Add in catch_packet each syscall to be caught (table[i] != 0). */
> + for (i = 0; i < table_size; i++)
> + {
> + if (table[i])
> + {
> + 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;
> + }
> + }
> + 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;
> + }
> +}
> +
> +
> +
> /* If 'QProgramSignals' is supported, tell the remote stub what
> signals it should pass through to the inferior when detaching. */
>
> @@ -4016,6 +4109,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,
> @@ -5591,6 +5686,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;
> +
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + p = unpack_varlen_hex (++p1, &sysno);
> + event->ws.value.syscall_number = (int) sysno;
> + }
> + else if (strncmp (p, "syscall_return", p1 - p) == 0)
> + {
> + ULONGEST sysno;
> +
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
> + p = unpack_varlen_hex (++p1, &sysno);
> + 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))
> @@ -11445,6 +11556,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;
> @@ -11939,6 +12051,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 29 Sep 2013 14:20:29 -0000
> @@ -361,16 +361,18 @@ 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.
> + We keep PTRACE_O_TRACEFORK option activated as a fork
> + event notification is expected after my_waitpid below. */
> 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));
> 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 +476,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 ();
> +
> 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.1113
> diff -u -p -r1.1113 gdb.texinfo
> --- doc/gdb.texinfo 25 Sep 2013 23:17:12 -0000 1.1113
> +++ doc/gdb.texinfo 29 Sep 2013 14:20:34 -0000
> @@ -18752,6 +18752,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}}
> @@ -38153,6 +38157,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.
> @@ -38523,6 +38532,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 syscall is not caught. 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
> @@ -38886,6 +38933,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{-}
> @@ -39046,6 +39098,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.248
> diff -u -p -r1.248 linux-low.c
> --- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
> +++ gdbserver/linux-low.c 29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,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
> @@ -481,6 +486,38 @@ get_pc (struct lwp_info *lwp)
> return pc;
> }
>
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
> + Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
> + return code. */
> +
> +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)
> + {
> + /* If we cannot get the syscall trapinfo, report an unknown
> + system call using -1 values. */
> + *sysno = -1;
> + *sysret = -1;
> + return;
> + }
> +
> + 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.
>
> @@ -2248,6 +2285,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_catch_this_syscall_p (struct lwp_info *event_child)
> +{
> + int i;
> + int sysno, sysret;
> +
> + if (!catching_syscalls_p)
> + return 0;
> +
> + if (syscalls_to_catch_size == 0)
> + return 1;
> +
> + get_syscall_trapinfo (event_child, &sysno, &sysret);
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + if (syscalls_to_catch[i] == sysno)
> + return 1;
> +
> + return 0;
> +}
> +
> /* Wait for process, returns status. */
>
> static ptid_t
> @@ -2529,6 +2589,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
> + && !gdb_catch_this_syscall_p (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
> @@ -2707,7 +2780,24 @@ 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);
> + /* Differentiate entry from return using the return code
> + -ENOSYS. This is not ideal as a syscall return from a not
> + implemented syscall will be seen as an entry. However, this
> + resists well to enabling/disabling catch syscall at various
> + moment. A better way to differentiate entry/return in GDB
> + and GDBSERVER would be nice. */
> + if (sysret == -ENOSYS)
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + else
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_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,
> @@ -3098,6 +3188,7 @@ linux_resume_one_lwp (struct lwp_info *l
> {
> struct thread_info *saved_inferior;
> int fast_tp_collecting;
> + int ptrace_request;
>
> if (lwp->stopped == 0)
> return;
> @@ -3269,7 +3360,14 @@ 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),
> + if (step)
> + ptrace_request = PTRACE_SINGLESTEP;
> + else if (catching_syscalls_p)
> + ptrace_request = PTRACE_SYSCALL;
> + else
> + ptrace_request = PTRACE_CONT;
> + ptrace (ptrace_request,
> + lwpid_of (lwp),
> (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. */
> @@ -5108,6 +5206,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());
> +}
> +
> +static int
> linux_supports_tracepoints (void)
> {
> if (*the_low_target.supports_tracepoints == NULL)
> @@ -5798,6 +5903,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 29 Sep 2013 14:20:35 -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
> + 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.51
> diff -u -p -r1.51 linux-x86-low.c
> --- gdbserver/linux-x86-low.c 5 Sep 2013 20:40:58 -0000 1.51
> +++ gdbserver/linux-x86-low.c 29 Sep 2013 14:20:35 -0000
> @@ -1474,6 +1474,36 @@ x86_arch_setup (void)
> current_process ()->tdesc = x86_linux_read_description ();
> }
>
> +/* Fill *SYSNO and *SYSRET with the syscall nr trapped and the syscall return
> + code. This should only be called if LWP got a SIGTRAP_SYSCALL. */
> +
> +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;
> +
> + 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;
> +
> + 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;
> + }
> +}
> +
> static int
> x86_supports_tracepoints (void)
> {
> @@ -3323,6 +3353,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.99
> diff -u -p -r1.99 remote-utils.c
> --- gdbserver/remote-utils.c 5 Sep 2013 20:41:55 -0000 1.99
> +++ gdbserver/remote-utils.c 29 Sep 2013 14:20:35 -0000
> @@ -1321,12 +1321,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);
> buf += strlen (buf);
>
> saved_inferior = current_inferior;
> @@ -1337,6 +1342,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.201
> diff -u -p -r1.201 server.c
> --- gdbserver/server.c 18 Sep 2013 01:59:59 -0000 1.201
> +++ gdbserver/server.c 29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,9 @@ int debug_threads;
> int debug_hw_points;
>
> int pass_signals[GDB_SIGNAL_LAST];
> +int catching_syscalls_p;
> +int syscalls_to_catch_size;
> +int *syscalls_to_catch;
> int program_signals[GDB_SIGNAL_LAST];
> int program_signals_p;
>
> @@ -511,6 +514,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;
> +
> + catching_syscalls_p = 1;
> + if (syscalls_to_catch != NULL)
> + {
> + xfree (syscalls_to_catch);
> + syscalls_to_catch = NULL;
> + }
> + syscalls_to_catch_size = 0;
> + p = own_buf + strlen("QCatchSyscalls:1");
> + while (*p)
> + {
> + if (*p++ == ';')
> + syscalls_to_catch_size++;
> + }
> + if (syscalls_to_catch_size > 0)
> + {
> + syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
> + p = strchr(own_buf, ';') + 1;
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + {
> + p = decode_address_to_semicolon (&sysno, p);
> + syscalls_to_catch[i] = (int) sysno;
> + }
> + }
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> + if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> + {
> + catching_syscalls_p = 0;
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
> {
> int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
> "PacketSize=%x;QPassSignals+;QProgramSignals+",
> PBUFSIZ - 1);
>
> + if (target_supports_catch_syscall ())
> + 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.118
> diff -u -p -r1.118 server.h
> --- gdbserver/server.h 5 Sep 2013 20:45:39 -0000 1.118
> +++ gdbserver/server.h 29 Sep 2013 14:20:35 -0000
> @@ -115,6 +115,15 @@ extern int server_waiting;
> extern int debug_threads;
> extern int debug_hw_points;
> extern int pass_signals[];
> +
> +/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise. */
> +extern int catching_syscalls_p;
> +/* syscalls_to_catch is the list of syscalls to report to GDB.
> + If catching_syscalls_p and syscalls_to_catch == NULL, it means
> + all syscalls must be reported. */
> +extern int syscalls_to_catch_size;
> +extern int *syscalls_to_catch;
> +
> 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.71
> diff -u -p -r1.71 target.h
> --- gdbserver/target.h 5 Sep 2013 20:41:22 -0000 1.71
> +++ gdbserver/target.h 29 Sep 2013 14:20:35 -0000
> @@ -268,6 +268,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);
> @@ -414,6 +418,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.69
> diff -u -p -r1.69 win32-low.c
> --- gdbserver/win32-low.c 5 Sep 2013 20:45:39 -0000 1.69
> +++ gdbserver/win32-low.c 29 Sep 2013 14:20:35 -0000
> @@ -1824,6 +1824,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 29 Sep 2013 14:20:36 -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
> +}
> +
> # This shall be updated whenever 'catch syscall' is implemented
> # on some architecture.
> #if { ![istarget "i\[34567\]86-*-linux*"]
--
Sergio
^ permalink raw reply [flat|nested] 30+ messages in thread
* Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver)
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
2013-10-01 5:16 ` Sergio Durigan Junior
@ 2013-10-02 19:41 ` Pedro Alves
2013-10-02 22:08 ` Philippe Waroquiers
2013-10-03 18:40 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
2013-10-04 4:22 ` Luis Machado
3 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2013-10-02 19:41 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Sergio Durigan Junior, gdb-patches
On 09/29/2013 04:04 PM, Philippe Waroquiers wrote:
> 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 29 Sep 2013 14:20:29 -0000
> @@ -361,16 +361,18 @@ 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.
> + We keep PTRACE_O_TRACEFORK option activated as a fork
> + event notification is expected after my_waitpid below. */
> 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));
This coupling looks unfortunate. Actually why wasn't this a
problem for the native target, even without GDBserver in the picture?
It just looks like a bug fix?
In any case, there's another related bug here. Above that we have:
/* First, set the PTRACE_O_TRACEFORK option. If this fails, we
know for sure that it is not supported. */
ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
if (ret != 0)
{
ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
{
warning (_("linux_check_ptrace_features: failed to kill child"));
return;
}
ret = my_waitpid (child_pid, &status, 0);
if (ret != child_pid)
warning (_("linux_check_ptrace_features: failed "
"to wait for killed child"));
else if (!WIFSIGNALED (status))
warning (_("linux_check_ptrace_features: unexpected "
"wait status 0x%x from killed child"), status);
return; <<<<<<<<<<<<<<<<<
}
Note that early return. If PTRACE_O_TRACEFORK isn't supported,
we're not checking PTRACE_O_TRACESYSGOOD. This didn't use to be
a problem before the unification of this whole detection business
in linux-ptrace.c. Before, the sysgood detection was
completely separate:
static void
linux_test_for_tracesysgood (int original_pid)
{
int ret;
sigset_t prev_mask;
/* We don't want those ptrace calls to be interrupted. */
block_child_signals (&prev_mask);
linux_supports_tracesysgood_flag = 0;
ret = ptrace (PTRACE_SETOPTIONS, original_pid, 0, PTRACE_O_TRACESYSGOOD);
if (ret != 0)
goto out;
linux_supports_tracesysgood_flag = 1;
out:
restore_child_signals_mask (&prev_mask);
}
Here's a patch that fixes these issues.
----
Subject: Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported.
If enabling PTRACE_O_TRACEFORK fails, we never test for
PTRACE_O_TRACESYSGOOD support. Before PTRACE_O_TRACESYSGOOD is checked,
we have:
/* First, set the PTRACE_O_TRACEFORK option. If this fails, we
know for sure that it is not supported. */
ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
if (ret != 0)
{
ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
{
warning (_("linux_check_ptrace_features: failed to kill child"));
return;
}
ret = my_waitpid (child_pid, &status, 0);
if (ret != child_pid)
warning (_("linux_check_ptrace_features: failed "
"to wait for killed child"));
else if (!WIFSIGNALED (status))
warning (_("linux_check_ptrace_features: unexpected "
"wait status 0x%x from killed child"), status);
return; <<<<<<<<<<<<<<<<<
}
Note that early return. If PTRACE_O_TRACEFORK isn't supported, we're
not checking PTRACE_O_TRACESYSGOOD. This didn't use to be a problem
before the unification of this whole detection business in
linux-ptrace.c. Before, the sysgood detection was completely
separate:
static void
linux_test_for_tracesysgood (int original_pid)
{
int ret;
sigset_t prev_mask;
/* We don't want those ptrace calls to be interrupted. */
block_child_signals (&prev_mask);
linux_supports_tracesysgood_flag = 0;
ret = ptrace (PTRACE_SETOPTIONS, original_pid, 0, PTRACE_O_TRACESYSGOOD);
if (ret != 0)
goto out;
linux_supports_tracesysgood_flag = 1;
out:
restore_child_signals_mask (&prev_mask);
}
So we need to get back the decoupling somehow. I think it's cleaner
to split the seperate feature detections to separate functions. This
patch does that. The new functions are named for their counterparts
that existed before this code was moved to linux-ptrace.c.
Note I've used forward declarations for the new functions to make the
patch clearer, as otherwise the patch would look like I'd be adding a
bunch of new code. A reorder can be done in a follow up patch.
Tested on x86_64 Fedora 17.
gdb/
2013-10-02 Pedro Alves <palves@redhat.com>
* common/linux-ptrace.c (linux_check_ptrace_features): Factor out
the PTRACE_O_TRACESYSGOOD and PTRACE_O_TRACEFORK to separate
functions. Always test for PTRACE_O_TRACESYSGOOD even if
PTRACE_O_TRACEFORK is not supported.
(linux_test_for_tracesysgood): New function.
(linux_test_for_tracefork): New function, factored out from
linux_check_ptrace_features, and also don't kill child_pid here.
---
gdb/common/linux-ptrace.c | 75 +++++++++++++++++++++++++--------------------
1 file changed, 42 insertions(+), 33 deletions(-)
diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 3a8e25e..3ea2d6d 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -308,13 +308,15 @@ linux_child_function (gdb_byte *child_stack)
_exit (0);
}
+static void linux_test_for_tracesysgood (int child_pid);
+static void linux_test_for_tracefork (int child_pid);
+
/* Determine ptrace features available on this target. */
static void
linux_check_ptrace_features (void)
{
int child_pid, ret, status;
- long second_pid;
/* Initialize the options. */
current_ptrace_options = 0;
@@ -335,42 +337,60 @@ linux_check_ptrace_features (void)
error (_("linux_check_ptrace_features: waitpid: unexpected status %d."),
status);
- /* First, set the PTRACE_O_TRACEFORK option. If this fails, we
- know for sure that it is not supported. */
- ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
+ linux_test_for_tracesysgood (child_pid);
- if (ret != 0)
+ linux_test_for_tracefork (child_pid);
+
+ /* Clean things up and kill any pending children. */
+ do
{
ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
- {
- warning (_("linux_check_ptrace_features: failed to kill child"));
- return;
- }
-
- ret = my_waitpid (child_pid, &status, 0);
- if (ret != child_pid)
- warning (_("linux_check_ptrace_features: failed "
- "to wait for killed child"));
- else if (!WIFSIGNALED (status))
- warning (_("linux_check_ptrace_features: unexpected "
- "wait status 0x%x from killed child"), status);
-
- return;
+ warning (_("linux_check_ptrace_features: failed to kill child"));
+ my_waitpid (child_pid, &status, 0);
}
+ while (WIFSTOPPED (status));
+}
+/* Determine if PTRACE_O_TRACESYSGOOD can be used to catch
+ syscalls. */
+
+static void
+linux_test_for_tracesysgood (int child_pid)
+{
#ifdef GDBSERVER
- /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
- PTRACE_O_TRACEVFORKDONE yet. */
+ /* gdbserver does not support PTRACE_O_TRACESYSGOOD. */
#else
+ int ret;
/* Check if the target supports PTRACE_O_TRACESYSGOOD. */
ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
if (ret == 0)
current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
+#endif
+}
+
+/* Determine if PTRACE_O_TRACEFORK can be used to follow fork
+ events. */
+static void
+linux_test_for_tracefork (int child_pid)
+{
+ int ret, status;
+ long second_pid;
+
+ /* First, set the PTRACE_O_TRACEFORK option. If this fails, we
+ know for sure that it is not supported. */
+ ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+ (PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
+
+ if (ret != 0)
+ return;
+
+#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
@@ -439,17 +459,6 @@ linux_check_ptrace_features (void)
else
warning (_("linux_check_ptrace_features: unexpected result from waitpid "
"(%d, status 0x%x)"), ret, status);
-
- /* Clean things up and kill any pending children. */
- do
- {
- ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) 0);
- if (ret != 0)
- warning (_("linux_check_ptrace_features: failed to kill child"));
- my_waitpid (child_pid, &status, 0);
- }
- while (WIFSTOPPED (status));
}
/* Enable reporting of all currently supported ptrace events. */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-01 5:16 ` Sergio Durigan Junior
@ 2013-10-02 21:02 ` Sergio Durigan Junior
0 siblings, 0 replies; 30+ messages in thread
From: Sergio Durigan Junior @ 2013-10-02 21:02 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: gdb-patches
On Tuesday, October 01 2013, I wrote:
>> 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.
>> * 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.
>> (Stop Reply Packets): Document new stop reasons syscall_entry and
>> syscall_return.
>> (Async Records): Fixed syscall-return item name.
>>
>> 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): Enable, detect and handle
>> SYSCALL_SIGTRAP.
>> (gdb_catch_this_syscall_p): New function.
>> (get_syscall_trapinfo): New function.
>> (linux_supports_catch_syscall): New function.
>> (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 catching_syscalls_p, syscalls_to_catch_size and
>> syscalls_to_catch.
>> * server.c: Define catching_syscalls_p, syscalls_to_catch_size and
>> syscalls_to_catch.
>> (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.
>>
BTW, I totally forgot about it, but you should take ownership of PR
remote/13585, mention it in the ChangeLog entries, and close it when the
patch is committed.
Thanks!
--
Sergio
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver)
2013-10-02 19:41 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] " Pedro Alves
@ 2013-10-02 22:08 ` Philippe Waroquiers
2013-10-03 10:16 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported Pedro Alves
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Waroquiers @ 2013-10-02 22:08 UTC (permalink / raw)
To: Pedro Alves; +Cc: Sergio Durigan Junior, gdb-patches
On Wed, 2013-10-02 at 20:41 +0100, Pedro Alves wrote:
> On 09/29/2013 04:04 PM, Philippe Waroquiers wrote:
>
> > 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 29 Sep 2013 14:20:29 -0000
> > @@ -361,16 +361,18 @@ 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.
> > + We keep PTRACE_O_TRACEFORK option activated as a fork
> > + event notification is expected after my_waitpid below. */
> > 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));
>
> This coupling looks unfortunate. Actually why wasn't this a
> problem for the native target, even without GDBserver in the picture?
> It just looks like a bug fix?
Yes I think there is already a similar coupling problem in the
native code:
if PTRACE_O_TRACESYSGOOD is supported but PTRACE_O_TRACEVFORKDONE
is not, then the PTRACE_O_TRACESYSGOOD verification has removed
the flag PTRACE_O_TRACEFORK and so the following my_waitpid will
fail.
The patch you give looks to solve all that.
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported.
2013-10-02 22:08 ` Philippe Waroquiers
@ 2013-10-03 10:16 ` Pedro Alves
0 siblings, 0 replies; 30+ messages in thread
From: Pedro Alves @ 2013-10-03 10:16 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Sergio Durigan Junior, gdb-patches
On 10/02/2013 11:08 PM, Philippe Waroquiers wrote:
>> This coupling looks unfortunate. Actually why wasn't this a
>> problem for the native target, even without GDBserver in the picture?
>> It just looks like a bug fix?
> Yes I think there is already a similar coupling problem in the
> native code:
> if PTRACE_O_TRACESYSGOOD is supported but PTRACE_O_TRACEVFORKDONE
> is not, then the PTRACE_O_TRACESYSGOOD verification has removed
> the flag PTRACE_O_TRACEFORK and so the following my_waitpid will
> fail.
>
> The patch you give looks to solve all that.
Great, I've checked it in now, with a minor update -- the function
name in the warning messages is updated.
-----
Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported.
If enabling PTRACE_O_TRACEFORK fails, we never test for
PTRACE_O_TRACESYSGOOD support. Before PTRACE_O_TRACESYSGOOD is checked,
we have:
/* First, set the PTRACE_O_TRACEFORK option. If this fails, we
know for sure that it is not supported. */
ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
if (ret != 0)
{
ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
{
warning (_("linux_check_ptrace_features: failed to kill child"));
return;
}
ret = my_waitpid (child_pid, &status, 0);
if (ret != child_pid)
warning (_("linux_check_ptrace_features: failed "
"to wait for killed child"));
else if (!WIFSIGNALED (status))
warning (_("linux_check_ptrace_features: unexpected "
"wait status 0x%x from killed child"), status);
return; <<<<<<<<<<<<<<<<<
}
Note that early return. If PTRACE_O_TRACEFORK isn't supported, we're
not checking PTRACE_O_TRACESYSGOOD. This didn't use to be a problem
before the unification of this whole detection business in
linux-ptrace.c. Before, the sysgood detection was completely
separate:
static void
linux_test_for_tracesysgood (int original_pid)
{
int ret;
sigset_t prev_mask;
/* We don't want those ptrace calls to be interrupted. */
block_child_signals (&prev_mask);
linux_supports_tracesysgood_flag = 0;
ret = ptrace (PTRACE_SETOPTIONS, original_pid, 0, PTRACE_O_TRACESYSGOOD);
if (ret != 0)
goto out;
linux_supports_tracesysgood_flag = 1;
out:
restore_child_signals_mask (&prev_mask);
}
So we need to get back the decoupling somehow. I think it's cleaner
to split the seperate feature detections to separate functions. This
patch does that. The new functions are named for their counterparts
that existed before this code was moved to linux-ptrace.c.
Note I've used forward declarations for the new functions to make the
patch clearer, as otherwise the patch would look like I'd be adding a
bunch of new code. A reorder can be done in a follow up patch.
Tested on x86_64 Fedora 17.
gdb/
2013-10-03 Pedro Alves <palves@redhat.com>
* common/linux-ptrace.c (linux_check_ptrace_features): Factor out
the PTRACE_O_TRACESYSGOOD and PTRACE_O_TRACEFORK to separate
functions. Always test for PTRACE_O_TRACESYSGOOD even if
PTRACE_O_TRACEFORK is not supported.
(linux_test_for_tracesysgood): New function.
(linux_test_for_tracefork): New function, factored out from
linux_check_ptrace_features, and also don't kill child_pid here.
---
gdb/common/linux-ptrace.c | 83 +++++++++++++++++++++++++--------------------
1 file changed, 46 insertions(+), 37 deletions(-)
diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 3a8e25e..9f11f2f 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -308,13 +308,15 @@ linux_child_function (gdb_byte *child_stack)
_exit (0);
}
+static void linux_test_for_tracesysgood (int child_pid);
+static void linux_test_for_tracefork (int child_pid);
+
/* Determine ptrace features available on this target. */
static void
linux_check_ptrace_features (void)
{
int child_pid, ret, status;
- long second_pid;
/* Initialize the options. */
current_ptrace_options = 0;
@@ -335,42 +337,60 @@ linux_check_ptrace_features (void)
error (_("linux_check_ptrace_features: waitpid: unexpected status %d."),
status);
- /* First, set the PTRACE_O_TRACEFORK option. If this fails, we
- know for sure that it is not supported. */
- ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
+ linux_test_for_tracesysgood (child_pid);
- if (ret != 0)
+ linux_test_for_tracefork (child_pid);
+
+ /* Clean things up and kill any pending children. */
+ do
{
ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
- {
- warning (_("linux_check_ptrace_features: failed to kill child"));
- return;
- }
-
- ret = my_waitpid (child_pid, &status, 0);
- if (ret != child_pid)
- warning (_("linux_check_ptrace_features: failed "
- "to wait for killed child"));
- else if (!WIFSIGNALED (status))
- warning (_("linux_check_ptrace_features: unexpected "
- "wait status 0x%x from killed child"), status);
-
- return;
+ warning (_("linux_check_ptrace_features: failed to kill child"));
+ my_waitpid (child_pid, &status, 0);
}
+ while (WIFSTOPPED (status));
+}
+
+/* Determine if PTRACE_O_TRACESYSGOOD can be used to catch
+ syscalls. */
+static void
+linux_test_for_tracesysgood (int child_pid)
+{
#ifdef GDBSERVER
- /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
- PTRACE_O_TRACEVFORKDONE yet. */
+ /* gdbserver does not support PTRACE_O_TRACESYSGOOD. */
#else
- /* Check if the target supports PTRACE_O_TRACESYSGOOD. */
+ int ret;
+
ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
if (ret == 0)
current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
+#endif
+}
+
+/* Determine if PTRACE_O_TRACEFORK can be used to follow fork
+ events. */
+static void
+linux_test_for_tracefork (int child_pid)
+{
+ int ret, status;
+ long second_pid;
+
+ /* First, set the PTRACE_O_TRACEFORK option. If this fails, we
+ know for sure that it is not supported. */
+ ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+ (PTRACE_TYPE_ARG4) PTRACE_O_TRACEFORK);
+
+ if (ret != 0)
+ return;
+
+#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
@@ -394,7 +414,7 @@ linux_check_ptrace_features (void)
ret = ptrace (PTRACE_CONT, child_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
- warning (_("linux_check_ptrace_features: failed to resume child"));
+ warning (_("linux_test_for_tracefork: failed to resume child"));
ret = my_waitpid (child_pid, &status, 0);
@@ -431,25 +451,14 @@ linux_check_ptrace_features (void)
ret = ptrace (PTRACE_KILL, second_pid, (PTRACE_TYPE_ARG3) 0,
(PTRACE_TYPE_ARG4) 0);
if (ret != 0)
- warning (_("linux_check_ptrace_features: "
+ warning (_("linux_test_for_tracefork: "
"failed to kill second child"));
my_waitpid (second_pid, &status, 0);
}
}
else
- warning (_("linux_check_ptrace_features: unexpected result from waitpid "
+ warning (_("linux_test_for_tracefork: unexpected result from waitpid "
"(%d, status 0x%x)"), ret, status);
-
- /* Clean things up and kill any pending children. */
- do
- {
- ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) 0);
- if (ret != 0)
- warning (_("linux_check_ptrace_features: failed to kill child"));
- my_waitpid (child_pid, &status, 0);
- }
- while (WIFSTOPPED (status));
}
/* Enable reporting of all currently supported ptrace events. */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
2013-10-01 5:16 ` Sergio Durigan Junior
2013-10-02 19:41 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] " Pedro Alves
@ 2013-10-03 18:40 ` Pedro Alves
2013-10-03 19:53 ` Tom Tromey
2013-10-03 22:02 ` Philippe Waroquiers
2013-10-04 4:22 ` Luis Machado
3 siblings, 2 replies; 30+ messages in thread
From: Pedro Alves @ 2013-10-03 18:40 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Sergio Durigan Junior, gdb-patches
On 09/29/2013 04:04 PM, Philippe Waroquiers wrote:
> On Fri, 2013-09-27 at 17:55 -0300, Sergio Durigan Junior wrote:
>> Still have some formatting issues, as pointed by Tom on IRC.
> Thanks, handled all comments (formatting and others),
> just a question below about the -1 syscall nr.
>
>
>>> + if (the_low_target.get_syscall_trapinfo == NULL)
>>> + {
>>> + *sysno = 0;
>>> + *sysret = 0;
>>> + return;
>>> + }
>>
>> Just a reminder that you were going to check if -1 is a better value to
>> represent this case. And if it is, then it should be documented, of course.
> Tested with -1 (on x86_64), and it works.
> I would have liked to re-use UNKNOWN_SYSCALL defined in gdbarch.h, but
> this is not included in gdbserver, and it sounds strange to me to
> include it to just get this constant.
> So, for the moment, I have just used -1 directly.
> Is there a correct place to define this constant ?
>
> Also, not sure what is meant by 'should be documented'.
> Do you mean add a sentence in the protocol documentation such as:
> "In case the remote stub cannot determine the syscall number,
> the remote stub must send -1 as syscall nr (encoded in hex,
> e.g. ffffffff)."
Please don't ever send back ffffffff as meaning -1.
Numbers in the RSP are variable length hex. You can't assume the
host side interprets them as two's complement 32-bit integers. If
sending a negative number, the '-' sign needs to be there explicitly
in the packet, like '-1'. Perhaps it would even be better to leave
-1 as implementation detail, and at the protocol level send something
else, like "unknown" or just "-", or nothing.
> (do we really want that to be part of the protocol spec ?
> This is really a very special case caused by a user "strange"
> manipulation of forcing to use the packet QCatchSyscalls).
Yes, please specify how that should work. It can happen, so
better define it. It should also be possible for the server to be
dumb, and have GDB itself figure out the syscall number. GDB
could read the registers itself, just like gdbserver is currently
doing. I'd assume the -ENOSYS trick might not always work on
all archs, for instance?
>
>
> In the meantime, find below the version 4 of the patch implementing
> catch syscall for gdbserver.
> The changes compared to v3 are:
> * handles the additional review comments given by Sergio Durigan Junior
> In particular, in case the syscall nr cannot be fetched from the low
> linux target, returns -1 syscall nr.
> * removes the useless "syscall" field in the stop_reply (i.e. similar
> to the removal of fields solibs_changes/replay_event done by Pedro Alves).
> * formatting changes (I hope I got the formatting all correct this time).
>
> Tested on x86_64, using:
> for t in "" "--target_board native-extended-gdbserver" "--target_board native-gdbserver"
> do
> make check RUNTESTFLAGS="$t"
> ....
> done
> (still without libexpat, waiting for Pedro's patch to be committed).
>
> Thanks ...
>
> Philippe
>
>
> 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.
QCatch not Qcatch.
> * remote.c (PACKET_QCatchSyscalls): New.
> (remote_protocol_features): Add QcatchSyscalls.
Ditto.
> (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.
> * common/linux-ptrace.c (linux_check_ptrace_features):
> Detect PTRACE_O_TRACESYSGOOD for gdbserver.
> (ptrace_supports_feature): Initializes ptrace features if needed.
s/Initializes/Initialize/
>
> doc/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * gdb.texinfo (General Query Packets): Document new QCatchSyscalls
> packet.
> (Stop Reply Packets): Document new stop reasons syscall_entry and
> syscall_return.
> (Async Records): Fixed syscall-return item name.
s/Fixed/Fix/.
>
> gdbserver/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * target.h (struct target_ops): Add supports_catch_syscall operation.
s/operation/field/.
> * linux-low.h (struct linux_target_ops): Add get_syscall_trapinfo
> operation.
Ditto.
> * linux-low.c (linux_wait_1): Enable, detect and handle
> SYSCALL_SIGTRAP.
> (gdb_catch_this_syscall_p): New function.
> (get_syscall_trapinfo): New function.
> (linux_supports_catch_syscall): New function.
> (struct target_ops linux_target_ops): Set linux_supports_catch_syscall.
Just write (linux_target_ops).
> * linux-x86-low.c (x86_get_syscall_trapinfo): New function.
> (struct linux_target_ops the_low_target): Set x86_get_syscall_trapinfo.
(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 catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
* server.h (catching_syscalls_p, syscalls_to_catch_size)
(syscalls_to_catch): Declare.
> * server.c: Define catching_syscalls_p, syscalls_to_catch_size and
> syscalls_to_catch.
* server.c (catching_syscalls_p, syscalls_to_catch_size)
(syscalls_to_catch): Define.
> (handle_general_set): Handle QCatchSyscalls packet.
> (handle_query): Reports if low target supports QCatchSyscalls.
s/Reports/Report/
> * win32-low.c (struct target_ops win32_target_op): Sets NULL
> for supports_catch_syscall.
* win32-low.c (win32_target_op): Set supports_catch_syscall to
NULL.
>
> testsuite/ChangeLog
> 2013-xx-yy Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * gdb.base/catch-syscall.exp: Enables test for x86 and amd64
> gdbserver.
s/Enables/Enable/.
>
>
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.616
> diff -u -p -r1.616 NEWS
> --- NEWS 25 Sep 2013 23:17:11 -0000 1.616
> +++ NEWS 29 Sep 2013 14:20:27 -0000
> @@ -157,11 +157,20 @@ 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.
So, "catch syscall" is per-inferior/process on the GDB side, but
this always sets the catchpoints on all processes. Was that
intended?
> +
> +
> * 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. Currently enabled
> + on x86/x86_64 GNU/Linux targets, and in the Valgrind gdbserver.
Really, Valgrind news don't belong here.
> +
> ** 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.579
> diff -u -p -r1.579 remote.c
> --- remote.c 27 Sep 2013 15:29:06 -0000 1.579
> +++ remote.c 29 Sep 2013 14:20:29 -0000
> @@ -1340,6 +1340,7 @@ enum {
> PACKET_qSupported,
> PACKET_qTStatus,
> PACKET_QPassSignals,
> + PACKET_QCatchSyscalls,
> PACKET_QProgramSignals,
Odd place to put this. Doesn't see to follow any logic?
(suspect merge-conflict resolution). PACKET_QPassSignals
and QProgramSignals are related. At least please leave them
together.
> PACKET_qSearch_memory,
> PACKET_vAttach,
> @@ -1728,6 +1729,98 @@ remote_pass_signals (int numsigs, unsign
> }
> }
>
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> + to report syscalls to GDB. */
> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> + int table_size, int *table)
> +{
> + char *catch_packet, *p;
> + enum packet_result result;
> + int n_sysno = 0;
> +
> + if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
> + {
> + /* Not supported. */
> + return 1;
> + }
> +
> + if (needed && !any_count)
if (needed && any_count == 0)
> + {
> + int i;
> +
> + /* Count how many syscalls are to be caught (table[sysno] != 0). */
> + for (i = 0; i < table_size; i++)
> + if (table[i])
You got the style right in the comment, but not in the code.
You don't really need that bit in the comment though, it's clear
already from the code:
/* Count how many syscalls are to be caught. */
for (i = 0; i < table_size; i++)
if (table[i] != 0)
> + 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";
This creates a string, and then a const pointer that points
to the string. If you declare it like this:
const char q1[] = "QCatchSyscalls:1";
you spare creating a pointer.
> + int i;
> + const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
And then, Sergio's (IIRC) suggestion would work:
const int maxpktsz = sizeof (q1) - 1 + n_sysno * 9 + 1;
> +
> + catch_packet = xmalloc (maxpktsz);
> + strcpy (catch_packet, q1);
> + if (!any_count)
if (any_count == 0)
> + {
> + char *p;
> +
> + p = catch_packet;
> + p += strlen (p);
> +
> + /* Add in catch_packet each syscall to be caught (table[i] != 0). */
> + for (i = 0; i < table_size; i++)
> + {
> + if (table[i])
Same comment as above.
> + {
> + 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;
(
catch_packet[sizeof (q1) - 1] = 0;
etc.
)
> + }
> + }
> + else
> + {
> + catch_packet = xmalloc (strlen ("QCatchSyscalls:0") + 1);
> + strcpy (catch_packet, "QCatchSyscalls:0");
Just write:
catch_packet = xstrdup ("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);
This should be done in a cleanup instead. putpkt/getpkt can throw.
> + if (result == PACKET_OK)
> + return 0;
> + else
> + return -1;
> + }
> +}
> +
> +
> +
> /* If 'QProgramSignals' is supported, tell the remote stub what
> signals it should pass through to the inferior when detaching. */
>
> @@ -4016,6 +4109,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,
> @@ -5591,6 +5686,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;
> +
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + p = unpack_varlen_hex (++p1, &sysno);
> + event->ws.value.syscall_number = (int) sysno;
> + }
> + else if (strncmp (p, "syscall_return", p1 - p) == 0)
> + {
> + ULONGEST sysno;
> +
> + event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
> + p = unpack_varlen_hex (++p1, &sysno);
> + 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))
> @@ -11445,6 +11556,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;
> @@ -11939,6 +12051,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 29 Sep 2013 14:20:29 -0000
> @@ -361,16 +361,18 @@ 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.
> + We keep PTRACE_O_TRACEFORK option activated as a fork
> + event notification is expected after my_waitpid below. */
> 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));
This was discussed in a separate email.
> 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 +476,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 ();
> +
> 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.1113
> diff -u -p -r1.1113 gdb.texinfo
> --- doc/gdb.texinfo 25 Sep 2013 23:17:12 -0000 1.1113
> +++ doc/gdb.texinfo 29 Sep 2013 14:20:34 -0000
> @@ -18752,6 +18752,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}}
> @@ -38153,6 +38157,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.
> @@ -38523,6 +38532,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 syscall is not caught. It is however more efficient
> +to only report the needed syscalls.
I think "if this syscall is not caught" is a little confusing. I'd suggest:
"Note that if a syscall not member of the list is reported, @value{GDBN}
will still filter it out. It is however more efficient to only report
the requested 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
> @@ -38886,6 +38933,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{-}
> @@ -39046,6 +39098,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.248
> diff -u -p -r1.248 linux-low.c
> --- gdbserver/linux-low.c 5 Sep 2013 20:45:39 -0000 1.248
> +++ gdbserver/linux-low.c 29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,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
> @@ -481,6 +486,38 @@ get_pc (struct lwp_info *lwp)
> return pc;
> }
>
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
> + Fill *SYSNO with the syscall nr trapped. Fill *SYSRET with the
> + return code. */
> +
> +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)
> + {
> + /* If we cannot get the syscall trapinfo, report an unknown
> + system call using -1 values. */
> + *sysno = -1;
> + *sysret = -1;
> + return;
> + }
> +
> + 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.
>
> @@ -2248,6 +2285,29 @@ linux_stabilize_threads (void)
> }
> }
>
> +/* Returns 1 if GDB is interested in the event_child syscall.
> +/* Returns 1 if GDB is interested in EVENT_CHILD's syscall.
> + Only to be called when stopped reason is SIGTRAP_SYSCALL. */
> +
> +static int
> +gdb_catch_this_syscall_p (struct lwp_info *event_child)
"this" looks odd, as a number is not passed as argument.
"gdb catch this syscall" doesn't really parse correctly.
I'd suggest: gdb_catching_syscall, gdb_catch_syscall_p,
or, gdb_interested_in_syscall_p. (the latter as there's
no need to describe this with "interested" everywhere
in comments, and then use another term in the function
name).
> +{
> + int i;
> + int sysno, sysret;
> +
> + if (!catching_syscalls_p)
> + return 0;
> +
> + if (syscalls_to_catch_size == 0)
> + return 1;
> +
> + get_syscall_trapinfo (event_child, &sysno, &sysret);
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + if (syscalls_to_catch[i] == sysno)
> + return 1;
> +
> + return 0;
> +}
> +
> /* Wait for process, returns status. */
>
> static ptid_t
> @@ -2529,6 +2589,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
> + && !gdb_catch_this_syscall_p (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
> @@ -2707,7 +2780,24 @@ 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);
> + /* Differentiate entry from return using the return code
> + -ENOSYS. This is not ideal as a syscall return from a not
> + implemented syscall will be seen as an entry. However, this
> + resists well to enabling/disabling catch syscall at various
> + moment. A better way to differentiate entry/return in GDB
> + and GDBSERVER would be nice. */
Indentation is wrong.
> + if (sysret == -ENOSYS)
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> + else
> + ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;
The whole 'SIGTRAP | 0x80' thing is super silly. The ideal way
would be for the kernel to tell us. See:
https://sourceware.org/gdb/wiki/LinuxKernelWishList
"A PTRACE_O_SYSCALL(_ENTER|_EXIT) ptrace option, along with
PTRACE_EVENT_SYSCALL(_ENTER|_EXIT). PTRACE_SYSCALL/sysgood doesn't leave
any way to trace syscalls when single-stepping. Separate syscall enter
and exit events would just be nice, to avoid having to keep track of
enter/exit - the kernel knows, might as well give us the info. (While
at it, a way to filter which syscalls trigger events would be nice to
avoid unnecessary slowing down the inferior until it calls the syscall the user
is interested in (you can catch specific syscalls with "catch syscall foosyscall";
gdb currently filters after the fact), though that's obviously a
bit more work.)"
Want to work on that? It'd be great...
Another idea would be, in addition to supporting syscall_entry
and syscall_exit stop replies, also support plain "syscall" (and
a new TARGET_WAITKIND_SYSCALL), meaning, "I don't know if it was
an entry or an exit, you decide." (just like SYSCALL_SIGTRAP), and then
we'd leave GDB to decide. GDB will end up fetching registers
off the target anyway, so I think I'd end up costing 0 in terms of
performance.
Then when we have real PTRACE_EVENT_SYSCALL(_ENTER|_EXIT), we'd
use syscall_entry/syscall_exit.
(not completely sure about this).
How portable is this -ENOSYS thing? What does strace do?
BTW, it'd be very very nice if GDB and GDBserver didn't end
up with different mechanisms for this. We've been trying to
make the gdb and gdbserver backends more similar, with the end
goal of merging them, and more points of divergence don't really
help that.
> + }
> + 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,
> @@ -3098,6 +3188,7 @@ linux_resume_one_lwp (struct lwp_info *l
> {
> struct thread_info *saved_inferior;
> int fast_tp_collecting;
> + int ptrace_request;
>
> if (lwp->stopped == 0)
> return;
> @@ -3269,7 +3360,14 @@ 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),
> + if (step)
> + ptrace_request = PTRACE_SINGLESTEP;
> + else if (catching_syscalls_p)
> + ptrace_request = PTRACE_SYSCALL;
> + else
> + ptrace_request = PTRACE_CONT;
> + ptrace (ptrace_request,
> + lwpid_of (lwp),
> (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. */
> @@ -5108,6 +5206,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());
Missing space "linux_supports_tracesysgood()".
> +}
> +
> +static int
> linux_supports_tracepoints (void)
> {
> if (*the_low_target.supports_tracepoints == NULL)
> @@ -5798,6 +5903,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 29 Sep 2013 14:20:35 -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
Spare the reader unnecessary effort and spell out "number".
> + 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.51
> diff -u -p -r1.51 linux-x86-low.c
> --- gdbserver/linux-x86-low.c 5 Sep 2013 20:40:58 -0000 1.51
> +++ gdbserver/linux-x86-low.c 29 Sep 2013 14:20:35 -0000
> @@ -1474,6 +1474,36 @@ x86_arch_setup (void)
> current_process ()->tdesc = x86_linux_read_description ();
> }
>
> +/* Fill *SYSNO and *SYSRET with the syscall nr trapped and the syscall return
Likewise.
> + code. This should only be called if LWP got a SIGTRAP_SYSCALL. */
Write "if the LWP". "if LWP" kind of implies there's an LWP
parameter, while there's no such thing.
> +
> +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;
> +
> + 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;
> +
> + 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;
> + }
This else branch can be simplified to:
{
collect_register_by_name (regcache, "orig_eax", sysno);
collect_register_by_name (regcache, "eax", sysret);
}
> +}
> +
> static int
> x86_supports_tracepoints (void)
> {
> @@ -3323,6 +3353,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.99
> diff -u -p -r1.99 remote-utils.c
> --- gdbserver/remote-utils.c 5 Sep 2013 20:41:55 -0000 1.99
> +++ gdbserver/remote-utils.c 29 Sep 2013 14:20:35 -0000
> @@ -1321,12 +1321,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);
SIGTRAP is host-dependent, while this is an RSP signal number.
Write:
sprintf (buf, "T%02x", GDB_SIGNAL_TRAP);
In practice, SIGTRAP==5==GDB_SIGNAL_TRAP just about
everywhere, but it's always better to write what we mean.
> buf += strlen (buf);
>
> saved_inferior = current_inferior;
> @@ -1337,6 +1342,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.201
> diff -u -p -r1.201 server.c
> --- gdbserver/server.c 18 Sep 2013 01:59:59 -0000 1.201
> +++ gdbserver/server.c 29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,9 @@ int debug_threads;
> int debug_hw_points;
>
> int pass_signals[GDB_SIGNAL_LAST];
> +int catching_syscalls_p;
No need for the "_p" suffix. "catching_syscalls" is
already clearly a predicate.
> +int syscalls_to_catch_size;
> +int *syscalls_to_catch;
> int program_signals[GDB_SIGNAL_LAST];
> int program_signals_p;
Odd positioning... I suspect you got that by resolving
a conflict. Please leave pass_signals and program_signals
together, as they're related.
> @@ -511,6 +514,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;
> +
> + catching_syscalls_p = 1;
> + if (syscalls_to_catch != NULL)
> + {
> + xfree (syscalls_to_catch);
> + syscalls_to_catch = NULL;
> + }
> + syscalls_to_catch_size = 0;
catching_syscalls_p = 1;
xfree (syscalls_to_catch);
syscalls_to_catch = NULL;
syscalls_to_catch_size = 0;
> + p = own_buf + strlen("QCatchSyscalls:1");
Missing space.
> + if (syscalls_to_catch_size > 0)
> + {
> + syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
> + p = strchr(own_buf, ';') + 1;
Above you used 'p = own_buf + strlen ("QCatchSyscalls:1")'.
Please do the same here so we're consistent.
> + for (i = 0; i < syscalls_to_catch_size; i++)
> + {
> + p = decode_address_to_semicolon (&sysno, p);
> + syscalls_to_catch[i] = (int) sysno;
> + }
> + }
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> + if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> + {
> + catching_syscalls_p = 0;
> + strcpy (own_buf, "OK");
> + return;
> + }
> +
> if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
> {
> int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
> "PacketSize=%x;QPassSignals+;QProgramSignals+",
> PBUFSIZ - 1);
>
> + if (target_supports_catch_syscall ())
> + 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.118
> diff -u -p -r1.118 server.h
> --- gdbserver/server.h 5 Sep 2013 20:45:39 -0000 1.118
> +++ gdbserver/server.h 29 Sep 2013 14:20:35 -0000
> @@ -115,6 +115,15 @@ extern int server_waiting;
> extern int debug_threads;
> extern int debug_hw_points;
> extern int pass_signals[];
> +
> +/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise. */
> +extern int catching_syscalls_p;
> +/* syscalls_to_catch is the list of syscalls to report to GDB.
> + If catching_syscalls_p and syscalls_to_catch == NULL, it means
> + all syscalls must be reported. */
> +extern int syscalls_to_catch_size;
> +extern int *syscalls_to_catch;
> +
> extern int program_signals[];
> extern int program_signals_p;
Likewise, keep pass_signals and program_signals together please.
>
> Index: gdbserver/target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
> retrieving revision 1.71
> diff -u -p -r1.71 target.h
> --- gdbserver/target.h 5 Sep 2013 20:41:22 -0000 1.71
> +++ gdbserver/target.h 29 Sep 2013 14:20:35 -0000
> @@ -268,6 +268,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);
> @@ -414,6 +418,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.69
> diff -u -p -r1.69 win32-low.c
> --- gdbserver/win32-low.c 5 Sep 2013 20:45:39 -0000 1.69
> +++ gdbserver/win32-low.c 29 Sep 2013 14:20:35 -0000
> @@ -1824,6 +1824,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 29 Sep 2013 14:20:36 -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
> +}
This isn't strictly correct, as it's quite concievable that
e.g., a non-x86 valgrind (or some other random target) gets
support for these packets before our own corresponding gdbserver
port gets it. Ideally we'd probe for support somehow, like by
looking for some GDB error/warning in the first "catch syscall", or
some such.
> +
> # This shall be updated whenever 'catch syscall' is implemented
> # on some architecture.
> #if { ![istarget "i\[34567\]86-*-linux*"]
git diff tells me the patch adds a bunch of trailing whitespace.
Please fix that up throughout.
Other issues:
- I think that if you set a catch syscall, and then disconnect
in a way that gdbserver leaves the target running (disconnected
tracing, or if we have any persistent commands, see server.c), we'll end up
with the program held stopped as soon as the program calls a
syscall. I think at least handle_target_event should be updated.
- linux-nat.c:linux_handle_syscall_trap has some hairy stuff
when "stopping" is true, due to syscall restarts. I was
a little surprised that the gdbserver patch didn't need
anything of the sort, but then again, I don't recall whether
that bits is covered by the testsuite.
I'm wondering whether this -ENOSYS mechanism would allow
getting rid of all that, of if something is missing on the
gdbserver side.
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-03 18:40 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
@ 2013-10-03 19:53 ` Tom Tromey
2013-10-04 17:41 ` Pedro Alves
2013-10-03 22:02 ` Philippe Waroquiers
1 sibling, 1 reply; 30+ messages in thread
From: Tom Tromey @ 2013-10-03 19:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: Philippe Waroquiers, Sergio Durigan Junior, gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>> +QCatchSyscalls:1 [;SYSNO]...
>> +QCatchSyscalls:0
>> + Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
>> + catching syscalls from the inferior process.
Pedro> So, "catch syscall" is per-inferior/process on the GDB side, but
Pedro> this always sets the catchpoints on all processes. Was that
Pedro> intended?
I wonder whether it is the right thing on the gdb side.
Right now we have the rule that linespecs for breakpoints apply to all
inferiors; but this rule isn't followed for catchpoints.
I tend to think it ought to be, for consistency and simplicity; followed
up by using it{etc}sets for filtering out uninteresting events.
I don't want to derail this patch though.
And arguably it is ok for gdb to present one thing to the user but more
useful for gdbserver to present a different view to gdb.
Tom
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-03 18:40 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
2013-10-03 19:53 ` Tom Tromey
@ 2013-10-03 22:02 ` Philippe Waroquiers
2013-10-04 17:29 ` Pedro Alves
1 sibling, 1 reply; 30+ messages in thread
From: Philippe Waroquiers @ 2013-10-03 22:02 UTC (permalink / raw)
To: Pedro Alves; +Cc: Sergio Durigan Junior, gdb-patches
Thanks for the in depth comments (sorry for still not getting all
style aspects ok).
I can handle the (easy) comments in the coming days.
However, there are a few not easy comments on which I have
questions/(long) feedback below ...
On Thu, 2013-10-03 at 19:40 +0100, Pedro Alves wrote:
> Please don't ever send back ffffffff as meaning -1.
> Numbers in the RSP are variable length hex. You can't assume the
> host side interprets them as two's complement 32-bit integers. If
> sending a negative number, the '-' sign needs to be there explicitly
> in the packet, like '-1'. Perhaps it would even be better to leave
> -1 as implementation detail, and at the protocol level send something
> else, like "unknown" or just "-", or nothing.
>
> > (do we really want that to be part of the protocol spec ?
> > This is really a very special case caused by a user "strange"
> > manipulation of forcing to use the packet QCatchSyscalls).
>
> Yes, please specify how that should work. It can happen, so
> better define it. It should also be possible for the server to be
> dumb, and have GDB itself figure out the syscall number. GDB
> could read the registers itself, just like gdbserver is currently
> doing. I'd assume the -ENOSYS trick might not always work on
> all archs, for instance?
I agree "unknown" or "-" or something like that is cleaner.
For what concerns the -ENOSYS nasty trick: no, this does not work on
all archs. I quickly hacked ppc64 gdbserver, and there the
nasty trick is to compare sysret and orig_r3 : if identical, it is
a syscall entry.
All these logics are all today partially wrong:
* The "flip/flop" logic in native GDB becomes completely lost
in case of enable/disable e.g. between entry and exit).
* x86 gdbserver has a minor bug (I think): a syscall "exit" from
a not implemented syscall is seen as an entry,
* the hacky ppc64 trick is wrong in case the sysret of the previous
syscall is equal to the r3 argument of the new syscall.
In summary: differentiate syscall entry from exit is currently
fully correct nowhere (the less bad being gdbserver x86, which is almost
always correct and "repairs" itself). ppc64 is not always correct,
but also repairs "itself". The flip/flop logic, once broken, confuses
all entry/exits until further notice.
I think we need to have something better e.g. in common/linux-ptrace.c
which would maintain the correct expected state using maybe the
following behaviour described in the ptrace man :
Syscall-enter-stop and syscall-exit-stop are indistinguishable from
each other by the tracer. The tracer needs to keep track of the
sequence of ptrace-stops in order to not misinterpret syscall-enter-
stop as syscall-exit-stop or vice versa. The rule is that syscall-
enter-stop is always followed by syscall-exit-stop, PTRACE_EVENT stop
or the tracee's death; no other kinds of ptrace-stop can occur in
between.
It is however not clear to me how the above works if e.g. gdb attaches
to a process which is blocked in a syscall, and then does PTRACE_SYSCALL
Will GDB get an entry for such a restarted syscall ?
Or will it get only an exit ?
I was intending to improve (and factorise) this logic after the RSP
protocol patch, but I have to admit I do not understand the above man.
I first intended to make a regtest showing the bug in the native
implementation.
> > +QCatchSyscalls:1 [;SYSNO]...
> > +QCatchSyscalls:0
> > + Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
> > + catching syscalls from the inferior process.
>
> So, "catch syscall" is per-inferior/process on the GDB side, but
> this always sets the catchpoints on all processes. Was that
> intended?
As I understand now (after the hint about add-inferior you gave
on IRC), gdbserver supports multi process.
However, it is not clear to me how gdbserver properly processes
e.g. the packet QPassSignals.
The QCatchSyscalls packet is just following the QCatchSyscalls packet.
So, if I understand properly the comment, the QCatchSyscalls packet
should modify a list of syscalls to maintain per process
i.e. in the inferiors.h struct process_info ?
To set the process to be used for a QCatchSyscalls packet, gdb will
then first have to send a Hg packet.
Do I correctly understand the comment (and the way GDB will or should
use Hg to indicate the inferior for the next QCatchSyscalls) ?
> The whole 'SIGTRAP | 0x80' thing is super silly. The ideal way
> would be for the kernel to tell us. See:
>
> https://sourceware.org/gdb/wiki/LinuxKernelWishList
>
> "A PTRACE_O_SYSCALL(_ENTER|_EXIT) ptrace option, along with
> PTRACE_EVENT_SYSCALL(_ENTER|_EXIT). PTRACE_SYSCALL/sysgood doesn't leave
> any way to trace syscalls when single-stepping. Separate syscall enter
> and exit events would just be nice, to avoid having to keep track of
> enter/exit - the kernel knows, might as well give us the info. (While
> at it, a way to filter which syscalls trigger events would be nice to
> avoid unnecessary slowing down the inferior until it calls the syscall the user
> is interested in (you can catch specific syscalls with "catch syscall foosyscall";
> gdb currently filters after the fact), though that's obviously a
> bit more work.)"
>
> Want to work on that? It'd be great...
Well, I will first concentrate on trying to make a correct GDB patch
and properly follow the GDB coding style rules.
>
> Another idea would be, in addition to supporting syscall_entry
> and syscall_exit stop replies, also support plain "syscall" (and
> a new TARGET_WAITKIND_SYSCALL), meaning, "I don't know if it was
> an entry or an exit, you decide." (just like SYSCALL_SIGTRAP), and then
> we'd leave GDB to decide. GDB will end up fetching registers
> off the target anyway, so I think I'd end up costing 0 in terms of
> performance.
>
> Then when we have real PTRACE_EVENT_SYSCALL(_ENTER|_EXIT), we'd
> use syscall_entry/syscall_exit.
>
> (not completely sure about this).
Not sure also. Adding this in the protocol before we know it is needed
is not ideal. However, it looks to me that not adding it now means
that a GDB supporting only syscall enter/exit will have a problem
with a newer gdbserver that would give back the 3rd reason.
>
> How portable is this -ENOSYS thing? What does strace do?
-ENOSYS is documented in man ptrace, but only for x86.
strace uses -ENOSYS for x86, and (from what I understood reading
the code) cannot differentiate entry/exit (in all cases) for other
archs.
I suspect strace uses for these archs a flip/flop which might
suffer from the same bug/weakness as the native GDB flip/flop.
>
>
> BTW, it'd be very very nice if GDB and GDBserver didn't end
> up with different mechanisms for this. We've been trying to
> make the gdb and gdbserver backends more similar, with the end
> goal of merging them, and more points of divergence don't really
> help that.
Yes, I agree. I was intending to look more in depth to that after
the RSP patch.
> > +# 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
> > +}
>
> This isn't strictly correct, as it's quite concievable that
> e.g., a non-x86 valgrind (or some other random target) gets
> support for these packets before our own corresponding gdbserver
> port gets it. Ideally we'd probe for support somehow, like by
> looking for some GDB error/warning in the first "catch syscall", or
> some such.
Effectively, at Valgrind side, it will be supported for all
archs out of the box (as Valgrind has already an arch independent
syscall interception, and properly identifies entry from exit).
However, up to now, I never dared to run the gdb testsuite with
Valgrind gdbserver.
> - I think that if you set a catch syscall, and then disconnect
> in a way that gdbserver leaves the target running (disconnected
> tracing, or if we have any persistent commands, see server.c), we'll end up
> with the program held stopped as soon as the program calls a
> syscall. I think at least handle_target_event should be updated.
I will need to dig into that.
>
> - linux-nat.c:linux_handle_syscall_trap has some hairy stuff
> when "stopping" is true, due to syscall restarts. I was
> a little surprised that the gdbserver patch didn't need
> anything of the sort, but then again, I don't recall whether
> that bits is covered by the testsuite.
> I'm wondering whether this -ENOSYS mechanism would allow
> getting rid of all that, of if something is missing on the
> gdbserver side.
I believe the hairy stuff for native is to implement the flip/flop
logic, but this logic is broken when enabling/disabling.
-ENOSYS does not suffer from this problem (see above), but is
arch dependent.
So, I believe the current x86 gdbserver -ENOSYS is better/simpler,
but for other archs, it seems such tricks do not exist or are
not working as well (e.g. ppc64).
I am not sure how to continue with this problem of differentiating
entry/exit, together with the GDB 7.7 deadline.
I see 3 approaches:
1. implement the QCatchSyscalls packet as it is now
(with only entry/exit), and -ENOSYS for x86,
and see later what to do better for other archs or native GDB.
2. Same as 1, but we add the 3rd stop reasons
syscall_no_idea_if_it_is_an_entry_or_return
so as to have the protocol ready for archs less easy than x86.
3. work longer on the patch, first trying to have a proper
way to differentiate entry/return in both native GDB and
gdbserver, and preferrably arch independent.
(unclear if this is possible. If not, we fallback to
option 2 or 1).
(or possibly, the bug in the GDB flip/flop is easy to fix ?
then the flip/flop should be used by gdbserver also)
I guess 1 and 2 could be done on time for GDB 7.7, but I will not have
enough evening/WE time to finish 3 in let's say the next two weeks
(there is soon a valgrind 3.9.0 to go out, for which I have a few
things still to do).
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
` (2 preceding siblings ...)
2013-10-03 18:40 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
@ 2013-10-04 4:22 ` Luis Machado
2013-10-04 17:40 ` Pedro Alves
3 siblings, 1 reply; 30+ messages in thread
From: Luis Machado @ 2013-10-04 4:22 UTC (permalink / raw)
To: Philippe Waroquiers, Sergio Durigan Junior; +Cc: gdb-patches, Pedro Alves
On 09/29/2013 12:04 PM, Philippe Waroquiers wrote:
> 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.
I'm late to the party, but i've always wondered why we have all these
different "insert_<foo>_catchpoint" and "remove_<foo>_catchpoint"
functions to accomplish tasks that seem to be very similar in nature.
Not saying we should go this route for this patch, but we may want to
consider a more generic RSP packet for catchpoints. Something like the
following:
QInsertCatchpoint:[syscall|fork|exec|vfork|unload|...]
QRemoveCatchpoint:[syscall|fork|exec|vfork|unload|...]
... or even communicate catchpoints through Z/z packets, though that
would be a more radical approach.
Anyway, just throwing a few ideas since i've been dealing with some of
the issues with catchpoints, forking and gdbserver as well.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-03 22:02 ` Philippe Waroquiers
@ 2013-10-04 17:29 ` Pedro Alves
2013-10-05 9:15 ` Pedro Alves
0 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2013-10-04 17:29 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Sergio Durigan Junior, gdb-patches
On 10/03/2013 11:02 PM, Philippe Waroquiers wrote:
> Thanks for the in depth comments (sorry for still not getting all
> style aspects ok).
>
> I can handle the (easy) comments in the coming days.
>
> However, there are a few not easy comments on which I have
> questions/(long) feedback below ...
>
>
> On Thu, 2013-10-03 at 19:40 +0100, Pedro Alves wrote:
>
>> Please don't ever send back ffffffff as meaning -1.
>> Numbers in the RSP are variable length hex. You can't assume the
>> host side interprets them as two's complement 32-bit integers. If
>> sending a negative number, the '-' sign needs to be there explicitly
>> in the packet, like '-1'. Perhaps it would even be better to leave
>> -1 as implementation detail, and at the protocol level send something
>> else, like "unknown" or just "-", or nothing.
>>
>>> (do we really want that to be part of the protocol spec ?
>>> This is really a very special case caused by a user "strange"
>>> manipulation of forcing to use the packet QCatchSyscalls).
>>
>> Yes, please specify how that should work. It can happen, so
>> better define it. It should also be possible for the server to be
>> dumb, and have GDB itself figure out the syscall number. GDB
>> could read the registers itself, just like gdbserver is currently
>> doing. I'd assume the -ENOSYS trick might not always work on
>> all archs, for instance?
> I agree "unknown" or "-" or something like that is cleaner.
> For what concerns the -ENOSYS nasty trick: no, this does not work on
> all archs. I quickly hacked ppc64 gdbserver, and there the
> nasty trick is to compare sysret and orig_r3 : if identical, it is
> a syscall entry.
> All these logics are all today partially wrong:
> * The "flip/flop" logic in native GDB becomes completely lost
> in case of enable/disable e.g. between entry and exit).
> * x86 gdbserver has a minor bug (I think): a syscall "exit" from
> a not implemented syscall is seen as an entry,
> * the hacky ppc64 trick is wrong in case the sysret of the previous
> syscall is equal to the r3 argument of the new syscall.
>
> In summary: differentiate syscall entry from exit is currently
> fully correct nowhere (the less bad being gdbserver x86, which is almost
> always correct and "repairs" itself). ppc64 is not always correct,
> but also repairs "itself". The flip/flop logic, once broken, confuses
> all entry/exits until further notice.
>
> I think we need to have something better e.g. in common/linux-ptrace.c
> which would maintain the correct expected state using maybe the
> following behaviour described in the ptrace man :
> Syscall-enter-stop and syscall-exit-stop are indistinguishable from
> each other by the tracer. The tracer needs to keep track of the
> sequence of ptrace-stops in order to not misinterpret syscall-enter-
> stop as syscall-exit-stop or vice versa. The rule is that syscall-
> enter-stop is always followed by syscall-exit-stop, PTRACE_EVENT stop
> or the tracee's death; no other kinds of ptrace-stop can occur in
> between.
> It is however not clear to me how the above works if e.g. gdb attaches
> to a process which is blocked in a syscall, and then does PTRACE_SYSCALL
> Will GDB get an entry for such a restarted syscall ?
> Or will it get only an exit ?
I don't know for sure off hand, I'd have to experiment, but I think yes,
GDB gets a syscall entry for the restarted syscall. I think
that's similar to what I saw when I wrote that comment in
linux-nat.c:linux_handle_syscall_trap -- "Later, when the user re-resumes
this LWP, we'd see another syscall entry event".
> I was intending to improve (and factorise) this logic after the RSP
> protocol patch, but I have to admit I do not understand the above man.
> I first intended to make a regtest showing the bug in the native
> implementation.
Well, the above man snippet makes it look like GDB's implementation
is closer to the "correct" one, modulo the bug you found. That makes
me think that we should try and fix the GDB-side bug in the existing
framework (to make sure it's solvable using that approach), and then
depending on the result of that, make gdbserver follow the same
approach.
>>> +QCatchSyscalls:1 [;SYSNO]...
>>> +QCatchSyscalls:0
>>> + Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
>>> + catching syscalls from the inferior process.
>>
>> So, "catch syscall" is per-inferior/process on the GDB side, but
>> this always sets the catchpoints on all processes. Was that
>> intended?
> As I understand now (after the hint about add-inferior you gave
> on IRC), gdbserver supports multi process.
> However, it is not clear to me how gdbserver properly processes
> e.g. the packet QPassSignals.
The "handle" command, or anything signal related (even "catch signal")
on the GDB side is _not_ per process.
> The QCatchSyscalls packet is just following the QCatchSyscalls packet.
>
> So, if I understand properly the comment, the QCatchSyscalls packet
> should modify a list of syscalls to maintain per process
> i.e. in the inferiors.h struct process_info ?
Hmm, not sure that'd be the ultimate way. I think it may be best to
leave this global. Thinking forward, with something like itsets on
the target side, a given "catch syscall" could be applicable to all
processes/threads, or some process, or multiple processes, but also to some
thread, to multiple threads, or any mix of those things. When you put
threads in the mix, you start wondering whether to maintain the list
also per-thread. But then, it'd probably make more sense to
keep the list of syscall catchpoints a global list of syscall catchpoint
objects, where this object has a "scope" field attached. When querying
whether GDB is interested in the event, we'd iterate over all catchpoints,
and check if the event thread matches the scope field of each. In fact,
this is just like the breakpoint module on the GDB side checks whether a
thread-specific event matches any thread-specific breakpoint.
> To set the process to be used for a QCatchSyscalls packet, gdb will
> then first have to send a Hg packet.
I'm thinking that, looking ahead at itsets, with the concept of two
sets - the trigger set (which threads should trigger/cause a stop), and
the stop set (which threads should be implicitly paused when the event
triggers, e.g., stop the whole process), Hg will not be sufficient. We
need a "scoping" machinery like that for all sorts of breakpoint/catchpoint
packets. E.g., we don't do thread-specific breakpoints on the target
side yet, and that would also benefit from a generalized scoping operator
or some such. So it's probably best to come up later with a
generalized machinery for specifying those scopes. Not clear to me
whether or not that should be a separate packet (like Hg is). There's
also the merging to breakpoints on the GDB side and on the server side
to handle. E.g., if the user does "catch syscall 2" for process 1, and
the "catch syscall 2" for process 2, will that end up sending just
one QCatchSyscalls packet, or multiple? The "a QCatchSyscalls always
replaces a previous QCatchSyscalls" is a point to consider there. What if
we support target-side commands and conditions associated with QCatchSyscalls, but
each catchpoint has a different target-side condition? Then the way merging
is to be done isn't as clear.
Hmm, why _don't_ we support target-side commands and conditions with
this? Indeed it looks like a step backward to not support features we've
been adding to the Z packets. So it looks like Luis may be right in that
we should really consider doing catchpoints in the RSP differently.
Thinking only in terms of multi-process/thread, I'm inclined to ignore the
"per-process" thing now, and just leave it as gdbserver making catch
syscall apply to all processes.
> Do I correctly understand the comment (and the way GDB will or should
> use Hg to indicate the inferior for the next QCatchSyscalls) ?
Not sure what comment you're referring to.
>> The whole 'SIGTRAP | 0x80' thing is super silly. The ideal way
>> would be for the kernel to tell us. See:
>>
>> https://sourceware.org/gdb/wiki/LinuxKernelWishList
>>
>> "A PTRACE_O_SYSCALL(_ENTER|_EXIT) ptrace option, along with
>> PTRACE_EVENT_SYSCALL(_ENTER|_EXIT). PTRACE_SYSCALL/sysgood doesn't leave
>> any way to trace syscalls when single-stepping. Separate syscall enter
>> and exit events would just be nice, to avoid having to keep track of
>> enter/exit - the kernel knows, might as well give us the info. (While
>> at it, a way to filter which syscalls trigger events would be nice to
>> avoid unnecessary slowing down the inferior until it calls the syscall the user
>> is interested in (you can catch specific syscalls with "catch syscall foosyscall";
>> gdb currently filters after the fact), though that's obviously a
>> bit more work.)"
>>
>> Want to work on that? It'd be great...
> Well, I will first concentrate on trying to make a correct GDB patch
> and properly follow the GDB coding style rules.
Sure, I didn't mean to imply doing that as prerequisite for the
GDB patch.
>> Another idea would be, in addition to supporting syscall_entry
>> and syscall_exit stop replies, also support plain "syscall" (and
>> a new TARGET_WAITKIND_SYSCALL), meaning, "I don't know if it was
>> an entry or an exit, you decide." (just like SYSCALL_SIGTRAP), and then
>> we'd leave GDB to decide. GDB will end up fetching registers
>> off the target anyway, so I think I'd end up costing 0 in terms of
>> performance.
>>
>> Then when we have real PTRACE_EVENT_SYSCALL(_ENTER|_EXIT), we'd
>> use syscall_entry/syscall_exit.
>>
>> (not completely sure about this).
> Not sure also. Adding this in the protocol before we know it is needed
> is not ideal. However, it looks to me that not adding it now means
> that a GDB supporting only syscall enter/exit will have a problem
> with a newer gdbserver that would give back the 3rd reason.
>
>
>>
>> How portable is this -ENOSYS thing? What does strace do?
> -ENOSYS is documented in man ptrace, but only for x86.
> strace uses -ENOSYS for x86, and (from what I understood reading
> the code) cannot differentiate entry/exit (in all cases) for other
> archs.
> I suspect strace uses for these archs a flip/flop which might
> suffer from the same bug/weakness as the native GDB flip/flop.
It'd be good to confirm that. Perhaps reach out to the strace
developers/list asking for their opinion on which scheme to
use, etc.? We shouldn't be guessing this stuff really. They
have experience we should be able to leverage.
>
>>
>>
>> BTW, it'd be very very nice if GDB and GDBserver didn't end
>> up with different mechanisms for this. We've been trying to
>> make the gdb and gdbserver backends more similar, with the end
>> goal of merging them, and more points of divergence don't really
>> help that.
> Yes, I agree. I was intending to look more in depth to that after
> the RSP patch.
As we're seeing, these issues interfere with how we end up
designing the the packets. IMO, we should really take a look
at the GDB bug before finalizing the RSP side. We can go back
and fix bugs easily, but we can't go back and change the
RSP as easily...
>
>>> +# 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
>>> +}
>>
>> This isn't strictly correct, as it's quite concievable that
>> e.g., a non-x86 valgrind (or some other random target) gets
>> support for these packets before our own corresponding gdbserver
>> port gets it. Ideally we'd probe for support somehow, like by
>> looking for some GDB error/warning in the first "catch syscall", or
>> some such.
> Effectively, at Valgrind side, it will be supported for all
> archs out of the box (as Valgrind has already an arch independent
> syscall interception, and properly identifies entry from exit).
> However, up to now, I never dared to run the gdb testsuite with
> Valgrind gdbserver.
Eh, perhaps you should. ;-)
>> - I think that if you set a catch syscall, and then disconnect
>> in a way that gdbserver leaves the target running (disconnected
>> tracing, or if we have any persistent commands, see server.c), we'll end up
>> with the program held stopped as soon as the program calls a
>> syscall. I think at least handle_target_event should be updated.
> I will need to dig into that.
>>
>> - linux-nat.c:linux_handle_syscall_trap has some hairy stuff
>> when "stopping" is true, due to syscall restarts. I was
>> a little surprised that the gdbserver patch didn't need
>> anything of the sort, but then again, I don't recall whether
>> that bits is covered by the testsuite.
>> I'm wondering whether this -ENOSYS mechanism would allow
>> getting rid of all that, of if something is missing on the
>> gdbserver side.
> I believe the hairy stuff for native is to implement the flip/flop
> logic, but this logic is broken when enabling/disabling.
It may be a simple bug. Until we analyze it more deeply, we
don't know.
> -ENOSYS does not suffer from this problem (see above), but is
> arch dependent.
> So, I believe the current x86 gdbserver -ENOSYS is better/simpler,
> but for other archs, it seems such tricks do not exist or are
> not working as well (e.g. ppc64).
>
> I am not sure how to continue with this problem of differentiating
> entry/exit, together with the GDB 7.7 deadline.
>
> I see 3 approaches:
> 1. implement the QCatchSyscalls packet as it is now
> (with only entry/exit), and -ENOSYS for x86,
> and see later what to do better for other archs or native GDB.
> 2. Same as 1, but we add the 3rd stop reasons
> syscall_no_idea_if_it_is_an_entry_or_return
> so as to have the protocol ready for archs less easy than x86.
> 3. work longer on the patch, first trying to have a proper
> way to differentiate entry/return in both native GDB and
> gdbserver, and preferrably arch independent.
> (unclear if this is possible. If not, we fallback to
> option 2 or 1).
> (or possibly, the bug in the GDB flip/flop is easy to fix ?
> then the flip/flop should be used by gdbserver also)
Argh, things look backwards to me when time-based releases get
delayed for features. It's supposed to be the opposite. :-/ Perhaps
Sergio or I can try to look at the bug, though I already have a huge
backlog of distractions, so I shouldn't promise anything...
> I guess 1 and 2 could be done on time for GDB 7.7, but I will not have
> enough evening/WE time to finish 3 in let's say the next two weeks
> (there is soon a valgrind 3.9.0 to go out, for which I have a few
> things still to do).
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-04 4:22 ` Luis Machado
@ 2013-10-04 17:40 ` Pedro Alves
2013-10-04 18:55 ` Stan Shebs
0 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2013-10-04 17:40 UTC (permalink / raw)
To: lgustavo; +Cc: Philippe Waroquiers, Sergio Durigan Junior, gdb-patches
On 10/04/2013 05:22 AM, Luis Machado wrote:
> On 09/29/2013 12:04 PM, Philippe Waroquiers wrote:
>> 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.
>
> I'm late to the party, but i've always wondered why we have all these
> different "insert_<foo>_catchpoint" and "remove_<foo>_catchpoint"
> functions to accomplish tasks that seem to be very similar in nature.
>
> Not saying we should go this route for this patch, but we may want to
> consider a more generic RSP packet for catchpoints. Something like the
> following:
>
> QInsertCatchpoint:[syscall|fork|exec|vfork|unload|...]
> QRemoveCatchpoint:[syscall|fork|exec|vfork|unload|...]
>
> ... or even communicate catchpoints through Z/z packets, though that
> would be a more radical approach.
You know, you actually have a very good point. It actually looks
unfortunate to come up with new packets that don't incorporate
all the nice new features we've added to the Z/z packets recently,
such as target side conditions and commands.
The issue I see is that syscall (and other catchpoints) have
arguments. What would you pass to QRemoveCatchpoint to remove
a previous catchpoint? Sounds like QInsertCatchpoint would need
to return a unique target-side identifier, that QRemoveCatchpoint
would then use? There's also the issue with the fact that
for Z packets, the RSP specifies that a second Z packet seen for
the same address replaces the previous packet, because it might
have happened that GDB and the server lost sync for a bit, and the
second packet was actually a retransmission. Making QInsertCatchpoint
return a reference conflicts with that. Unless perhaps we make GDB
send a unique id along as well... I think the RSP used to always send
a sequence number with each packet, and that has been removed a long
time ago. I wish I know why it was removed. It would solve these
issues. Maybe we should add it back.
> Anyway, just throwing a few ideas since i've been dealing with some of
> the issues with catchpoints, forking and gdbserver as well.
Yeah.
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-03 19:53 ` Tom Tromey
@ 2013-10-04 17:41 ` Pedro Alves
0 siblings, 0 replies; 30+ messages in thread
From: Pedro Alves @ 2013-10-04 17:41 UTC (permalink / raw)
To: Tom Tromey; +Cc: Philippe Waroquiers, Sergio Durigan Junior, gdb-patches
On 10/03/2013 08:53 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
>>> +QCatchSyscalls:1 [;SYSNO]...
>>> +QCatchSyscalls:0
>>> + Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
>>> + catching syscalls from the inferior process.
>
> Pedro> So, "catch syscall" is per-inferior/process on the GDB side, but
> Pedro> this always sets the catchpoints on all processes. Was that
> Pedro> intended?
>
> I wonder whether it is the right thing on the gdb side.
>
> Right now we have the rule that linespecs for breakpoints apply to all
> inferiors; but this rule isn't followed for catchpoints.
Yeah.
> I tend to think it ought to be, for consistency and simplicity; followed
> up by using it{etc}sets for filtering out uninteresting events.
Yeah. That was part of the reason I just asked it is was intended,
instead of requesting to make it per-process.
>
>
> I don't want to derail this patch though.
>
> And arguably it is ok for gdb to present one thing to the user but more
> useful for gdbserver to present a different view to gdb.
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-04 17:40 ` Pedro Alves
@ 2013-10-04 18:55 ` Stan Shebs
2013-10-07 19:07 ` Pedro Alves
0 siblings, 1 reply; 30+ messages in thread
From: Stan Shebs @ 2013-10-04 18:55 UTC (permalink / raw)
To: gdb-patches
On 10/4/13 10:40 AM, Pedro Alves wrote:
> [...] Unless perhaps we make GDB
> send a unique id along as well... I think the RSP used to always send
> a sequence number with each packet, and that has been removed a long
> time ago. I wish I know why it was removed. It would solve these
> issues. Maybe we should add it back.
I researched this a bit, since I had the same vague memory, but the
situation is odd. In 1994, Jim Kingdon documented a $cc:<restofpacket>
at the top of remote.c, as an option that stubs accept, and to this
day the example stubs have their bit of code, but I don't see any
evidence that anything was done in GDB before or since. (The rationale
may simply have been that the stubs needed to be extended first, this
being before the days of qSupported.)
Sequence numbers don't seem like a great idea, though, potentially
introducing brittleness of various sorts. Although we didn't really
think about it explicitly at the outset, the remote protocol has
generally evolved in the direction of being stateless; for instance,
we don't send a half-dozen vCont packets that must all be applied
together, we send one packet that includes everything.
Philosophically, this makes sense because real targets are quite
unpredictable - two reads in a row can return different results,
a single-step can result in a dozen new threads, etc.
Another well-known stateless protocol is HTTP, needed due to its
own unpredictabilities (as we all experience every day :-) ). Now
there are situations where statefulness is useful, which they solve
by using cookies. In the remote protocol, our analogous situation
is where the target needs to be more like an agent, such as with
tracing, and in those cases we do have identifiers that are
coordinated between host and target, though in a somewhat adhoc
way right now.
I think we could develop the id concept into more of a generic mechanism
that is available for target-side commands, actionpoints in general,
and so forth, perhaps with everything using a single numeric (or
textual?) space, so that "231" can be unambiguously a catchpoint,
rather than either a catchpoint or a trace state variable, depending
on context.
Stan
stan@codesourcery.com
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-04 17:29 ` Pedro Alves
@ 2013-10-05 9:15 ` Pedro Alves
2013-10-09 21:54 ` Philippe Waroquiers
0 siblings, 1 reply; 30+ messages in thread
From: Pedro Alves @ 2013-10-05 9:15 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Sergio Durigan Junior, gdb-patches
Hi,
I don't have time right now for a more elaborate answer, but ...
On 10/04/2013 06:29 PM, Pedro Alves wrote:
>
> Thinking only in terms of multi-process/thread, I'm inclined to ignore the
> "per-process" thing now, and just leave it as gdbserver making catch
> syscall apply to all processes.
I just realized that we really can't do that. The syscall numbers
sent across the wire are the target-specific numbers. Since gdbserver
might well be debugging processes of different gdbarch's simultaneously
(see the multi-arch support patches from a while ago, the tdesc support
in gdbserver, etc.), we can't assume the same syscall array makes sense
for all processes under gdbserver's control.
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-04 18:55 ` Stan Shebs
@ 2013-10-07 19:07 ` Pedro Alves
0 siblings, 0 replies; 30+ messages in thread
From: Pedro Alves @ 2013-10-07 19:07 UTC (permalink / raw)
To: Stan Shebs; +Cc: gdb-patches
On 10/04/2013 07:55 PM, Stan Shebs wrote:
> On 10/4/13 10:40 AM, Pedro Alves wrote:
>
>> [...] Unless perhaps we make GDB
>> send a unique id along as well... I think the RSP used to always send
>> a sequence number with each packet, and that has been removed a long
>> time ago. I wish I know why it was removed. It would solve these
>> issues. Maybe we should add it back.
>
> I researched this a bit, since I had the same vague memory, but the
> situation is odd. In 1994, Jim Kingdon documented a $cc:<restofpacket>
> at the top of remote.c, as an option that stubs accept, and to this
> day the example stubs have their bit of code, but I don't see any
> evidence that anything was done in GDB before or since. (The rationale
> may simply have been that the stubs needed to be extended first, this
> being before the days of qSupported.)
Indeed, in gdb 4.13:
/* Remote communication protocol.
A debug packet whose contents are <data>
is encapsulated for transmission in the form:
$ <data> # CSUM1 CSUM2
<data> must be ASCII alphanumeric and cannot include characters
'$' or '#'. If <data> starts with two characters followed by
':', then the existing stubs interpret this as a sequence number.
CSUM1 and CSUM2 are ascii hex representation of an 8-bit
checksum of <data>, the most significant nibble is sent first.
the hex digits 0-9,a-f are used.
>
> Sequence numbers don't seem like a great idea, though, potentially
> introducing brittleness of various sorts.
Hmm, not sure what brittleness would that be? The RSP, with its
ack/nak and retransmission support, thinking in network protocol
terms, is really an application layer protocol that also tries to be
a transport layer protocol. But then I'd say it's not really good
at either. :-) The way the ack/nak/timeout process is designed,
if RSP is being pushed on top of an unreliable network protocol/link
(like serial/RS232, or udp), then it sometimes happens that GDB
and the server get out of sync. A sequence number is just the
basic thing that reliable transport protocols (such as TCP) have
to sort out that issue, so that the receiver of an ack can tell
what is being acked. Currently, if acks are lost, havoc
usually ensues. The implemention in 1994 does look too naive though.
It just feels to me that the RSP should either have had ack/naks
and retransmissions + sequence numbers, or none of all that at all.
> Although we didn't really
> think about it explicitly at the outset, the remote protocol has
> generally evolved in the direction of being stateless;
I think it's useful to consider the transport and application
layers independently here, even though the RSP really mixes/messes
thing up there. If we think at the transport level, if GDB can
assume the transport is reliable, it'll disable its own transport
layer features -- that is, the ack/naks. That is the QStartNoAckMode
stuff (which gdbserver makes use of, when the connection is through
tcp/ip, but not when it's serial). This was something that was added
not that long ago.
Now, if we move on past that to the RSP commands/packets
themselves, we start thinking in terms of application layer.
At that conceptual level, both client and server should
assume that packets always get to the other side in sequence.
GDB should be able to be sure that the "OK" it just pulled
out of the packet stream is the reply to the command it just
sent, not for the previous command that happened to fail.
Today, in scenarios around packet loss, it can't.
As a result of the lame ack/retransmission design, any
problem at the communication level needs to result in force closing
the connection. If the user wants to keep debugging, she
should reconnect. The disconnect might well delete useful state
from GDB that might be painful to re-setup.
> for instance,
> we don't send a half-dozen vCont packets that must all be applied
> together, we send one packet that includes everything.
True that we've been evolving in that direction, though we have
several packets that aren't like that.
For example qfThreadInfo/qmThreadInfo (get first thread; get next thread;
get next thread; etc.). The server needs to know which thread to
return next. Another example is downloading tracepoints. We first
create the tracepoint (QTDP:...) , and then augment the tracepoint
with its actions, one by one, by sending (QTDP:-...).
Or even for more recent examples, how sometimes when we
fetch an xml off the target, it needs to first take a snapshot
of whatever it's going to return when GDB first reads offset 0,
and then subsequent reads are served from that cached snapshot.
E.g., gdbserver/server.c:handle_qxfer_threads.
I don't see a conceptual issue with preparing a request
in chunks, and then issuing a final "commit", if it makes sense
for the particulars of the action being performed.
The main reasons we give when designing packets and pushing in the
direction of sending one big packet, are simplicity, and minimizing
roundtrips. The reverse side of the coin, is that then we need
to care whether we're overflowing the maximum packet buffer size.
Actually, the packet proposed in this thread has code exactly for
that, because a request for catching syscalls
lists all the syscalls GDB is interested in.
But, this is talking about the semantics of the commands/packets,
IOW, talking at a higher conceptual level than the acks/naks or
sequence numbers.
My main complain here is with the bad mixup of transport and application
layers, which ends up manifesting in things like the Z/z packets having
this requirement:
"To avoid potential problems with duplicate packets, the operations
should be implemented in an idempotent way."
(that's talking about Z vs z).
And this meant we ended up e.g., merging all multiple breakpoints
with target-assisted conditions, and breakpoints with target-side
commands all as a single breakpoint on the target, as long as their
at the same address.
[And then that's the root cause of breakpoints/15180.
If we try extending the Z packets to support target-side
thread-specific breakpoint handling, such merging by address gets
even less clearly a good idea to me.]
With all that in mind, and given we do have plenty of sequences
in the protocol that do require state, let me clarify my original
concern:
> There's also the issue with the fact that
> for Z packets, the RSP specifies that a second Z packet seen for
> the same address replaces the previous packet, because it might
> have happened that GDB and the server lost sync for a bit, and the
> second packet was actually a retransmission. Making QInsertCatchpoint
> return a reference conflicts with that.
... by saying that IMO, this "potential problems with duplicate packets"
concern in the Z/z packets is IMO solved in the wrong place, and
that issue IMO should not prevent coming up with a breakpoint/catchpoint
packet that returns a target-generated token/reference/handle for
the created resource. A better solution for that would IMO be done
at the "transport" side of the protocol (at the level of ack/naks),
and that probably would mean introducing sequence numbers.
> Another well-known stateless protocol is HTTP, needed due to its
> own unpredictabilities (as we all experience every day :-) ). Now
> there are situations where statefulness is useful, which they solve
> by using cookies. In the remote protocol, our analogous situation
> is where the target needs to be more like an agent, such as with
> tracing, and in those cases we do have identifiers that are
> coordinated between host and target, though in a somewhat adhoc
> way right now.
Right.
> I think we could develop the id concept into more of a generic mechanism
> that is available for target-side commands, actionpoints in general,
> and so forth, perhaps with everything using a single numeric (or
> textual?) space, so that "231" can be unambiguously a catchpoint,
> rather than either a catchpoint or a trace state variable, depending
> on context.
Yeah, good idea. That might came in handy.
--
Pedro Alves
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-05 9:15 ` Pedro Alves
@ 2013-10-09 21:54 ` Philippe Waroquiers
2013-10-09 22:05 ` Sergio Durigan Junior
0 siblings, 1 reply; 30+ messages in thread
From: Philippe Waroquiers @ 2013-10-09 21:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: Sergio Durigan Junior, gdb-patches
On Sat, 2013-10-05 at 10:15 +0100, Pedro Alves wrote:
> Hi,
>
> I don't have time right now for a more elaborate answer, but ...
>
> On 10/04/2013 06:29 PM, Pedro Alves wrote:
> >
> > Thinking only in terms of multi-process/thread, I'm inclined to ignore the
> > "per-process" thing now, and just leave it as gdbserver making catch
> > syscall apply to all processes.
>
> I just realized that we really can't do that. The syscall numbers
> sent across the wire are the target-specific numbers. Since gdbserver
> might well be debugging processes of different gdbarch's simultaneously
> (see the multi-arch support patches from a while ago, the tdesc support
> in gdbserver, etc.), we can't assume the same syscall array makes sense
> for all processes under gdbserver's control.
>
Hello,
Getting back to this after a period of other activitites,
here is a suggested list of approaches
to address the major comments given in various mails:
* QCatchSyscalls contains target specific numbers (this is the
above comment)
=> have gdbserver handling QCatchSyscalls packet per inferior
* ensure QCatchSyscall packet can (in the future) be extended with
a COND_LIST (similar to the Z packets).
To do that, I suggest to have the QCatchSyscalls separating syscall
numbers with a , rather than a ;
(so that a ; can be used later to separate the list of syscalls
from the COND_LIST)
Note: Luis suggested the alternative to have a packet
QInsertCatchPoint:[fork|syscall|exec|...]
Then gdbserver will tell in QSupported that it e.g. support
QInsertCatchPoint=syscall,fork
For what concerns the problem of identifying which catchpoint
to remove in the QRemoveCatchPoint: not too sure we need
an catch point id for that. We can assume that an QInsertCatchPoint
of a certain kind fully replace the previously inserted catchpoint
of the same kind. A QRemoveCatchpoint removes completely
the catchpoint of the same kind.
I can go the QInsertCatchPoint way if it is confirmed this is a better
approach.
* Need to investigate the bug in gdb 'catch syscall' flip/flop logic.
If this logic can be fixed, then have gdbserver and gdb using
the same logic.
* extend the stop reply packet to allow to return a
"syscall" stop reason that does not specify if this is a syscall
entry or exit.
I suggest to do this even if a correct flip/flop logic can be
found during the previous investigation.
This 3rd syscall stop reason allow stubs to report a syscall
without necessarily having the logic to differentiate entry
from return.
Any comments about the above approaches ?
(in particular, about the choice between QCatchSyscalls
and QInsertCatchPoint).
Thanks
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-09 21:54 ` Philippe Waroquiers
@ 2013-10-09 22:05 ` Sergio Durigan Junior
2013-10-09 22:09 ` Philippe Waroquiers
0 siblings, 1 reply; 30+ messages in thread
From: Sergio Durigan Junior @ 2013-10-09 22:05 UTC (permalink / raw)
To: Philippe Waroquiers; +Cc: Pedro Alves, gdb-patches
On Wednesday, October 09 2013, Philippe Waroquiers wrote:
> * Need to investigate the bug in gdb 'catch syscall' flip/flop logic.
> If this logic can be fixed, then have gdbserver and gdb using
> the same logic.
Just FYI, I started a weekend-project to implement the
PTRACE_O_SYSCALL_{ENTER,EXIT} ideas from our wishlist :-). I already
have a patch working, but I need to give it some love before posting
it...
--
Sergio
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)
2013-10-09 22:05 ` Sergio Durigan Junior
@ 2013-10-09 22:09 ` Philippe Waroquiers
0 siblings, 0 replies; 30+ messages in thread
From: Philippe Waroquiers @ 2013-10-09 22:09 UTC (permalink / raw)
To: Sergio Durigan Junior; +Cc: Pedro Alves, gdb-patches
On Wed, 2013-10-09 at 19:05 -0300, Sergio Durigan Junior wrote:
> On Wednesday, October 09 2013, Philippe Waroquiers wrote:
>
> > * Need to investigate the bug in gdb 'catch syscall' flip/flop logic.
> > If this logic can be fixed, then have gdbserver and gdb using
> > the same logic.
>
> Just FYI, I started a weekend-project to implement the
> PTRACE_O_SYSCALL_{ENTER,EXIT} ideas from our wishlist :-). I already
> have a patch working, but I need to give it some love before posting
> it...
That is nice.
But I guess we still need gdb to work with (old/current) kernels
that will not provide this functionality (and/or with stubs
for non linux based OS that will never benefit from this).
Philippe
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2013-10-09 22:09 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-21 20:55 RFA [PATCH v3] Implement 'catch syscall' for gdbserver Philippe Waroquiers
2013-09-21 21:03 ` Eli Zaretskii
2013-09-23 11:51 ` Agovic, Sanimir
2013-09-23 19:32 ` Philippe Waroquiers
2013-09-24 7:07 ` Agovic, Sanimir
2013-09-25 16:55 ` Sergio Durigan Junior
2013-09-25 22:55 ` Philippe Waroquiers
2013-09-27 13:25 ` [COMMIT PATCH] remote.c: Remove unnecessary fields from 'struct stop_reply'. (was: Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
2013-09-27 19:30 ` RFA [PATCH v3] Implement 'catch syscall' for gdbserver Pedro Alves
2013-09-27 20:13 ` Philippe Waroquiers
2013-09-27 20:55 ` Sergio Durigan Junior
2013-09-29 15:04 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Philippe Waroquiers
2013-10-01 5:16 ` Sergio Durigan Junior
2013-10-02 21:02 ` Sergio Durigan Junior
2013-10-02 19:41 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported. (was: Re: RFA [PATCH v4] " Pedro Alves
2013-10-02 22:08 ` Philippe Waroquiers
2013-10-03 10:16 ` Always run the PTRACE_O_TRACESYSGOOD tests even if PTRACE_O_TRACEFORK is not supported Pedro Alves
2013-10-03 18:40 ` RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver) Pedro Alves
2013-10-03 19:53 ` Tom Tromey
2013-10-04 17:41 ` Pedro Alves
2013-10-03 22:02 ` Philippe Waroquiers
2013-10-04 17:29 ` Pedro Alves
2013-10-05 9:15 ` Pedro Alves
2013-10-09 21:54 ` Philippe Waroquiers
2013-10-09 22:05 ` Sergio Durigan Junior
2013-10-09 22:09 ` Philippe Waroquiers
2013-10-04 4:22 ` Luis Machado
2013-10-04 17:40 ` Pedro Alves
2013-10-04 18:55 ` Stan Shebs
2013-10-07 19:07 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).