RFA [PATCH] Implement 'catch syscall' for gdbserver

RFA [PATCH] Implement 'catch syscall' for gdbserver

[Philippe Waroquiers]

This is the 2nd version of the patch implementing 'catch syscalls' for
gdbserver.
First version was sent in an RFC (no feedback yet, so here is a completed
and tested RFA ready version).
Tested (no regression) on linux amd64, native and gdbserver.
Manually tested with a patched Valgrind gdbserver.

Here are the changes logs:
ChangeLog
2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* NEWS: Document new QcatchSyscalls packet and its use
	in x86/amd64 linux gdbserver and Valgrind gdbserver.
	* remote.c (PACKET_QCatchSyscalls): New.
	(remote_protocol_features): Add Qcatchsyscalls.
	(remote_set_syscall_catchpoint): New function.
	(remote_parse_stop_reply): New stop reasons syscall_entry
	and syscall_return.
	(init_remote_ops): Registers remote_set_syscall_catchpoint
	and the config commands for [PACKET_QCatchSyscalls.
	* 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): Enables, detects and handle
	SYSCALL_SIGTRAP.
	(gdb_catched_syscall): 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 catch_syscalls_p, catched_syscalls_size and
	catched_syscalls.
	* server.c: Define catch_syscalls_p, catched_syscalls_size and
	catched_syscalls.
	(handle_general_set): Handle QCatchSyscalls packet.
	(handle_query): Reports if low target supports QCatchSyscalls.
	* win32-low.c (struct target_ops win32_target_op): Sets NULL
	for supports_catch_syscall.

testsuite/ChangeLog
2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.base/catch-syscall.exp: Enables test for x86 and amd64
	gdbserver.

Two specific points that might be worth looking at (after this patch
probably):

1. GDB native has a bug in the detection  of "syscall entry/return",
   when the catch syscall is disabled when inferior is stopped
   on a syscall entry, and then catch syscall is re-enabled later:

  Catchpoint 1 (call to syscall brk), 0x00207ead in brk ()
     from /lib/ld-linux.so.2
  (gdb) break main
  Breakpoint 2 at 0x80486ca: file reach_thread_register.c, line 42.
  (gdb) disa 1
  (gdb) c
  Continuing.
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/libthread_db.so.1".

  Breakpoint 2, main () at reach_thread_register.c:42
  42      pthread_barrier_init(&bar, NULL, 2);
  (gdb) enable
  (gdb) c
  Continuing.

  Catchpoint 1 (returned from syscall mmap2), 0x00121416 in __kernel_vsyscall ()
  (gdb)         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ should be call to syscall mmap2
/// after this, all entries are seen as returns, and all returns are seen
/// as entries.

In GDBSERVER, to differentiate entry from return, I use the
syscall retcode -ENOSYS.
This is working properly including for the case above.
The only problem is that if a syscall is not implemented
(and so really returns -ENOSYS)
then GDBSERVER will wrongly report the return of the system call
as an entry (but subsequent syscall entries/returns will be properly
reported).
Maybe there is a better approach, including for fixing
the  'GDB native catch syscall' ?

2. GDB (probably in breakpoint.c) has a bug with catch syscalls
   in remote mode when 'set breakpoint always-inserted on' :
   Disabling all the catch syscalls does not cause
   a call to target_set_syscall_catchpoint with needed = 0,
   which means that QCatchSyscalls:0 is not sent.
   After that, GDBSERVER continues to report various syscalls
   to GDB, that filters them, so no functional bug, only
   efficiency bug.

----------------- Patch:
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.611
diff -u -p -r1.611 NEWS
--- NEWS	27 Aug 2013 05:20:56 -0000	1.611
+++ NEWS	30 Aug 2013 15:18:08 -0000
@@ -127,11 +127,21 @@ qXfer:libraries-svr4:read's annex
   necessary for library list updating, resulting in significant
   speedup.
 
+QCatchSyscalls:1 [;SYSNO]...
+QCatchSyscalls:0
+  Enable (`QCatchSyscalls:1') or disable (`QCatchSyscalls:0')
+  catching syscalls from the inferior process.
+
+
 * New features in the GDB remote stub, GDBserver
 
   ** GDBserver now supports target-assisted range stepping.  Currently
      enabled on x86/x86_64 GNU/Linux targets.
 
+  ** GDBserver now supports catch syscall catchpoints.  Currently
+     enabled on x86/x86_64 GNU/Linux targets, and in the Valgrind
+     gdbserver.
+
   ** GDBserver now adds element 'tvar' in the XML in the reply to
      'qXfer:traceframe-info:read'.  It has the id of the collected
      trace state variables.
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.577
diff -u -p -r1.577 remote.c
--- remote.c	23 Aug 2013 13:12:17 -0000	1.577
+++ remote.c	30 Aug 2013 15:18:09 -0000
@@ -1340,6 +1340,7 @@ enum {
   PACKET_qSupported,
   PACKET_qTStatus,
   PACKET_QPassSignals,
+  PACKET_QCatchSyscalls,
   PACKET_QProgramSignals,
   PACKET_qSearch_memory,
   PACKET_vAttach,
@@ -1728,6 +1729,93 @@ remote_pass_signals (int numsigs, unsign
     }
 }
 
+/* If 'QCatchSyscalls' is supported, tell the remote stub
+   to report syscalls to GDB.  */
+
+static int
+remote_set_syscall_catchpoint (int pid, int needed, int any_count,
+			       int table_size, int *table)
+{
+  if (remote_protocol_packets[PACKET_QCatchSyscalls].support != PACKET_DISABLE)
+    {
+      char *catch_packet, *p;
+      enum packet_result result;
+      int n_sysno = 0;
+
+      if (needed && !any_count)
+	{
+	  int i;
+
+	  for (i = 0; i < table_size; i++)
+	    if (table[i])
+	      n_sysno++;
+	}
+
+      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);
+	      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;
+      }
+    }
+  else
+    return 1; /* not supported */
+}
+
+
 /* If 'QProgramSignals' is supported, tell the remote stub what
    signals it should pass through to the inferior when detaching.  */
 
@@ -4016,6 +4104,8 @@ static const struct protocol_feature rem
     PACKET_qXfer_traceframe_info },
   { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
     PACKET_QPassSignals },
+  { "QCatchSyscalls", PACKET_DISABLE, remote_supported_packet,
+    PACKET_QCatchSyscalls },
   { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
     PACKET_QProgramSignals },
   { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
@@ -5283,7 +5373,8 @@ typedef struct stop_reply
 
   int stopped_by_watchpoint_p;
   CORE_ADDR watch_data_address;
-
+  
+  int syscall;
   int solibs_changed;
   int replay_event;
 
@@ -5546,6 +5637,7 @@ remote_parse_stop_reply (char *buf, stru
   event->ptid = null_ptid;
   event->ws.kind = TARGET_WAITKIND_IGNORE;
   event->ws.value.integer = 0;
+  event->syscall = 0;
   event->solibs_changed = 0;
   event->replay_event = 0;
   event->stopped_by_watchpoint_p = 0;
@@ -5596,6 +5688,22 @@ Packet: '%s'\n"),
 		       p, buf);
 	      if (strncmp (p, "thread", p1 - p) == 0)
 		event->ptid = read_ptid (++p1, &p);
+	      else if (strncmp (p, "syscall_entry", p1 - p) == 0)
+		{
+		  ULONGEST sysno;
+		  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 +5789,11 @@ Packet: '%s'\n"),
 	event->ws.kind = TARGET_WAITKIND_LOADED;
       else if (event->replay_event)
 	event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
+      else if (event->syscall)
+	{
+	  gdb_assert (event->ws.kind == TARGET_WAITKIND_SYSCALL_ENTRY
+		      || event->ws.kind == TARGET_WAITKIND_SYSCALL_RETURN);
+	}
       else
 	{
 	  event->ws.kind = TARGET_WAITKIND_STOPPED;
@@ -11452,6 +11565,7 @@ Specify the serial device it is connecte
   remote_ops.to_load = generic_load;
   remote_ops.to_mourn_inferior = remote_mourn;
   remote_ops.to_pass_signals = remote_pass_signals;
+  remote_ops.to_set_syscall_catchpoint = remote_set_syscall_catchpoint;
   remote_ops.to_program_signals = remote_program_signals;
   remote_ops.to_thread_alive = remote_thread_alive;
   remote_ops.to_find_new_threads = remote_threads_info;
@@ -11946,6 +12060,9 @@ Show the maximum size of the address (in
   add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
 			 "QPassSignals", "pass-signals", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_QCatchSyscalls],
+			 "QCatchSyscalls", "catch-syscalls", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
 			 "QProgramSignals", "program-signals", 0);
 
Index: common/linux-ptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
retrieving revision 1.12
diff -u -p -r1.12 linux-ptrace.c
--- common/linux-ptrace.c	28 Aug 2013 14:09:31 -0000	1.12
+++ common/linux-ptrace.c	30 Aug 2013 15:18:09 -0000
@@ -361,16 +361,17 @@ linux_check_ptrace_features (void)
       return;
     }
 
-#ifdef GDBSERVER
-  /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
-     PTRACE_O_TRACEVFORKDONE yet.  */
-#else
-  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
+  /* Check if the target supports PTRACE_O_TRACESYSGOOD, keeping
+     PTRACE_O_TRACEFORK option activated.  */
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
-		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+				    | PTRACE_O_TRACESYSGOOD));
   if (ret == 0)
     current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
 
+#ifdef GDBSERVER
+  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
+#else
   /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
   ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
 		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
@@ -474,6 +475,11 @@ linux_enable_event_reporting (pid_t pid)
 static int
 ptrace_supports_feature (int ptrace_options)
 {
+  /* Check if we have initialized the ptrace features for this
+     target.  If not, do it now.  */
+  if (current_ptrace_options == -1)
+    linux_check_ptrace_features ();
+
   gdb_assert (current_ptrace_options >= 0);
 
   return ((current_ptrace_options & ptrace_options) == ptrace_options);
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.1105
diff -u -p -r1.1105 gdb.texinfo
--- doc/gdb.texinfo	27 Aug 2013 05:20:57 -0000	1.1105
+++ doc/gdb.texinfo	30 Aug 2013 15:18:15 -0000
@@ -18681,6 +18681,10 @@ are:
 @tab @code{qSupported}
 @tab Remote communications parameters
 
+@item @code{catch-syscalls}
+@tab @code{QCatchSyscalls}
+@tab @code{catch syscall}
+
 @item @code{pass-signals}
 @tab @code{QPassSignals}
 @tab @code{handle @var{signal}}
@@ -38077,6 +38081,11 @@ The currently defined stop reasons are:
 The packet indicates a watchpoint hit, and @var{r} is the data address, in
 hex.
 
+@item syscall_entry
+@itemx syscall_return
+The packet indicates a syscall entry or return, and @var{r} is the 
+syscall number, in hex.
+
 @cindex shared library events, remote reply
 @item library
 The packet indicates that the loaded libraries have changed.
@@ -38447,6 +38456,44 @@ by supplying an appropriate @samp{qSuppo
 Use of this packet is controlled by the @code{set non-stop} command; 
 @pxref{Non-Stop Mode}.
 
+@item QCatchSyscalls:1 @r{[};@var{sysno}@r{]}@dots{}
+@itemx QCatchSyscalls:0
+@cindex catch syscalls from inferior, remote request
+@cindex @samp{QCatchSyscalls} packet
+@anchor{QCatchSyscalls}
+Enable (@samp{QCatchSyscalls:1}) or disable (@samp{QCatchSyscalls:0})
+catching syscalls from the inferior process.
+
+For @samp{QCatchSyscalls:1}, each listed syscall @var{sysno} (encoded
+in hex) should be reported to @value{GDBN}. If no syscall @var{sysno}
+is listed, every system call should be reported.
+
+Note that if a syscall not member of the list is reported, @value{GDBN}
+will filter it if this signal is not catched. It is however more efficient
+to only report the needed syscalls.
+ 
+Multiple @samp{QCatchSyscalls:1} packets do not
+combine; any earlier @samp{QCatchSyscalls:1} list is completely replaced by the
+new list.
+
+Reply:
+@table @samp
+@item OK
+The request succeeded.
+
+@item E @var{nn}
+An error occurred.  @var{nn} are hex digits.
+
+@item @w{}
+An empty reply indicates that @samp{QCatchSyscalls} is not supported by
+the stub.
+@end table
+
+Use of this packet is controlled by the @code{set remote catch-syscalls}
+command (@pxref{Remote Configuration, set remote catch-syscalls}).
+This packet is not probed by default; the remote stub must request it,
+by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
+
 @item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
 @cindex pass signals to inferior, remote request
 @cindex @samp{QPassSignals} packet
@@ -38810,6 +38857,11 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab Yes
 
+@item @samp{QCatchSyscalls}
+@tab No
+@tab @samp{-}
+@tab Yes
+
 @item @samp{QPassSignals}
 @tab No
 @tab @samp{-}
@@ -38970,6 +39022,10 @@ packet (@pxref{qXfer fdpic loadmap read}
 The remote stub understands the @samp{QNonStop} packet
 (@pxref{QNonStop}).
 
+@item QCatchSyscalls
+The remote stub understands the @samp{QCatchSyscalls} packet
+(@pxref{QCatchSyscalls}).
+
 @item QPassSignals
 The remote stub understands the @samp{QPassSignals} packet
 (@pxref{QPassSignals}).
Index: gdbserver/linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.246
diff -u -p -r1.246 linux-low.c
--- gdbserver/linux-low.c	28 Aug 2013 17:40:58 -0000	1.246
+++ gdbserver/linux-low.c	30 Aug 2013 15:18:16 -0000
@@ -72,6 +72,11 @@
 #define W_STOPCODE(sig) ((sig) << 8 | 0x7f)
 #endif
 
+/* Unlike other extended result codes, WSTOPSIG (status) on
+   PTRACE_O_TRACESYSGOOD syscall events doesn't return SIGTRAP, but
+   instead SIGTRAP with bit 7 set.  */
+#define SYSCALL_SIGTRAP (SIGTRAP | 0x80)
+
 /* This is the kernel's hard limit.  Not to be confused with
    SIGRTMIN.  */
 #ifndef __SIGRTMIN
@@ -479,6 +484,33 @@ get_pc (struct lwp_info *lwp)
   return pc;
 }
 
+/* This function should only be called if LWP got a SIGTRAP_SYSCALL.  */
+
+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;
+    }
+
+  saved_inferior = current_inferior;
+  current_inferior = get_lwp_thread (lwp);
+
+  regcache = get_thread_regcache (current_inferior, 1);
+  (*the_low_target.get_syscall_trapinfo) (regcache, sysno, sysret);
+
+  if (debug_threads)
+    fprintf (stderr, "get_syscall_trapinfo sysno %d sysret %d\n",
+	     *sysno, *sysret);
+
+  current_inferior = saved_inferior;
+}
+
 /* This function should only be called if LWP got a SIGTRAP.
    The SIGTRAP could mean several things.
 
@@ -2246,6 +2278,29 @@ linux_stabilize_threads (void)
     }
 }
 
+/* Returns 1 if GDB is interested in the event_child syscall.
+   Only to be called when stopped reason is SIGTRAP_SYSCALL.  */
+
+static int
+gdb_catched_syscall (struct lwp_info *event_child)
+{
+  int i;
+  int sysno, sysret;
+
+  if (!catch_syscalls_p)
+    return 0;
+
+  if (catched_syscalls_size == 0)
+    return 1;
+
+  get_syscall_trapinfo (event_child, &sysno, &sysret);
+  for (i = 0; i < catched_syscalls_size; i++)
+    if (catched_syscalls[i] == sysno)
+      return 1;
+
+  return 0;
+}
+
 /* Wait for process, returns status.  */
 
 static ptid_t
@@ -2527,6 +2582,19 @@ Check if we're already there.\n",
 
   /* Check whether GDB would be interested in this event.  */
 
+  /* Check if GDB is interested in this syscall.  */
+  if (WIFSTOPPED (w)
+      && WSTOPSIG (w) == SYSCALL_SIGTRAP 
+      && !gdb_catched_syscall (event_child))
+    {
+      if (debug_threads)
+	fprintf (stderr, "Ignored syscall for LWP %ld.\n",
+		 lwpid_of (event_child));
+      linux_resume_one_lwp (event_child, event_child->stepping,
+			    0, NULL);
+      goto retry;
+    }
+
   /* If GDB is not interested in this signal, don't stop other
      threads, and don't report it to GDB.  Just resume the inferior
      right away.  We do this for threading-related signals as well as
@@ -2705,7 +2773,18 @@ Check if we're already there.\n",
 
   ourstatus->kind = TARGET_WAITKIND_STOPPED;
 
-  if (current_inferior->last_resume_kind == resume_stop
+  if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
+    {
+      int sysret;
+
+      get_syscall_trapinfo (event_child,
+			    &ourstatus->value.syscall_number, &sysret);
+      if (sysret == -ENOSYS)
+	ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
+      else
+	ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;
+    }
+  else if (current_inferior->last_resume_kind == resume_stop
       && WSTOPSIG (w) == SIGSTOP)
     {
       /* A thread that has been requested to stop by GDB with vCont;t,
@@ -3267,7 +3346,8 @@ lwp %ld wants to get out of fast tracepo
   lwp->stopped = 0;
   lwp->stopped_by_watchpoint = 0;
   lwp->stepping = step;
-  ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (lwp),
+  ptrace (step ? PTRACE_SINGLESTEP : catch_syscalls_p ? PTRACE_SYSCALL : PTRACE_CONT, 
+	  lwpid_of (lwp),
 	  (PTRACE_TYPE_ARG3) 0,
 	  /* Coerce to a uintptr_t first to avoid potential gcc warning
 	     of coercing an 8 byte integer to a 4 byte pointer.  */
@@ -5106,6 +5186,13 @@ linux_process_qsupported (const char *qu
 }
 
 static int
+linux_supports_catch_syscall (void)
+{
+  return the_low_target.get_syscall_trapinfo != NULL
+    && linux_supports_tracesysgood();
+}
+
+static int
 linux_supports_tracepoints (void)
 {
   if (*the_low_target.supports_tracepoints == NULL)
@@ -5796,6 +5883,7 @@ static struct target_ops linux_target_op
   linux_common_core_of_thread,
   linux_read_loadmap,
   linux_process_qsupported,
+  linux_supports_catch_syscall,
   linux_supports_tracepoints,
   linux_read_pc,
   linux_write_pc,
Index: gdbserver/linux-low.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
retrieving revision 1.65
diff -u -p -r1.65 linux-low.h
--- gdbserver/linux-low.h	22 Aug 2013 23:46:29 -0000	1.65
+++ gdbserver/linux-low.h	30 Aug 2013 15:18:16 -0000
@@ -187,6 +187,12 @@ struct linux_target_ops
   /* Hook to support target specific qSupported.  */
   void (*process_qsupported) (const char *);
 
+  /* Fill SYSNO with the syscall nr trapped. Fill SYSRET with the
+     return code.  Only to be called when inferior is stopped
+     due to SYSCALL_SIGTRAP.  */
+  void (*get_syscall_trapinfo) (struct regcache *regcache,
+				int *sysno, int *sysret);
+
   /* Returns true if the low target supports tracepoints.  */
   int (*supports_tracepoints) (void);
 
Index: gdbserver/linux-x86-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-low.c,v
retrieving revision 1.49
diff -u -p -r1.49 linux-x86-low.c
--- gdbserver/linux-x86-low.c	12 Jun 2013 16:05:39 -0000	1.49
+++ gdbserver/linux-x86-low.c	30 Aug 2013 15:18:16 -0000
@@ -1472,6 +1472,32 @@ x86_arch_setup (void)
   current_process ()->tdesc = x86_linux_read_description ();
 }
 
+static void
+x86_get_syscall_trapinfo (struct regcache *regcache, int *sysno, int *sysret)
+{
+  int use_64bit = register_size (regcache->tdesc, 0) == 8;
+
+  if (use_64bit)
+    {
+      long l_sysno;
+      long l_sysret;
+      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)
 {
@@ -3321,6 +3347,7 @@ struct linux_target_ops the_low_target =
   x86_linux_new_thread,
   x86_linux_prepare_to_resume,
   x86_linux_process_qsupported,
+  x86_get_syscall_trapinfo,
   x86_supports_tracepoints,
   x86_get_thread_area,
   x86_install_fast_tracepoint_jump_pad,
Index: gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.98
diff -u -p -r1.98 remote-utils.c
--- gdbserver/remote-utils.c	1 Jul 2013 11:19:27 -0000	1.98
+++ gdbserver/remote-utils.c	30 Aug 2013 15:18:16 -0000
@@ -1319,12 +1319,17 @@ prepare_resume_reply (char *buf, ptid_t 
   switch (status->kind)
     {
     case TARGET_WAITKIND_STOPPED:
+    case TARGET_WAITKIND_SYSCALL_ENTRY:
+    case TARGET_WAITKIND_SYSCALL_RETURN:
       {
 	struct thread_info *saved_inferior;
 	const char **regp;
 	struct regcache *regcache;
 
-	sprintf (buf, "T%02x", status->value.sig);
+	if (status->kind == TARGET_WAITKIND_STOPPED)
+	  sprintf (buf, "T%02x", status->value.sig);
+	else
+	  sprintf (buf, "T%02x", SIGTRAP);
 	buf += strlen (buf);
 
 	saved_inferior = current_inferior;
@@ -1335,6 +1340,16 @@ prepare_resume_reply (char *buf, ptid_t 
 
 	regcache = get_thread_regcache (current_inferior, 1);
 
+	if (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
+	    || status->kind == TARGET_WAITKIND_SYSCALL_RETURN)
+	  {
+	    sprintf (buf, "%s:%x;",
+		     status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
+		     ? "syscall_entry" : "syscall_return",
+		     status->value.syscall_number);
+	    buf += strlen (buf);
+	  }
+	  
 	if (the_target->stopped_by_watchpoint != NULL
 	    && (*the_target->stopped_by_watchpoint) ())
 	  {
Index: gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.196
diff -u -p -r1.196 server.c
--- gdbserver/server.c	28 Aug 2013 17:40:58 -0000	1.196
+++ gdbserver/server.c	30 Aug 2013 15:18:17 -0000
@@ -71,6 +71,9 @@ int debug_threads;
 int debug_hw_points;
 
 int pass_signals[GDB_SIGNAL_LAST];
+int catch_syscalls_p;
+int catched_syscalls_size;
+int *catched_syscalls;
 int program_signals[GDB_SIGNAL_LAST];
 int program_signals_p;
 
@@ -507,6 +510,46 @@ handle_general_set (char *own_buf)
       return;
     }
 
+  if (strncmp ("QCatchSyscalls:1", own_buf, strlen ("QCatchSyscalls:1")) == 0)
+    {
+      int i;
+      const char *p;
+      CORE_ADDR sysno;
+
+      catch_syscalls_p = 1;
+      if (catched_syscalls != NULL)
+	{
+	  free (catched_syscalls);
+	  catched_syscalls = NULL;
+	}
+      catched_syscalls_size = 0;
+      p = own_buf + strlen("QCatchSyscalls:1");
+      while (*p)
+	{
+	  if (*p++ == ';')
+	    catched_syscalls_size++;
+	}
+      if (catched_syscalls_size > 0)
+	{
+	  catched_syscalls = xmalloc (catched_syscalls_size * sizeof (int));
+	  p = strchr(own_buf, ';') + 1;
+	  for (i = 0; i < catched_syscalls_size; i++)
+	    {
+	      p = decode_address_to_semicolon (&sysno, p);
+	      catched_syscalls [i] = (int) sysno;
+	    }
+	}
+      strcpy (own_buf, "OK");
+      return;
+    }
+
+  if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
+    {
+      catch_syscalls_p = 0;
+      strcpy (own_buf, "OK");
+      return;
+    }
+
   if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
     {
       int numsigs = (int) GDB_SIGNAL_LAST, i;
@@ -1740,6 +1783,9 @@ handle_query (char *own_buf, int packet_
 	       "PacketSize=%x;QPassSignals+;QProgramSignals+",
 	       PBUFSIZ - 1);
 
+      if (target_supports_catch_syscall())
+	strcat (own_buf, ";QCatchSyscalls+");
+
       if (the_target->qxfer_libraries_svr4 != NULL)
 	strcat (own_buf, ";qXfer:libraries-svr4:read+"
 		";augmented-libraries-svr4-read+");
Index: gdbserver/server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.109
diff -u -p -r1.109 server.h
--- gdbserver/server.h	1 Jul 2013 11:28:30 -0000	1.109
+++ gdbserver/server.h	30 Aug 2013 15:18:17 -0000
@@ -230,6 +230,15 @@ extern int server_waiting;
 extern int debug_threads;
 extern int debug_hw_points;
 extern int pass_signals[];
+
+/* 1 if some (or all) syscalls are catched. */
+extern int catch_syscalls_p;
+/* catched_syscalls is the list of syscalls to report to GDB.
+   If catch_syscalls_p and catched_syscalls == NULL, it means
+   all syscalls must be reported.  */
+extern int catched_syscalls_size;
+extern int *catched_syscalls;
+
 extern int program_signals[];
 extern int program_signals_p;
 
Index: gdbserver/target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.70
diff -u -p -r1.70 target.h
--- gdbserver/target.h	19 Aug 2013 16:54:11 -0000	1.70
+++ gdbserver/target.h	30 Aug 2013 15:18:17 -0000
@@ -267,6 +267,10 @@ struct target_ops
   /* Target specific qSupported support.  */
   void (*process_qsupported) (const char *);
 
+  /* Return 1 if the target supports catch syscall, 0 (or leave the
+     callback NULL) otherwise.  */
+  int (*supports_catch_syscall) (void);
+
   /* Return 1 if the target supports tracepoints, 0 (or leave the
      callback NULL) otherwise.  */
   int (*supports_tracepoints) (void);
@@ -413,6 +417,10 @@ int kill_inferior (int);
 	the_target->process_qsupported (query);		\
     } while (0)
 
+#define target_supports_catch_syscall()              	\
+  (the_target->supports_catch_syscall ?			\
+   (*the_target->supports_catch_syscall) () : 0)
+
 #define target_supports_tracepoints()			\
   (the_target->supports_tracepoints			\
    ? (*the_target->supports_tracepoints) () : 0)
Index: gdbserver/win32-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
retrieving revision 1.66
diff -u -p -r1.66 win32-low.c
--- gdbserver/win32-low.c	2 Jul 2013 11:59:24 -0000	1.66
+++ gdbserver/win32-low.c	30 Aug 2013 15:18:17 -0000
@@ -1813,6 +1813,7 @@ static struct target_ops win32_target_op
   NULL, /* core_of_thread */
   NULL, /* read_loadmap */
   NULL, /* process_qsupported */
+  NULL, /* supports_catch_syscall */
   NULL, /* supports_tracepoints */
   NULL, /* read_pc */
   NULL, /* write_pc */
Index: testsuite/gdb.base/catch-syscall.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-syscall.exp,v
retrieving revision 1.19
diff -u -p -r1.19 catch-syscall.exp
--- testsuite/gdb.base/catch-syscall.exp	22 Aug 2013 20:32:54 -0000	1.19
+++ testsuite/gdb.base/catch-syscall.exp	30 Aug 2013 15:18:17 -0000
@@ -19,7 +19,7 @@
 # It was written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
 # on September/2008.
 
-if { [is_remote target] || ![isnative] } then {
+if { ![isnative] } then {
     continue
 }
 
@@ -28,6 +28,14 @@ if {![istarget "hppa*-hp-hpux*"] && ![is
     continue
 }
 
+# This shall be updated whenever QCatchSyscalls packet support is implemented
+# on some gdbserver architecture.
+if { [is_remote target]
+     && ![istarget "x86_64-*-linux*"] 
+     && ![istarget "i\[34567\]86-*-linux*"] } {
+    continue
+}
+
 # This shall be updated whenever 'catch syscall' is implemented
 # on some architecture.
 #if { ![istarget "i\[34567\]86-*-linux*"]

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Eli Zaretskii]

> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Fri, 30 Aug 2013 17:26:28 +0200
> 
> This is the 2nd version of the patch implementing 'catch syscalls' for
> gdbserver.
> First version was sent in an RFC (no feedback yet, so here is a completed
> and tested RFA ready version).
> Tested (no regression) on linux amd64, native and gdbserver.
> Manually tested with a patched Valgrind gdbserver.

Thanks.  The documentation parts are OK, but please be sure to leave 2
spaces between sentences (some of them have only one in this
changeset).

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Philippe Waroquiers]

On Fri, 2013-08-30 at 18:44 +0300, Eli Zaretskii wrote:
> > From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> > Date: Fri, 30 Aug 2013 17:26:28 +0200
> > 
> > This is the 2nd version of the patch implementing 'catch syscalls' for
> > gdbserver.
> > First version was sent in an RFC (no feedback yet, so here is a completed
> > and tested RFA ready version).
> > Tested (no regression) on linux amd64, native and gdbserver.
> > Manually tested with a patched Valgrind gdbserver.
> 
> Thanks.  The documentation parts are OK, but please be sure to leave 2
> spaces between sentences (some of them have only one in this
> changeset).
Oops, sorry, even if my brain knows it, my fingers don't.

Fixed the occurences detected with:
grep '\. [^ ]' patch.txt
+	     max 8+1 characters for a sysno. If the resulting
+	      /* catch_packet too big. Fallback to less efficient
+in hex) should be reported to @value{GDBN}. If no syscall @var{sysno}
+will filter it if this signal is not catched. It is however more efficient
+  /* Fill SYSNO with the syscall nr trapped. Fill SYSRET with the
+/* 1 if some (or all) syscalls are catched. */

Thanks for the quick review

Philippe

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Philippe Waroquiers]

On Fri, 2013-08-30 at 17:26 +0200, Philippe Waroquiers wrote:
> This is the 2nd version of the patch implementing 'catch syscalls' for
> gdbserver.
ping code review ?
(doc was already reviewed).

Thanks

Philippe

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Sergio Durigan Junior]

On Friday, August 30 2013, Philippe Waroquiers wrote:

> This is the 2nd version of the patch implementing 'catch syscalls' for
> gdbserver.
> First version was sent in an RFC (no feedback yet, so here is a completed
> and tested RFA ready version).
> Tested (no regression) on linux amd64, native and gdbserver.
> Manually tested with a patched Valgrind gdbserver.

Haven't tested it yet (was/am going to review it), but as we have
discussed on IRC, you might want to take a look at
<https://sourceware.org/bugzilla/show_bug.cgi?id=10737>.  Not that you
should fix it, but probably see how it affects this feature on
gdbserver.

Thanks,

-- 
Sergio

added to 7.7-branch wiki: Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Joel Brobecker]

For the record, since this patch has been submitted in a reasonable
timeframe prior to the planned 7.7 branch creation, I've added it
to our 7.7 release wiki page. It does not mean that we must get
this patch in the release (for instance if considered too risky),
but hopefully this will place this patch a little higher on the
reviewing queue...

Thank you :)

> Here are the changes logs:
> ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* NEWS: Document new QcatchSyscalls packet and its use
> 	in x86/amd64 linux gdbserver and Valgrind gdbserver.
> 	* remote.c (PACKET_QCatchSyscalls): New.
> 	(remote_protocol_features): Add Qcatchsyscalls.
> 	(remote_set_syscall_catchpoint): New function.
> 	(remote_parse_stop_reply): New stop reasons syscall_entry
> 	and syscall_return.
> 	(init_remote_ops): Registers remote_set_syscall_catchpoint
> 	and the config commands for [PACKET_QCatchSyscalls.
> 	* 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): Enables, detects and handle
> 	SYSCALL_SIGTRAP.
> 	(gdb_catched_syscall): 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 catch_syscalls_p, catched_syscalls_size and
> 	catched_syscalls.
> 	* server.c: Define catch_syscalls_p, catched_syscalls_size and
> 	catched_syscalls.
> 	(handle_general_set): Handle QCatchSyscalls packet.
> 	(handle_query): Reports if low target supports QCatchSyscalls.
> 	* win32-low.c (struct target_ops win32_target_op): Sets NULL
> 	for supports_catch_syscall.
> 
> testsuite/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.base/catch-syscall.exp: Enables test for x86 and amd64
> 	gdbserver.

-- 
Joel

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Sergio Durigan Junior]

On Friday, August 30 2013, Philippe Waroquiers wrote:

> This is the 2nd version of the patch implementing 'catch syscalls' for
> gdbserver.
> First version was sent in an RFC (no feedback yet, so here is a completed
> and tested RFA ready version).
> Tested (no regression) on linux amd64, native and gdbserver.
> Manually tested with a patched Valgrind gdbserver.

Thank you very much for this, Philippe :-).  I have some comments.

> Here are the changes logs:
> ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>
> 	* NEWS: Document new QcatchSyscalls packet and its use
> 	in x86/amd64 linux gdbserver and Valgrind gdbserver.
> 	* remote.c (PACKET_QCatchSyscalls): New.
> 	(remote_protocol_features): Add Qcatchsyscalls.
> 	(remote_set_syscall_catchpoint): New function.
> 	(remote_parse_stop_reply): New stop reasons syscall_entry
> 	and syscall_return.
> 	(init_remote_ops): Registers remote_set_syscall_catchpoint
> 	and the config commands for [PACKET_QCatchSyscalls.
                                    ^

Typo (wrong "[" at the beginning)?

> 	* common/linux-ptrace.c (linux_check_ptrace_features):
> 	Detect PTRACE_O_TRACESYSGOOD for gdbserver.
> 	(ptrace_supports_feature): Initializes ptrace features if needed.
>
> doc/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>
> 	* gdb.texinfo (General Query Packets): Document new QcatchSyscalls
> 	packet.

It's QCatchSyscalls (capital "C"), right?

> 	(Stop Reply Packets): Document new stop reasons syscall_entry and
> 	syscall_return.
> 	(Async Records): fixed syscall-return item name.

Capital "F" for "Fixed".

>
> gdbserver/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>
> 	* target.h (struct target_ops): Add supports_catch_syscall operation.
> 	* linux-low.h (struct linux_target_ops): Add get_syscall_trapinfo
> 	operation.
> 	* linux-low.c (linux_wait_1): Enables, detects and handle
> 	SYSCALL_SIGTRAP.

I'd rewrite it in the imperative form, i.e., "Enable, detect and handle".

> 	(gdb_catched_syscall): New function.
> 	(get_syscall_trapinfo): New function.
>         (linux_supports_catch_syscall): New function.

Wrong indentation.

> 	(struct target_ops linux_target_ops): Set linux_supports_catch_syscall.
> 	* linux-x86-low.c (x86_get_syscall_trapinfo): New function.
> 	(struct linux_target_ops the_low_target): Set x86_get_syscall_trapinfo.
> 	* remote-utils.c (prepare_resume_reply): Handle status kinds
> 	TARGET_WAITKIND_SYSCALL_ENTRY and TARGET_WAITKIND_SYSCALL_RETURN.
> 	* server.h: Declare catch_syscalls_p, catched_syscalls_size and
> 	catched_syscalls.
> 	* server.c: Define catch_syscalls_p, catched_syscalls_size and
> 	catched_syscalls.
> 	(handle_general_set): Handle QCatchSyscalls packet.
> 	(handle_query): Reports if low target supports QCatchSyscalls.
> 	* win32-low.c (struct target_ops win32_target_op): Sets NULL
> 	for supports_catch_syscall.
>
> testsuite/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>
> 	* gdb.base/catch-syscall.exp: Enables test for x86 and amd64
> 	gdbserver.
>
> Two specific points that might be worth looking at (after this patch
> probably):
>
> 1. GDB native has a bug in the detection  of "syscall entry/return",
>    when the catch syscall is disabled when inferior is stopped
>    on a syscall entry, and then catch syscall is re-enabled later:
>
>   Catchpoint 1 (call to syscall brk), 0x00207ead in brk ()
>      from /lib/ld-linux.so.2
>   (gdb) break main
>   Breakpoint 2 at 0x80486ca: file reach_thread_register.c, line 42.
>   (gdb) disa 1
>   (gdb) c
>   Continuing.
>   [Thread debugging using libthread_db enabled]
>   Using host libthread_db library "/lib/libthread_db.so.1".
>
>   Breakpoint 2, main () at reach_thread_register.c:42
>   42      pthread_barrier_init(&bar, NULL, 2);
>   (gdb) enable
>   (gdb) c
>   Continuing.
>
>   Catchpoint 1 (returned from syscall mmap2), 0x00121416 in __kernel_vsyscall ()
>   (gdb)         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ should be call to syscall mmap2
> /// after this, all entries are seen as returns, and all returns are seen
> /// as entries.
>
> In GDBSERVER, to differentiate entry from return, I use the
> syscall retcode -ENOSYS.
> This is working properly including for the case above.
> The only problem is that if a syscall is not implemented
> (and so really returns -ENOSYS)
> then GDBSERVER will wrongly report the return of the system call
> as an entry (but subsequent syscall entries/returns will be properly
> reported).
> Maybe there is a better approach, including for fixing
> the  'GDB native catch syscall' ?

Hm, I remember seeing and trying to fix this bug a long time ago.  Not
sure if I managed to do it or not (guess not, since I didn't send the
patch, but I remember having something close to the fix).  My initial
idea was to record the event (syscall entry or return) inside GDB and
make it decide based on it.

Anyway, I agree it's a bug and that it deserves a bug report :-).  Could
you please file it?

> 2. GDB (probably in breakpoint.c) has a bug with catch syscalls
>    in remote mode when 'set breakpoint always-inserted on' :
>    Disabling all the catch syscalls does not cause
>    a call to target_set_syscall_catchpoint with needed = 0,
>    which means that QCatchSyscalls:0 is not sent.
>    After that, GDBSERVER continues to report various syscalls
>    to GDB, that filters them, so no functional bug, only
>    efficiency bug.
>
> ----------------- Patch:
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.611
> diff -u -p -r1.611 NEWS
> --- NEWS	27 Aug 2013 05:20:56 -0000	1.611
> +++ NEWS	30 Aug 2013 15:18:08 -0000
> @@ -127,11 +127,21 @@ qXfer:libraries-svr4:read's annex
>    necessary for library list updating, resulting in significant
>    speedup.
>  
> +QCatchSyscalls:1 [;SYSNO]...
> +QCatchSyscalls:0
> +  Enable (`QCatchSyscalls:1') or disable (`QCatchSyscalls:0')
> +  catching syscalls from the inferior process.

We should not be using ` and ' anymore, now one should use " and ".

> +
> +
>  * New features in the GDB remote stub, GDBserver
>  
>    ** GDBserver now supports target-assisted range stepping.  Currently
>       enabled on x86/x86_64 GNU/Linux targets.
>  
> +  ** GDBserver now supports catch syscall catchpoints.  Currently

"catch syscall" or "syscall catchpoints", I think.

> +     enabled on x86/x86_64 GNU/Linux targets, and in the Valgrind
> +     gdbserver.
> +
>    ** GDBserver now adds element 'tvar' in the XML in the reply to
>       'qXfer:traceframe-info:read'.  It has the id of the collected
>       trace state variables.
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.577
> diff -u -p -r1.577 remote.c
> --- remote.c	23 Aug 2013 13:12:17 -0000	1.577
> +++ remote.c	30 Aug 2013 15:18:09 -0000
> @@ -1340,6 +1340,7 @@ enum {
>    PACKET_qSupported,
>    PACKET_qTStatus,
>    PACKET_QPassSignals,
> +  PACKET_QCatchSyscalls,
>    PACKET_QProgramSignals,
>    PACKET_qSearch_memory,
>    PACKET_vAttach,
> @@ -1728,6 +1729,93 @@ remote_pass_signals (int numsigs, unsign
>      }
>  }
>  
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> +   to report syscalls to GDB.  */

I generally prefer to explicitly say which method this function is
implementing.  Like, in this case, mention that it is implementing
"target_set_syscall_catchpoint".  It makes it easier to find the
definitions of each argument.

> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> +			       int table_size, int *table)
> +{
> +  if (remote_protocol_packets[PACKET_QCatchSyscalls].support != PACKET_DISABLE)
> +    {
> +      char *catch_packet, *p;
> +      enum packet_result result;
> +      int n_sysno = 0;
> +
> +      if (needed && !any_count)
> +	{
> +	  int i;
> +
> +	  for (i = 0; i < table_size; i++)
> +	    if (table[i])
> +	      n_sysno++;

When would table[i] be 0 in this case?  I guess you could use table_size
directly without worrying about n_sysno.

> +	}
> +
> +      if (remote_debug)
> +	fprintf_unfiltered (gdb_stdlog,
> +			    "remote_set_syscall_catchpoint "
> +			    "pid %d needed %d any_count %d n_sysno %d\n",
> +			    pid, needed, any_count, n_sysno);
> +      if (needed)
> +	{
> +	  /* Prepare a packet with the sysno list, assuming
> +	     max 8+1 characters for a sysno. If the resulting
> +	     packet size is too big, fallback on the non
> +	     selective packet.  */
> +	  const char *q1 = "QCatchSyscalls:1";

Really nitpicking but you could avoid calling strlen for q1 and replace
it by a size_t variable holding the sizeof (q1) - 1.  But this is just
a minor thing.

> +	  int i;
> +	  const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;
> +
> +	  catch_packet = xmalloc (maxpktsz);
> +	  strcpy (catch_packet, q1);
> +	  if (!any_count)
> +	    {
> +	      char *p;

Empty line between variable declaration and code.

> +	      p = catch_packet;
> +	      p += strlen(p);
> +	      for (i = 0; i < table_size; i++)
> +		{
> +		  if (table[i])

Again, I don't see the reason of this check.

> +		    {
> +		      xsnprintf(p, catch_packet + maxpktsz - p,
> +				";%x", i);
> +		      p += strlen(p);
> +		    }
> +		}
> +	    }
> +	  if (strlen(catch_packet) > get_remote_packet_size())
> +	    {
> +	      /* catch_packet too big. Fallback to less efficient
> +		 non selective mode, with GDB doing the filtering.  */
> +	      catch_packet[strlen (q1)] = 0;
> +	    }

Hm, can't you do this check before start filling catch_packet?  This
would save some time.

> +	}
> +      else
> +	{
> +	  catch_packet = xmalloc (strlen ("QCatchSyscalls:0") + 1);
> +	  strcpy (catch_packet, "QCatchSyscalls:0");
> +	}
> +
> +      {
> +	struct remote_state *rs = get_remote_state ();
> +	char *buf = rs->buf;
> +
> +	putpkt (catch_packet);
> +	getpkt (&rs->buf, &rs->buf_size, 0);
> +	result = packet_ok (buf,
> +			    &remote_protocol_packets[PACKET_QCatchSyscalls]);
> +	xfree (catch_packet);
> +	if (result == PACKET_OK)
> +	  return 0;
> +	else
> +	  return -1;
> +      }
> +    }
> +  else
> +    return 1; /* not supported */

Would you mind putting this at the beginning of the function?  Like:

  if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
    {
      /* Not supported.  */
      return 1;
    }

You would save one level of indentation without losing clarity.

> +}
> +
> +
>  /* If 'QProgramSignals' is supported, tell the remote stub what
>     signals it should pass through to the inferior when detaching.  */
>  
> @@ -4016,6 +4104,8 @@ static const struct protocol_feature rem
>      PACKET_qXfer_traceframe_info },
>    { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QPassSignals },
> +  { "QCatchSyscalls", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_QCatchSyscalls },
>    { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QProgramSignals },
>    { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
> @@ -5283,7 +5373,8 @@ typedef struct stop_reply
>  
>    int stopped_by_watchpoint_p;
>    CORE_ADDR watch_data_address;
> -
> +  

Spurious line with spaces.

> +  int syscall;

I know this struct is already messed up, but I would put a comment above
this field to explain its purpose.

I also wonder if all those fields could be turned into bitfields, but
that's another issue not relevant here :-).

>    int solibs_changed;
>    int replay_event;
>  
> @@ -5546,6 +5637,7 @@ remote_parse_stop_reply (char *buf, stru
>    event->ptid = null_ptid;
>    event->ws.kind = TARGET_WAITKIND_IGNORE;
>    event->ws.value.integer = 0;
> +  event->syscall = 0;
>    event->solibs_changed = 0;
>    event->replay_event = 0;
>    event->stopped_by_watchpoint_p = 0;
> @@ -5596,6 +5688,22 @@ Packet: '%s'\n"),
>  		       p, buf);
>  	      if (strncmp (p, "thread", p1 - p) == 0)
>  		event->ptid = read_ptid (++p1, &p);
> +	      else if (strncmp (p, "syscall_entry", p1 - p) == 0)
> +		{
> +		  ULONGEST sysno;

Empty newline missing.

> +		  event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> +		  p = unpack_varlen_hex (++p1, &sysno);
> +		  event->syscall = 1;
> +		  event->ws.value.syscall_number = (int) sysno;
> +		}
> +	      else if (strncmp (p, "syscall_return", p1 - p) == 0)
> +		{
> +		  ULONGEST sysno;

Empty newline missing.

> +		  event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
> +		  p = unpack_varlen_hex (++p1, &sysno);
> +		  event->syscall = 1;
> +		  event->ws.value.syscall_number = (int) sysno;
> +		}
>  	      else if ((strncmp (p, "watch", p1 - p) == 0)
>  		       || (strncmp (p, "rwatch", p1 - p) == 0)
>  		       || (strncmp (p, "awatch", p1 - p) == 0))
> @@ -5681,6 +5789,11 @@ Packet: '%s'\n"),
>  	event->ws.kind = TARGET_WAITKIND_LOADED;
>        else if (event->replay_event)
>  	event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
> +      else if (event->syscall)
> +	{
> +	  gdb_assert (event->ws.kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +		      || event->ws.kind == TARGET_WAITKIND_SYSCALL_RETURN);
> +	}

No need to open/close brackets.

>        else
>  	{
>  	  event->ws.kind = TARGET_WAITKIND_STOPPED;
> @@ -11452,6 +11565,7 @@ Specify the serial device it is connecte
>    remote_ops.to_load = generic_load;
>    remote_ops.to_mourn_inferior = remote_mourn;
>    remote_ops.to_pass_signals = remote_pass_signals;
> +  remote_ops.to_set_syscall_catchpoint = remote_set_syscall_catchpoint;
>    remote_ops.to_program_signals = remote_program_signals;
>    remote_ops.to_thread_alive = remote_thread_alive;
>    remote_ops.to_find_new_threads = remote_threads_info;
> @@ -11946,6 +12060,9 @@ Show the maximum size of the address (in
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
>  			 "QPassSignals", "pass-signals", 0);
>  
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_QCatchSyscalls],
> +			 "QCatchSyscalls", "catch-syscalls", 0);
> +
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
>  			 "QProgramSignals", "program-signals", 0);
>  
> Index: common/linux-ptrace.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 linux-ptrace.c
> --- common/linux-ptrace.c	28 Aug 2013 14:09:31 -0000	1.12
> +++ common/linux-ptrace.c	30 Aug 2013 15:18:09 -0000
> @@ -361,16 +361,17 @@ linux_check_ptrace_features (void)
>        return;
>      }
>  
> -#ifdef GDBSERVER
> -  /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
> -     PTRACE_O_TRACEVFORKDONE yet.  */
> -#else
> -  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
> +  /* Check if the target supports PTRACE_O_TRACESYSGOOD, keeping
> +     PTRACE_O_TRACEFORK option activated.  */
>    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> -		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> +		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> +				    | PTRACE_O_TRACESYSGOOD));

Any reason why you are keeping PTRACE_O_TRACEFORK set?

>    if (ret == 0)
>      current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
>  
> +#ifdef GDBSERVER
> +  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
> +#else
>    /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
>    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
>  		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> @@ -474,6 +475,11 @@ linux_enable_event_reporting (pid_t pid)
>  static int
>  ptrace_supports_feature (int ptrace_options)
>  {
> +  /* Check if we have initialized the ptrace features for this
> +     target.  If not, do it now.  */
> +  if (current_ptrace_options == -1)
> +    linux_check_ptrace_features ();
> +

This seems correct to me, but I think you should send it in a separate
patch, because it is not specific to catch syscall IMO.

>    gdb_assert (current_ptrace_options >= 0);
>  
>    return ((current_ptrace_options & ptrace_options) == ptrace_options);
> Index: doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.1105
> diff -u -p -r1.1105 gdb.texinfo
> --- doc/gdb.texinfo	27 Aug 2013 05:20:57 -0000	1.1105
> +++ doc/gdb.texinfo	30 Aug 2013 15:18:15 -0000
> @@ -18681,6 +18681,10 @@ are:
>  @tab @code{qSupported}
>  @tab Remote communications parameters
>  
> +@item @code{catch-syscalls}
> +@tab @code{QCatchSyscalls}
> +@tab @code{catch syscall}
> +
>  @item @code{pass-signals}
>  @tab @code{QPassSignals}
>  @tab @code{handle @var{signal}}
> @@ -38077,6 +38081,11 @@ The currently defined stop reasons are:
>  The packet indicates a watchpoint hit, and @var{r} is the data address, in
>  hex.
>  
> +@item syscall_entry
> +@itemx syscall_return
> +The packet indicates a syscall entry or return, and @var{r} is the 
> +syscall number, in hex.
> +
>  @cindex shared library events, remote reply
>  @item library
>  The packet indicates that the loaded libraries have changed.
> @@ -38447,6 +38456,44 @@ by supplying an appropriate @samp{qSuppo
>  Use of this packet is controlled by the @code{set non-stop} command; 
>  @pxref{Non-Stop Mode}.
>  
> +@item QCatchSyscalls:1 @r{[};@var{sysno}@r{]}@dots{}
> +@itemx QCatchSyscalls:0
> +@cindex catch syscalls from inferior, remote request
> +@cindex @samp{QCatchSyscalls} packet
> +@anchor{QCatchSyscalls}
> +Enable (@samp{QCatchSyscalls:1}) or disable (@samp{QCatchSyscalls:0})
> +catching syscalls from the inferior process.
> +
> +For @samp{QCatchSyscalls:1}, each listed syscall @var{sysno} (encoded
> +in hex) should be reported to @value{GDBN}. If no syscall @var{sysno}
> +is listed, every system call should be reported.
> +
> +Note that if a syscall not member of the list is reported, @value{GDBN}
> +will filter it if this signal is not catched. It is however more efficient
> +to only report the needed syscalls.
> + 
> +Multiple @samp{QCatchSyscalls:1} packets do not
> +combine; any earlier @samp{QCatchSyscalls:1} list is completely replaced by the
> +new list.
> +
> +Reply:
> +@table @samp
> +@item OK
> +The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  @var{nn} are hex digits.
> +
> +@item @w{}
> +An empty reply indicates that @samp{QCatchSyscalls} is not supported by
> +the stub.
> +@end table
> +
> +Use of this packet is controlled by the @code{set remote catch-syscalls}
> +command (@pxref{Remote Configuration, set remote catch-syscalls}).
> +This packet is not probed by default; the remote stub must request it,
> +by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
> +
>  @item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
>  @cindex pass signals to inferior, remote request
>  @cindex @samp{QPassSignals} packet
> @@ -38810,6 +38857,11 @@ These are the currently defined stub fea
>  @tab @samp{-}
>  @tab Yes
>  
> +@item @samp{QCatchSyscalls}
> +@tab No
> +@tab @samp{-}
> +@tab Yes
> +
>  @item @samp{QPassSignals}
>  @tab No
>  @tab @samp{-}
> @@ -38970,6 +39022,10 @@ packet (@pxref{qXfer fdpic loadmap read}
>  The remote stub understands the @samp{QNonStop} packet
>  (@pxref{QNonStop}).
>  
> +@item QCatchSyscalls
> +The remote stub understands the @samp{QCatchSyscalls} packet
> +(@pxref{QCatchSyscalls}).
> +
>  @item QPassSignals
>  The remote stub understands the @samp{QPassSignals} packet
>  (@pxref{QPassSignals}).
> Index: gdbserver/linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 linux-low.c
> --- gdbserver/linux-low.c	28 Aug 2013 17:40:58 -0000	1.246
> +++ gdbserver/linux-low.c	30 Aug 2013 15:18:16 -0000
> @@ -72,6 +72,11 @@
>  #define W_STOPCODE(sig) ((sig) << 8 | 0x7f)
>  #endif
>  
> +/* Unlike other extended result codes, WSTOPSIG (status) on
> +   PTRACE_O_TRACESYSGOOD syscall events doesn't return SIGTRAP, but
> +   instead SIGTRAP with bit 7 set.  */
> +#define SYSCALL_SIGTRAP (SIGTRAP | 0x80)
> +
>  /* This is the kernel's hard limit.  Not to be confused with
>     SIGRTMIN.  */
>  #ifndef __SIGRTMIN
> @@ -479,6 +484,33 @@ get_pc (struct lwp_info *lwp)
>    return pc;
>  }
>  
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.  */

Missing explanation of what the function does.

> +
> +static void
> +get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
> +{
> +  struct thread_info *saved_inferior;
> +  struct regcache *regcache;
> +
> +  if (the_low_target.get_syscall_trapinfo == NULL)
> +    {
> +      *sysno = 0;
> +      *sysret = 0;

Aren't you missing a "return" here?

In fact, is there any way for "the_low_target.get_syscall_trapinfo" to
be NULL?  It seems to me that this is a necessary condition for this
function to be called (see my comment about putting an assert at
server.c:handle_general_set).  So maybe you should put an assert here to
check that the_low_target.get_syscall_trapinfo != NULL.  WDYT?

> +    }
> +
> +  saved_inferior = current_inferior;
> +  current_inferior = get_lwp_thread (lwp);
> +
> +  regcache = get_thread_regcache (current_inferior, 1);
> +  (*the_low_target.get_syscall_trapinfo) (regcache, sysno, sysret);
> +
> +  if (debug_threads)
> +    fprintf (stderr, "get_syscall_trapinfo sysno %d sysret %d\n",
> +	     *sysno, *sysret);
> +
> +  current_inferior = saved_inferior;
> +}
> +
>  /* This function should only be called if LWP got a SIGTRAP.
>     The SIGTRAP could mean several things.
>  
> @@ -2246,6 +2278,29 @@ linux_stabilize_threads (void)
>      }
>  }
>  
> +/* Returns 1 if GDB is interested in the event_child syscall.
> +   Only to be called when stopped reason is SIGTRAP_SYSCALL.  */
> +
> +static int
> +gdb_catched_syscall (struct lwp_info *event_child)

s/catched/caught/g ?

Maybe also rename the function to "catch_current_syscall_p"?  Or
something to give the idea that we are checking if this specific syscall
is interesting for us.

> +{
> +  int i;
> +  int sysno, sysret;
> +
> +  if (!catch_syscalls_p)

I think you could rename this to "catching_syscalls_p".

> +    return 0;
> +
> +  if (catched_syscalls_size == 0)
> +    return 1;
> +
> +  get_syscall_trapinfo (event_child, &sysno, &sysret);
> +  for (i = 0; i < catched_syscalls_size; i++)
> +    if (catched_syscalls[i] == sysno)
> +      return 1;

Also, I think the names of the variables are a bit misleading.
"catched_syscalls" does not represent syscalls that were caught, but
rather syscalls that are to be caught.  Maybe rename them to
"syscalls_to_be_caught" or something?

> +
> +  return 0;
> +}
> +
>  /* Wait for process, returns status.  */
>  
>  static ptid_t
> @@ -2527,6 +2582,19 @@ Check if we're already there.\n",
>  
>    /* Check whether GDB would be interested in this event.  */
>  
> +  /* Check if GDB is interested in this syscall.  */
> +  if (WIFSTOPPED (w)
> +      && WSTOPSIG (w) == SYSCALL_SIGTRAP 

Spurious whitespace at the end.  This line could also be joined with the
line above.

> +      && !gdb_catched_syscall (event_child))
> +    {
> +      if (debug_threads)
> +	fprintf (stderr, "Ignored syscall for LWP %ld.\n",
> +		 lwpid_of (event_child));
> +      linux_resume_one_lwp (event_child, event_child->stepping,
> +			    0, NULL);
> +      goto retry;
> +    }
> +
>    /* If GDB is not interested in this signal, don't stop other
>       threads, and don't report it to GDB.  Just resume the inferior
>       right away.  We do this for threading-related signals as well as
> @@ -2705,7 +2773,18 @@ Check if we're already there.\n",
>  
>    ourstatus->kind = TARGET_WAITKIND_STOPPED;
>  
> -  if (current_inferior->last_resume_kind == resume_stop
> +  if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
> +    {
> +      int sysret;
> +
> +      get_syscall_trapinfo (event_child,
> +			    &ourstatus->value.syscall_number, &sysret);
> +      if (sysret == -ENOSYS)
> +	ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> +      else
> +	ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;

It would be good to include a comment here explaining why you're using
-ENOSYS to determine whether it's a syscall entry or return.

> +    }
> +  else if (current_inferior->last_resume_kind == resume_stop
>        && WSTOPSIG (w) == SIGSTOP)
>      {
>        /* A thread that has been requested to stop by GDB with vCont;t,
> @@ -3267,7 +3346,8 @@ lwp %ld wants to get out of fast tracepo
>    lwp->stopped = 0;
>    lwp->stopped_by_watchpoint = 0;
>    lwp->stepping = step;
> -  ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (lwp),
> +  ptrace (step ? PTRACE_SINGLESTEP : catch_syscalls_p ? PTRACE_SYSCALL : PTRACE_CONT, 
> +	  lwpid_of (lwp),

The line above is too long.  I am also not fond of using
"double-ternary" :-/.  Maybe it's just me, but I find them quite
confusing sometimes.

>  	  (PTRACE_TYPE_ARG3) 0,
>  	  /* Coerce to a uintptr_t first to avoid potential gcc warning
>  	     of coercing an 8 byte integer to a 4 byte pointer.  */
> @@ -5106,6 +5186,13 @@ linux_process_qsupported (const char *qu
>  }
>  
>  static int
> +linux_supports_catch_syscall (void)
> +{
> +  return the_low_target.get_syscall_trapinfo != NULL
> +    && linux_supports_tracesysgood();

You could use a parenthesis here to properly indent the check.

> +}
> +
> +static int
>  linux_supports_tracepoints (void)
>  {
>    if (*the_low_target.supports_tracepoints == NULL)
> @@ -5796,6 +5883,7 @@ static struct target_ops linux_target_op
>    linux_common_core_of_thread,
>    linux_read_loadmap,
>    linux_process_qsupported,
> +  linux_supports_catch_syscall,
>    linux_supports_tracepoints,
>    linux_read_pc,
>    linux_write_pc,
> Index: gdbserver/linux-low.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.h,v
> retrieving revision 1.65
> diff -u -p -r1.65 linux-low.h
> --- gdbserver/linux-low.h	22 Aug 2013 23:46:29 -0000	1.65
> +++ gdbserver/linux-low.h	30 Aug 2013 15:18:16 -0000
> @@ -187,6 +187,12 @@ struct linux_target_ops
>    /* Hook to support target specific qSupported.  */
>    void (*process_qsupported) (const char *);
>  
> +  /* Fill SYSNO with the syscall nr trapped. Fill SYSRET with the
                                               ^

Double space after dot.

> +     return code.  Only to be called when inferior is stopped
> +     due to SYSCALL_SIGTRAP.  */
> +  void (*get_syscall_trapinfo) (struct regcache *regcache,
> +				int *sysno, int *sysret);
> +
>    /* Returns true if the low target supports tracepoints.  */
>    int (*supports_tracepoints) (void);
>  
> Index: gdbserver/linux-x86-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-low.c,v
> retrieving revision 1.49
> diff -u -p -r1.49 linux-x86-low.c
> --- gdbserver/linux-x86-low.c	12 Jun 2013 16:05:39 -0000	1.49
> +++ gdbserver/linux-x86-low.c	30 Aug 2013 15:18:16 -0000
> @@ -1472,6 +1472,32 @@ x86_arch_setup (void)
>    current_process ()->tdesc = x86_linux_read_description ();
>  }
>  
> +static void
> +x86_get_syscall_trapinfo (struct regcache *regcache, int *sysno, int *sysret)
> +{
> +  int use_64bit = register_size (regcache->tdesc, 0) == 8;
> +
> +  if (use_64bit)
> +    {
> +      long l_sysno;
> +      long l_sysret;

Empty newline missing.

> +      collect_register_by_name (regcache, "orig_rax", &l_sysno);
> +      collect_register_by_name (regcache, "rax", &l_sysret);
> +      *sysno = (int) l_sysno;
> +      *sysret = (int) l_sysret;
> +    }
> +  else
> +    {
> +      int l_sysno;
> +      int l_sysret;

Empty newline missing.

> +      collect_register_by_name (regcache, "orig_eax", &l_sysno);
> +      collect_register_by_name (regcache, "eax", &l_sysret);
> +      *sysno = (int) l_sysno;
> +      *sysret = (int) l_sysret;
> +    }
> +

Spurious newline.
> +}
> +
>  static int
>  x86_supports_tracepoints (void)
>  {
> @@ -3321,6 +3347,7 @@ struct linux_target_ops the_low_target =
>    x86_linux_new_thread,
>    x86_linux_prepare_to_resume,
>    x86_linux_process_qsupported,
> +  x86_get_syscall_trapinfo,
>    x86_supports_tracepoints,
>    x86_get_thread_area,
>    x86_install_fast_tracepoint_jump_pad,
> Index: gdbserver/remote-utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 remote-utils.c
> --- gdbserver/remote-utils.c	1 Jul 2013 11:19:27 -0000	1.98
> +++ gdbserver/remote-utils.c	30 Aug 2013 15:18:16 -0000
> @@ -1319,12 +1319,17 @@ prepare_resume_reply (char *buf, ptid_t 
>    switch (status->kind)
>      {
>      case TARGET_WAITKIND_STOPPED:
> +    case TARGET_WAITKIND_SYSCALL_ENTRY:
> +    case TARGET_WAITKIND_SYSCALL_RETURN:
>        {
>  	struct thread_info *saved_inferior;
>  	const char **regp;
>  	struct regcache *regcache;
>  
> -	sprintf (buf, "T%02x", status->value.sig);
> +	if (status->kind == TARGET_WAITKIND_STOPPED)
> +	  sprintf (buf, "T%02x", status->value.sig);
> +	else
> +	  sprintf (buf, "T%02x", SIGTRAP);

Again, maybe it's just me, but would you mind putting a comment here
explainig why you're using SIGTRAP directly if status->kind ==
TARGET_WAITKIND_SYSCALL_{ENTRY,RETURN} ?

>  	buf += strlen (buf);
>  
>  	saved_inferior = current_inferior;
> @@ -1335,6 +1340,16 @@ prepare_resume_reply (char *buf, ptid_t 
>  
>  	regcache = get_thread_regcache (current_inferior, 1);
>  
> +	if (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +	    || status->kind == TARGET_WAITKIND_SYSCALL_RETURN)
> +	  {
> +	    sprintf (buf, "%s:%x;",
> +		     status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +		     ? "syscall_entry" : "syscall_return",
> +		     status->value.syscall_number);
> +	    buf += strlen (buf);
> +	  }
> +	  
>  	if (the_target->stopped_by_watchpoint != NULL
>  	    && (*the_target->stopped_by_watchpoint) ())
>  	  {
> Index: gdbserver/server.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
> retrieving revision 1.196
> diff -u -p -r1.196 server.c
> --- gdbserver/server.c	28 Aug 2013 17:40:58 -0000	1.196
> +++ gdbserver/server.c	30 Aug 2013 15:18:17 -0000
> @@ -71,6 +71,9 @@ int debug_threads;
>  int debug_hw_points;
>  
>  int pass_signals[GDB_SIGNAL_LAST];
> +int catch_syscalls_p;
> +int catched_syscalls_size;

size_t?  :-)

(I know, the interface should be changed as well, but...)

> +int *catched_syscalls;
>  int program_signals[GDB_SIGNAL_LAST];
>  int program_signals_p;
>  
> @@ -507,6 +510,46 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (strncmp ("QCatchSyscalls:1", own_buf, strlen ("QCatchSyscalls:1")) == 0)
> +    {
> +      int i;
> +      const char *p;
> +      CORE_ADDR sysno;
> +

I am not very familiar with the remote protocol, but IMHO you could do
an assert here to check if target_supports_catch_syscall returns 1,
WDYT?  IOW, we shouldn't get here if it returns 0, right?

Or is there some sort of mechanism intended to overwrite the initial
probing of features and force unsupported features to be enabled?  In
that case, I guess the assert would trigger every time...  Not sure what
to do.

> +      catch_syscalls_p = 1;
> +      if (catched_syscalls != NULL)
> +	{
> +	  free (catched_syscalls);

xfree.

> +	  catched_syscalls = NULL;
> +	}
> +      catched_syscalls_size = 0;
> +      p = own_buf + strlen("QCatchSyscalls:1");
> +      while (*p)
> +	{
> +	  if (*p++ == ';')
> +	    catched_syscalls_size++;
> +	}
> +      if (catched_syscalls_size > 0)
> +	{
> +	  catched_syscalls = xmalloc (catched_syscalls_size * sizeof (int));
> +	  p = strchr(own_buf, ';') + 1;
> +	  for (i = 0; i < catched_syscalls_size; i++)
> +	    {
> +	      p = decode_address_to_semicolon (&sysno, p);
> +	      catched_syscalls [i] = (int) sysno;
                              ^

Spurious whitespace.

> +	    }
> +	}
> +      strcpy (own_buf, "OK");
> +      return;
> +    }
> +
> +  if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> +    {
> +      catch_syscalls_p = 0;
> +      strcpy (own_buf, "OK");
> +      return;
> +    }
> +
>    if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
>      {
>        int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1740,6 +1783,9 @@ handle_query (char *own_buf, int packet_
>  	       "PacketSize=%x;QPassSignals+;QProgramSignals+",
>  	       PBUFSIZ - 1);
>  
> +      if (target_supports_catch_syscall())

Missing space between function name and parenthesis.

> +	strcat (own_buf, ";QCatchSyscalls+");
> +
>        if (the_target->qxfer_libraries_svr4 != NULL)
>  	strcat (own_buf, ";qXfer:libraries-svr4:read+"
>  		";augmented-libraries-svr4-read+");
> Index: gdbserver/server.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
> retrieving revision 1.109
> diff -u -p -r1.109 server.h
> --- gdbserver/server.h	1 Jul 2013 11:28:30 -0000	1.109
> +++ gdbserver/server.h	30 Aug 2013 15:18:17 -0000
> @@ -230,6 +230,15 @@ extern int server_waiting;
>  extern int debug_threads;
>  extern int debug_hw_points;
>  extern int pass_signals[];
> +
> +/* 1 if some (or all) syscalls are catched. */

I'd rewrite this comment:

  /* Set to 1 if we are catching syscalls, zero otherwise.  */

> +extern int catch_syscalls_p;
> +/* catched_syscalls is the list of syscalls to report to GDB.
> +   If catch_syscalls_p and catched_syscalls == NULL, it means
> +   all syscalls must be reported.  */
> +extern int catched_syscalls_size;
> +extern int *catched_syscalls;
> +
>  extern int program_signals[];
>  extern int program_signals_p;
>  
> Index: gdbserver/target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
> retrieving revision 1.70
> diff -u -p -r1.70 target.h
> --- gdbserver/target.h	19 Aug 2013 16:54:11 -0000	1.70
> +++ gdbserver/target.h	30 Aug 2013 15:18:17 -0000
> @@ -267,6 +267,10 @@ struct target_ops
>    /* Target specific qSupported support.  */
>    void (*process_qsupported) (const char *);
>  
> +  /* Return 1 if the target supports catch syscall, 0 (or leave the
> +     callback NULL) otherwise.  */
> +  int (*supports_catch_syscall) (void);
> +
>    /* Return 1 if the target supports tracepoints, 0 (or leave the
>       callback NULL) otherwise.  */
>    int (*supports_tracepoints) (void);
> @@ -413,6 +417,10 @@ int kill_inferior (int);
>  	the_target->process_qsupported (query);		\
>      } while (0)
>  
> +#define target_supports_catch_syscall()              	\
> +  (the_target->supports_catch_syscall ?			\
> +   (*the_target->supports_catch_syscall) () : 0)
> +
>  #define target_supports_tracepoints()			\
>    (the_target->supports_tracepoints			\
>     ? (*the_target->supports_tracepoints) () : 0)
> Index: gdbserver/win32-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 win32-low.c
> --- gdbserver/win32-low.c	2 Jul 2013 11:59:24 -0000	1.66
> +++ gdbserver/win32-low.c	30 Aug 2013 15:18:17 -0000
> @@ -1813,6 +1813,7 @@ static struct target_ops win32_target_op
>    NULL, /* core_of_thread */
>    NULL, /* read_loadmap */
>    NULL, /* process_qsupported */
> +  NULL, /* supports_catch_syscall */
>    NULL, /* supports_tracepoints */
>    NULL, /* read_pc */
>    NULL, /* write_pc */
> Index: testsuite/gdb.base/catch-syscall.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-syscall.exp,v
> retrieving revision 1.19
> diff -u -p -r1.19 catch-syscall.exp
> --- testsuite/gdb.base/catch-syscall.exp	22 Aug 2013 20:32:54 -0000	1.19
> +++ testsuite/gdb.base/catch-syscall.exp	30 Aug 2013 15:18:17 -0000
> @@ -19,7 +19,7 @@
>  # It was written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
>  # on September/2008.
>  
> -if { [is_remote target] || ![isnative] } then {
> +if { ![isnative] } then {
>      continue
>  }
>  
> @@ -28,6 +28,14 @@ if {![istarget "hppa*-hp-hpux*"] && ![is
>      continue
>  }
>  
> +# This shall be updated whenever QCatchSyscalls packet support is implemented
> +# on some gdbserver architecture.
> +if { [is_remote target]
> +     && ![istarget "x86_64-*-linux*"] 
> +     && ![istarget "i\[34567\]86-*-linux*"] } {
> +    continue
> +}

Hm, am I too sleepy or this test does nothing at all?  :-).

> +
>  # This shall be updated whenever 'catch syscall' is implemented
>  # on some architecture.
>  #if { ![istarget "i\[34567\]86-*-linux*"]

Otherwise, the patch looks pretty good to me!  I have tested it here and
as far as I have seen it is working OK.  I hope my comments help you
with getting the patch approved.  I am not a maintainer, so you will
need to wait for one to review the patch as well :-).

Thank you very much for working on this.

-- 
Sergio

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Philippe Waroquiers]

On Thu, 2013-09-19 at 02:29 -0300, Sergio Durigan Junior wrote:

> Thank you very much for this, Philippe :-).  I have some comments.
Thanks for the in-depth review.
I have a few comments for which I have questions/remarks below.


> > 1. GDB native has a bug in the detection  of "syscall entry/return",
> >    when the catch syscall is disabled when inferior is stopped
> >    on a syscall entry, and then catch syscall is re-enabled later:
> Hm, I remember seeing and trying to fix this bug a long time ago.  Not
> sure if I managed to do it or not (guess not, since I didn't send the
> patch, but I remember having something close to the fix).  My initial
> idea was to record the event (syscall entry or return) inside GDB and
> make it decide based on it.
> 
> Anyway, I agree it's a bug and that it deserves a bug report :-).  Could
> you please file it?
I stared at the problem when I detected it,
but could not see any easy fix which would work properly
in all circumstances e.g. enable/disable catch at various
moments, including interrupting a process blocked in a syscall,
enable catch syscall and have the syscall restarted
(did not check the behaviour of ptrace, I guess we will directly
have a syscall return event, and so GDB has to guess this is a return
without having seen the entry).
I will file a bug report (will analyse first the various problematic
cases and maybe do a test to reproduce them).

> > RCS file: /cvs/src/src/gdb/remote.c,v
> > +/* If 'QCatchSyscalls' is supported, tell the remote stub
> > +   to report syscalls to GDB.  */
> 
> I generally prefer to explicitly say which method this function is
> implementing.  Like, in this case, mention that it is implementing
> "target_set_syscall_catchpoint".  It makes it easier to find the
> definitions of each argument.
No problem to do that, however it looks like all other functions
in the same file are not commented like that.
So, doing this would make this new function commented differently
of all others.
Maybe better to keep it consistent ?
(and in another patch add for all functions a
  "implements target_xxxxxxxx") ?


> 
> > +
> > +static int
> > +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> > +			       int table_size, int *table)
> > +{
> > +  if (remote_protocol_packets[PACKET_QCatchSyscalls].support != PACKET_DISABLE)
> > +    {
> > +      char *catch_packet, *p;
> > +      enum packet_result result;
> > +      int n_sysno = 0;
> > +
> > +      if (needed && !any_count)
> > +	{
> > +	  int i;
> > +
> > +	  for (i = 0; i < table_size; i++)
> > +	    if (table[i])
> > +	      n_sysno++;
> 
> When would table[i] be 0 in this case?  I guess you could use table_size
> directly without worrying about n_sysno.
No, the 0 condition is to be checked.
See following comment in target.h: 
   TABLE is an array of ints, indexed by syscall number.  An element in
   this array is nonzero if that syscall should be caught.  This argument
   only matters if ANY_COUNT is zero.

I added a comment:
      /* Count how many syscalls are to be caught (table[sysno] != 0).  */


> 
> > +	}
> > +
> > +      if (remote_debug)
> > +	fprintf_unfiltered (gdb_stdlog,
> > +			    "remote_set_syscall_catchpoint "
> > +			    "pid %d needed %d any_count %d n_sysno %d\n",
> > +			    pid, needed, any_count, n_sysno);
> > +      if (needed)
> > +	{
> > +	  /* Prepare a packet with the sysno list, assuming
> > +	     max 8+1 characters for a sysno. If the resulting
> > +	     packet size is too big, fallback on the non
> > +	     selective packet.  */
> > +	  const char *q1 = "QCatchSyscalls:1";
> 
> Really nitpicking but you could avoid calling strlen for q1 and replace
> it by a size_t variable holding the sizeof (q1) - 1.  But this is just
> a minor thing.

I cannot use sizeof(q1) but I could possibly do
   const size_t q1len = strlen (q1);
and use q1len after ?
(that would however only spare a (very unlikely) second call
to strlen, probably optimised by the compiler in any case).


> > +	      p = catch_packet;
> > +	      p += strlen(p);
> > +	      for (i = 0; i < table_size; i++)
> > +		{
> > +		  if (table[i])
> 
> Again, I don't see the reason of this check.
See above.
Added a comment:
       /* Add in catch_packet each syscall to be caught (table[i] != 0).  */

> 
> > +		    {
> > +		      xsnprintf(p, catch_packet + maxpktsz - p,
> > +				";%x", i);
> > +		      p += strlen(p);
> > +		    }
> > +		}
> > +	    }
> > +	  if (strlen(catch_packet) > get_remote_packet_size())
> > +	    {
> > +	      /* catch_packet too big. Fallback to less efficient
> > +		 non selective mode, with GDB doing the filtering.  */
> > +	      catch_packet[strlen (q1)] = 0;
> > +	    }
> 
> Hm, can't you do this check before start filling catch_packet?  This
> would save some time.
Not easy to do (and saving probably very little cpu time in very
unfrequent cases):
The size of the packet depends on which syscalls
are added, and not only on the nr of syscalls to add in the packet.
E.g. a sysno can give either 2 or 3 or 4 characters (if we have
sysno only below 999). 


> > +  int syscall;
> 
> I know this struct is already messed up, but I would put a comment above
> this field to explain its purpose.
Added the comment:
  /* Set to 1 if the stop_reply is for a syscall entry or
     return.  The field ws.value.syscall_number then identifies
     the syscall.  */


> > -  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
> > +  /* Check if the target supports PTRACE_O_TRACESYSGOOD, keeping
> > +     PTRACE_O_TRACEFORK option activated.  */
> >    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> > -		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> > +		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> > +				    | PTRACE_O_TRACESYSGOOD));
> 
> Any reason why you are keeping PTRACE_O_TRACEFORK set?
Changed the comment to:
  /* 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.  */

> > RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
> >    if (ret == 0)
> >      current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
> >  
> > +#ifdef GDBSERVER
> > +  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
> > +#else
> >    /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
> >    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> >  		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> > @@ -474,6 +475,11 @@ linux_enable_event_reporting (pid_t pid)
> >  static int
> >  ptrace_supports_feature (int ptrace_options)
> >  {
> > +  /* Check if we have initialized the ptrace features for this
> > +     target.  If not, do it now.  */
> > +  if (current_ptrace_options == -1)
> > +    linux_check_ptrace_features ();
> > +
> 
> This seems correct to me, but I think you should send it in a separate
> patch, because it is not specific to catch syscall IMO.
It looks better to me to keep it in this patch, as it was not needed
before, but is now needed due to the "early calls" to
ptrace_supports_feature needed for catch syscalls in gdbserver.


> > ===================================================================
> > RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v

> > +static void
> > +get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
> > +{
> > +  struct thread_info *saved_inferior;
> > +  struct regcache *regcache;
> > +
> > +  if (the_low_target.get_syscall_trapinfo == NULL)
> > +    {
> > +      *sysno = 0;
> > +      *sysret = 0;
> 
> Aren't you missing a "return" here?
Yes, added it.
> 
> In fact, is there any way for "the_low_target.get_syscall_trapinfo" to
> be NULL?  It seems to me that this is a necessary condition for this
> function to be called (see my comment about putting an assert at
> server.c:handle_general_set).  So maybe you should put an assert here to
> check that the_low_target.get_syscall_trapinfo != NULL.  WDYT?
Good question, I am not sure what to do.
It looks like many  other calls to low level target
are protected similarly (which is not a good enough reason).
However, in this case, I suspect that if the GDB users forces
the use of the QCatchSyscalls packet, that we might end up there:
GDB will send such a packet, resulting in server.c setting
catching_syscalls_p to 1, resulting to ptrace "syscall" to be activated
resulting in a syscall trap event resulting in a call to getsyscall
trapinfo.
So, returning sysno 0 looks safe to me, to avoid a GDB user
causing an assert due to forcing the use of a (normally)
auto detected packet.


> > +/* Returns 1 if GDB is interested in the event_child syscall.
> > +   Only to be called when stopped reason is SIGTRAP_SYSCALL.  */
> > +
> > +static int
> > +gdb_catched_syscall (struct lwp_info *event_child)
> 
> s/catched/caught/g ?
> 
> Maybe also rename the function to "catch_current_syscall_p"?  Or
> something to give the idea that we are checking if this specific syscall
> is interesting for us.
changed to gdb_catch_this_syscall_p.
_
> 
> > +{
> > +  int i;
> > +  int sysno, sysret;
> > +
> > +  if (!catch_syscalls_p)
> 
> I think you could rename this to "catching_syscalls_p".
Yes, good idea.

> 
> > +    return 0;
> > +
> > +  if (catched_syscalls_size == 0)
> > +    return 1;
> > +
> > +  get_syscall_trapinfo (event_child, &sysno, &sysret);
> > +  for (i = 0; i < catched_syscalls_size; i++)
> > +    if (catched_syscalls[i] == sysno)
> > +      return 1;
> 
> Also, I think the names of the variables are a bit misleading.
> "catched_syscalls" does not represent syscalls that were caught, but
> rather syscalls that are to be caught.  Maybe rename them to
> "syscalls_to_be_caught" or something?
Yes.
Renamed to syscalls_to_catch_size and syscalls_to_catch.
grep-ped for frenglish 'catched' to ensure I catched all errors :).



> > Index: gdbserver/remote-utils.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> > retrieving revision 1.98
> > diff -u -p -r1.98 remote-utils.c
> > --- gdbserver/remote-utils.c	1 Jul 2013 11:19:27 -0000	1.98
> > +++ gdbserver/remote-utils.c	30 Aug 2013 15:18:16 -0000
> > @@ -1319,12 +1319,17 @@ prepare_resume_reply (char *buf, ptid_t 
> >    switch (status->kind)
> >      {
> >      case TARGET_WAITKIND_STOPPED:
> > +    case TARGET_WAITKIND_SYSCALL_ENTRY:
> > +    case TARGET_WAITKIND_SYSCALL_RETURN:
> >        {
> >  	struct thread_info *saved_inferior;
> >  	const char **regp;
> >  	struct regcache *regcache;
> >  
> > -	sprintf (buf, "T%02x", status->value.sig);
> > +	if (status->kind == TARGET_WAITKIND_STOPPED)
> > +	  sprintf (buf, "T%02x", status->value.sig);
> > +	else
> > +	  sprintf (buf, "T%02x", SIGTRAP);
> 
> Again, maybe it's just me, but would you mind putting a comment here
> explainig why you're using SIGTRAP directly if status->kind ==
> TARGET_WAITKIND_SYSCALL_{ENTRY,RETURN} ?
This is according to the protocol definition:
        * If N is a recognized "stop reason", it describes a more
          specific event that stopped the target.  The currently
          defined stop reasons are listed below.  AA should be `05',
          the trap signal.  At most one stop reason should be present.


> >  	  {
> > Index: gdbserver/server.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
> > retrieving revision 1.196
> > diff -u -p -r1.196 server.c
> > --- gdbserver/server.c	28 Aug 2013 17:40:58 -0000	1.196
> > +++ gdbserver/server.c	30 Aug 2013 15:18:17 -0000
> > @@ -71,6 +71,9 @@ int debug_threads;
> >  int debug_hw_points;
> >  
> >  int pass_signals[GDB_SIGNAL_LAST];
> > +int catch_syscalls_p;
> > +int catched_syscalls_size;
> 
> size_t?  :-)
I guess an int will be ok unless we reach zillions
of different syscalls, and then the protocol packet
decoding code will suffer a lot :).
 

> I am not very familiar with the remote protocol, but IMHO you could do
> an assert here to check if target_supports_catch_syscall returns 1,
> WDYT?  IOW, we shouldn't get here if it returns 0, right?
> 
> Or is there some sort of mechanism intended to overwrite the initial
> probing of features and force unsupported features to be enabled?  In
> that case, I guess the assert would trigger every time...  Not sure what
> to do.
Yes, I suspect this is true (so has not added asserts).
It is however not very clear to me why GDB allows to overwrite
auto-detected packets, as this might effectively give
unexpected results. But it looks better to not crash gdbserver
following a "legal" user action.


> > Index: testsuite/gdb.base/catch-syscall.exp
> >
> > +# This shall be updated whenever QCatchSyscalls packet support is implemented
> > +# on some gdbserver architecture.
> > +if { [is_remote target]
> > +     && ![istarget "x86_64-*-linux*"] 
> > +     && ![istarget "i\[34567\]86-*-linux*"] } {
> > +    continue
> > +}
> 
> Hm, am I too sleepy or this test does nothing at all?  :-).
We both suspect we are too sleepy :).
Looks to me the above is the expected condition, which is to "continue
somewhere else" (whatever that "continue" exactly means in tcl/dejagnu,
I have no idea :).
This condition looks similar to equivalent conditions just before,
that are skipping this test on non matching target.
But my tcl/dejagnu knowledge is close to 0, so I might have missed
something.


> Otherwise, the patch looks pretty good to me!  I have tested it here and
> as far as I have seen it is working OK.  I hope my comments help you
> with getting the patch approved.  I am not a maintainer, so you will
> need to wait for one to review the patch as well :-).
Thanks for the extensive and helpful comments.
I will retest and submit another patch version (I guess this WE).
Any additional comment or feedback welcome in the meantime ...

Philippe

Re: RFA [PATCH] Implement 'catch syscall' for gdbserver

[Sergio Durigan Junior]

On Thursday, September 19 2013, Philippe Waroquiers wrote:

> On Thu, 2013-09-19 at 02:29 -0300, Sergio Durigan Junior wrote:

>> > 1. GDB native has a bug in the detection  of "syscall entry/return",
>> >    when the catch syscall is disabled when inferior is stopped
>> >    on a syscall entry, and then catch syscall is re-enabled later:
>> Hm, I remember seeing and trying to fix this bug a long time ago.  Not
>> sure if I managed to do it or not (guess not, since I didn't send the
>> patch, but I remember having something close to the fix).  My initial
>> idea was to record the event (syscall entry or return) inside GDB and
>> make it decide based on it.
>> 
>> Anyway, I agree it's a bug and that it deserves a bug report :-).  Could
>> you please file it?
> I stared at the problem when I detected it,
> but could not see any easy fix which would work properly
> in all circumstances e.g. enable/disable catch at various
> moments, including interrupting a process blocked in a syscall,
> enable catch syscall and have the syscall restarted
> (did not check the behaviour of ptrace, I guess we will directly
> have a syscall return event, and so GDB has to guess this is a return
> without having seen the entry).

Yes, I remember it wasn't an easy problem to solve, indeed.

> I will file a bug report (will analyse first the various problematic
> cases and maybe do a test to reproduce them).

Thanks for that, much appreciated.  Please add me in the Cc list of the
bug if you remember.

>
>> > RCS file: /cvs/src/src/gdb/remote.c,v
>> > +/* If 'QCatchSyscalls' is supported, tell the remote stub
>> > +   to report syscalls to GDB.  */
>> 
>> I generally prefer to explicitly say which method this function is
>> implementing.  Like, in this case, mention that it is implementing
>> "target_set_syscall_catchpoint".  It makes it easier to find the
>> definitions of each argument.
> No problem to do that, however it looks like all other functions
> in the same file are not commented like that.
> So, doing this would make this new function commented differently
> of all others.
> Maybe better to keep it consistent ?
> (and in another patch add for all functions a
>   "implements target_xxxxxxxx") ?

Yes, this is a recorrent problem in GDB.  What I prefer to do is to
ignore the wrong functions and make my functions in "The Right Way", but
I do realize it's a battle between consistency and correctness, so I
will not insist on it.

>> > +
>> > +static int
>> > +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
>> > +			       int table_size, int *table)
>> > +{
>> > +  if (remote_protocol_packets[PACKET_QCatchSyscalls].support != PACKET_DISABLE)
>> > +    {
>> > +      char *catch_packet, *p;
>> > +      enum packet_result result;
>> > +      int n_sysno = 0;
>> > +
>> > +      if (needed && !any_count)
>> > +	{
>> > +	  int i;
>> > +
>> > +	  for (i = 0; i < table_size; i++)
>> > +	    if (table[i])
>> > +	      n_sysno++;
>> 
>> When would table[i] be 0 in this case?  I guess you could use table_size
>> directly without worrying about n_sysno.
> No, the 0 condition is to be checked.
> See following comment in target.h: 
>    TABLE is an array of ints, indexed by syscall number.  An element in
>    this array is nonzero if that syscall should be caught.  This argument
>    only matters if ANY_COUNT is zero.
>
> I added a comment:
>       /* Count how many syscalls are to be caught (table[sysno] != 0).  */

You are right, of course.  Sorry about the confusion, this interface
messed with my brain.

>> > +	}
>> > +
>> > +      if (remote_debug)
>> > +	fprintf_unfiltered (gdb_stdlog,
>> > +			    "remote_set_syscall_catchpoint "
>> > +			    "pid %d needed %d any_count %d n_sysno %d\n",
>> > +			    pid, needed, any_count, n_sysno);
>> > +      if (needed)
>> > +	{
>> > +	  /* Prepare a packet with the sysno list, assuming
>> > +	     max 8+1 characters for a sysno. If the resulting
>> > +	     packet size is too big, fallback on the non
>> > +	     selective packet.  */
>> > +	  const char *q1 = "QCatchSyscalls:1";
>> 
>> Really nitpicking but you could avoid calling strlen for q1 and replace
>> it by a size_t variable holding the sizeof (q1) - 1.  But this is just
>> a minor thing.
>
> I cannot use sizeof(q1) but I could possibly do
>    const size_t q1len = strlen (q1);
> and use q1len after ?
> (that would however only spare a (very unlikely) second call
> to strlen, probably optimised by the compiler in any case).

As I said, just nitpicking :-).  I prefer to use sizeof and store the
result in a variable, but that's a matter of style.  I guess it's fine
as is.

>
>> > +	      p = catch_packet;
>> > +	      p += strlen(p);
>> > +	      for (i = 0; i < table_size; i++)
>> > +		{
>> > +		  if (table[i])
>> 
>> Again, I don't see the reason of this check.
> See above.
> Added a comment:
>        /* Add in catch_packet each syscall to be caught (table[i] != 0).  */

Thanks again.

>> 
>> > +		    {
>> > +		      xsnprintf(p, catch_packet + maxpktsz - p,
>> > +				";%x", i);
>> > +		      p += strlen(p);
>> > +		    }
>> > +		}
>> > +	    }
>> > +	  if (strlen(catch_packet) > get_remote_packet_size())
>> > +	    {
>> > +	      /* catch_packet too big. Fallback to less efficient
>> > +		 non selective mode, with GDB doing the filtering.  */
>> > +	      catch_packet[strlen (q1)] = 0;
>> > +	    }
>> 
>> Hm, can't you do this check before start filling catch_packet?  This
>> would save some time.
> Not easy to do (and saving probably very little cpu time in very
> unfrequent cases):
> The size of the packet depends on which syscalls
> are added, and not only on the nr of syscalls to add in the packet.
> E.g. a sysno can give either 2 or 3 or 4 characters (if we have
> sysno only below 999). 

OK, nevermind then.

>> > -  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
>> > +  /* Check if the target supports PTRACE_O_TRACESYSGOOD, keeping
>> > +     PTRACE_O_TRACEFORK option activated.  */
>> >    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
>> > -		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
>> > +		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
>> > +				    | PTRACE_O_TRACESYSGOOD));
>> 
>> Any reason why you are keeping PTRACE_O_TRACEFORK set?
> Changed the comment to:
>   /* 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.  */

Oh, OK, now it makes sense, I guess I stopped reading the code here.
Thanks for the comment.

>> > RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
>> >    if (ret == 0)
>> >      current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
>> >  
>> > +#ifdef GDBSERVER
>> > +  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
>> > +#else
>> >    /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
>> >    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
>> >  		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
>> > @@ -474,6 +475,11 @@ linux_enable_event_reporting (pid_t pid)
>> >  static int
>> >  ptrace_supports_feature (int ptrace_options)
>> >  {
>> > +  /* Check if we have initialized the ptrace features for this
>> > +     target.  If not, do it now.  */
>> > +  if (current_ptrace_options == -1)
>> > +    linux_check_ptrace_features ();
>> > +
>> 
>> This seems correct to me, but I think you should send it in a separate
>> patch, because it is not specific to catch syscall IMO.
> It looks better to me to keep it in this patch, as it was not needed
> before, but is now needed due to the "early calls" to
> ptrace_supports_feature needed for catch syscalls in gdbserver.

Well, it just seemed to me that doing this check and calling
linux_check_ptrace_features is the right thing to do, with or without
catch syscall.  I.e., it seems that someone forgot to make this check
here, and you're fixing this.  Anyway, leave it as is, let's wait until
someone more experienced comments about it.

>> > ===================================================================
>> > RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
>
>> > +static void
>> > +get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
>> > +{
>> > +  struct thread_info *saved_inferior;
>> > +  struct regcache *regcache;
>> > +
>> > +  if (the_low_target.get_syscall_trapinfo == NULL)
>> > +    {
>> > +      *sysno = 0;
>> > +      *sysret = 0;
>> 
>> Aren't you missing a "return" here?
> Yes, added it.
>> 
>> In fact, is there any way for "the_low_target.get_syscall_trapinfo" to
>> be NULL?  It seems to me that this is a necessary condition for this
>> function to be called (see my comment about putting an assert at
>> server.c:handle_general_set).  So maybe you should put an assert here to
>> check that the_low_target.get_syscall_trapinfo != NULL.  WDYT?
> Good question, I am not sure what to do.
> It looks like many  other calls to low level target
> are protected similarly (which is not a good enough reason).
> However, in this case, I suspect that if the GDB users forces
> the use of the QCatchSyscalls packet, that we might end up there:
> GDB will send such a packet, resulting in server.c setting
> catching_syscalls_p to 1, resulting to ptrace "syscall" to be activated
> resulting in a syscall trap event resulting in a call to getsyscall
> trapinfo.
> So, returning sysno 0 looks safe to me, to avoid a GDB user
> causing an assert due to forcing the use of a (normally)
> auto detected packet.

If the user can indeed force GDB to use QCatchSyscalls, then I agree
that the assert is not the right solution.  I.e., things are fine as is.

>> > +    return 0;
>> > +
>> > +  if (catched_syscalls_size == 0)
>> > +    return 1;
>> > +
>> > +  get_syscall_trapinfo (event_child, &sysno, &sysret);
>> > +  for (i = 0; i < catched_syscalls_size; i++)
>> > +    if (catched_syscalls[i] == sysno)
>> > +      return 1;
>> 
>> Also, I think the names of the variables are a bit misleading.
>> "catched_syscalls" does not represent syscalls that were caught, but
>> rather syscalls that are to be caught.  Maybe rename them to
>> "syscalls_to_be_caught" or something?
> Yes.
> Renamed to syscalls_to_catch_size and syscalls_to_catch.
> grep-ped for frenglish 'catched' to ensure I catched all errors :).

Hahaha, I swear I was going to correct you again, but then I realized it
is a joke :-P.

>> > Index: gdbserver/remote-utils.c
>> > ===================================================================
>> > RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
>> > retrieving revision 1.98
>> > diff -u -p -r1.98 remote-utils.c
>> > --- gdbserver/remote-utils.c	1 Jul 2013 11:19:27 -0000	1.98
>> > +++ gdbserver/remote-utils.c	30 Aug 2013 15:18:16 -0000
>> > @@ -1319,12 +1319,17 @@ prepare_resume_reply (char *buf, ptid_t 
>> >    switch (status->kind)
>> >      {
>> >      case TARGET_WAITKIND_STOPPED:
>> > +    case TARGET_WAITKIND_SYSCALL_ENTRY:
>> > +    case TARGET_WAITKIND_SYSCALL_RETURN:
>> >        {
>> >  	struct thread_info *saved_inferior;
>> >  	const char **regp;
>> >  	struct regcache *regcache;
>> >  
>> > -	sprintf (buf, "T%02x", status->value.sig);
>> > +	if (status->kind == TARGET_WAITKIND_STOPPED)
>> > +	  sprintf (buf, "T%02x", status->value.sig);
>> > +	else
>> > +	  sprintf (buf, "T%02x", SIGTRAP);
>> 
>> Again, maybe it's just me, but would you mind putting a comment here
>> explainig why you're using SIGTRAP directly if status->kind ==
>> TARGET_WAITKIND_SYSCALL_{ENTRY,RETURN} ?
> This is according to the protocol definition:
>         * If N is a recognized "stop reason", it describes a more
>           specific event that stopped the target.  The currently
>           defined stop reasons are listed below.  AA should be `05',
>           the trap signal.  At most one stop reason should be present.

Right, one more thing I learned :-).

>> >  	  {
>> > Index: gdbserver/server.c
>> > ===================================================================
>> > RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
>> > retrieving revision 1.196
>> > diff -u -p -r1.196 server.c
>> > --- gdbserver/server.c	28 Aug 2013 17:40:58 -0000	1.196
>> > +++ gdbserver/server.c	30 Aug 2013 15:18:17 -0000
>> > @@ -71,6 +71,9 @@ int debug_threads;
>> >  int debug_hw_points;
>> >  
>> >  int pass_signals[GDB_SIGNAL_LAST];
>> > +int catch_syscalls_p;
>> > +int catched_syscalls_size;
>> 
>> size_t?  :-)
> I guess an int will be ok unless we reach zillions
> of different syscalls, and then the protocol packet
> decoding code will suffer a lot :).

Fair enough.  I can't avoid suggesting such things, sorry :-).

>> I am not very familiar with the remote protocol, but IMHO you could do
>> an assert here to check if target_supports_catch_syscall returns 1,
>> WDYT?  IOW, we shouldn't get here if it returns 0, right?
>> 
>> Or is there some sort of mechanism intended to overwrite the initial
>> probing of features and force unsupported features to be enabled?  In
>> that case, I guess the assert would trigger every time...  Not sure what
>> to do.
> Yes, I suspect this is true (so has not added asserts).
> It is however not very clear to me why GDB allows to overwrite
> auto-detected packets, as this might effectively give
> unexpected results. But it looks better to not crash gdbserver
> following a "legal" user action.

Agreed.

>> > Index: testsuite/gdb.base/catch-syscall.exp
>> >
>> > +# This shall be updated whenever QCatchSyscalls packet support is implemented
>> > +# on some gdbserver architecture.
>> > +if { [is_remote target]
>> > +     && ![istarget "x86_64-*-linux*"] 
>> > +     && ![istarget "i\[34567\]86-*-linux*"] } {
>> > +    continue
>> > +}
>> 
>> Hm, am I too sleepy or this test does nothing at all?  :-).
> We both suspect we are too sleepy :).
> Looks to me the above is the expected condition, which is to "continue
> somewhere else" (whatever that "continue" exactly means in tcl/dejagnu,
> I have no idea :).
> This condition looks similar to equivalent conditions just before,
> that are skipping this test on non matching target.
> But my tcl/dejagnu knowledge is close to 0, so I might have missed
> something.

Yeah, you're right again :-).  My TCL-fu is not strong, as you might
have noticed.  I just tested here (should have done this before), and
"continue" is indeed just a way to say "stop testing this and go to the
next testcase".  So everything's right.

>> Otherwise, the patch looks pretty good to me!  I have tested it here and
>> as far as I have seen it is working OK.  I hope my comments help you
>> with getting the patch approved.  I am not a maintainer, so you will
>> need to wait for one to review the patch as well :-).
> Thanks for the extensive and helpful comments.
> I will retest and submit another patch version (I guess this WE).
> Any additional comment or feedback welcome in the meantime ...

Thank you.

-- 
Sergio