This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFA [PATCH v4] Implement 'catch syscall' for gdbserver (was Re: RFA [PATCH v3] Implement 'catch syscall' for gdbserver)


On 09/29/2013 04:04 PM, Philippe Waroquiers wrote:
> On Fri, 2013-09-27 at 17:55 -0300, Sergio Durigan Junior wrote:
>> Still have some formatting issues, as pointed by Tom on IRC.
> Thanks, handled all comments (formatting and others),
> just a question below about the -1 syscall nr.
> 
> 
>>> +  if (the_low_target.get_syscall_trapinfo == NULL)
>>> +    {
>>> +      *sysno = 0;
>>> +      *sysret = 0;
>>> +      return;
>>> +    }
>>
>> Just a reminder that you were going to check if -1 is a better value to
>> represent this case.  And if it is, then it should be documented, of course.
> Tested with -1 (on x86_64), and it works.
> I would have liked to re-use UNKNOWN_SYSCALL defined in gdbarch.h, but
> this is not included in gdbserver, and it sounds strange to me to
> include it to just get this constant.
> So, for the moment, I have just used -1 directly. 
> Is there a correct place to define this constant ?
> 
> Also, not sure what is meant by 'should be documented'.
> Do you mean add a sentence in the protocol documentation such as:
> "In case the remote stub cannot determine the syscall number,
> the remote stub must send -1 as syscall nr (encoded in hex,
> e.g. ffffffff)."

Please don't ever send back ffffffff as meaning -1.
Numbers in the RSP are variable length hex.  You can't assume the
host side interprets them as two's complement 32-bit integers.  If
sending a negative number, the '-' sign needs to be there explicitly
in the packet, like '-1'.  Perhaps it would even be better to leave
-1 as implementation detail, and at the protocol level send something
else, like "unknown" or just "-", or nothing.

> (do we really want that to be part of the protocol spec ?
> This is really a very special case caused by a user "strange"
> manipulation of forcing to use the packet QCatchSyscalls).

Yes, please specify how that should work.  It can happen, so
better define it.  It should also be possible for the server to be
dumb, and have GDB itself figure out the syscall number.  GDB
could read the registers itself, just like gdbserver is currently
doing.  I'd assume the -ENOSYS trick might not always work on
all archs, for instance?

> 
> 
> In the meantime, find below the version 4 of the patch implementing
> catch syscall for gdbserver.
> The changes compared to v3 are:
>   * handles the additional review comments given by Sergio Durigan Junior
>     In particular, in case the syscall nr cannot be fetched from the low
>     linux target, returns -1 syscall nr.
>   * removes the useless "syscall" field in the stop_reply (i.e. similar
>     to the removal of fields solibs_changes/replay_event done by Pedro Alves).
>   * formatting changes (I hope I got the formatting all correct this time).
> 
> Tested on x86_64, using:
>   for t in "" "--target_board native-extended-gdbserver" "--target_board native-gdbserver"
>   do
>      make check RUNTESTFLAGS="$t"
>      ....
>   done
> (still without libexpat, waiting for Pedro's patch to be committed).
> 
> Thanks ...
> 
> Philippe
> 
> 
> ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* NEWS: Document new QcatchSyscalls packet and its use
> 	in x86/amd64 linux gdbserver and Valgrind gdbserver.

QCatch not Qcatch.

> 	* remote.c (PACKET_QCatchSyscalls): New.
> 	(remote_protocol_features): Add QcatchSyscalls.

Ditto.

> 	(remote_set_syscall_catchpoint): New function.
> 	(remote_parse_stop_reply): New stop reasons syscall_entry
> 	and syscall_return.
> 	(init_remote_ops): Registers remote_set_syscall_catchpoint
> 	and the config commands for PACKET_QCatchSyscalls.
> 	* common/linux-ptrace.c (linux_check_ptrace_features):
> 	Detect PTRACE_O_TRACESYSGOOD for gdbserver.
> 	(ptrace_supports_feature): Initializes ptrace features if needed.

s/Initializes/Initialize/

> 
> doc/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.texinfo (General Query Packets): Document new QCatchSyscalls
> 	packet.
> 	(Stop Reply Packets): Document new stop reasons syscall_entry and
> 	syscall_return.
> 	(Async Records): Fixed syscall-return item name.

s/Fixed/Fix/.

> 
> gdbserver/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* target.h (struct target_ops): Add supports_catch_syscall operation.

s/operation/field/.

> 	* linux-low.h (struct linux_target_ops): Add get_syscall_trapinfo
> 	operation.

Ditto.

> 	* linux-low.c (linux_wait_1): Enable, detect and handle
> 	SYSCALL_SIGTRAP.
> 	(gdb_catch_this_syscall_p): New function.
> 	(get_syscall_trapinfo): New function.
>  	(linux_supports_catch_syscall): New function.
> 	(struct target_ops linux_target_ops): Set linux_supports_catch_syscall.

Just write (linux_target_ops).

> 	* linux-x86-low.c (x86_get_syscall_trapinfo): New function.
> 	(struct linux_target_ops the_low_target): Set x86_get_syscall_trapinfo.

 	(the_low_target): Set x86_get_syscall_trapinfo.


> 	* remote-utils.c (prepare_resume_reply): Handle status kinds
> 	TARGET_WAITKIND_SYSCALL_ENTRY and TARGET_WAITKIND_SYSCALL_RETURN.
> 	* server.h: Declare catching_syscalls_p, syscalls_to_catch_size and
> 	syscalls_to_catch.

 	* server.h (catching_syscalls_p, syscalls_to_catch_size)
 	(syscalls_to_catch): Declare.


> 	* server.c: Define catching_syscalls_p, syscalls_to_catch_size and
> 	syscalls_to_catch.

 	* server.c (catching_syscalls_p, syscalls_to_catch_size)
 	(syscalls_to_catch): Define.


> 	(handle_general_set): Handle QCatchSyscalls packet.
> 	(handle_query): Reports if low target supports QCatchSyscalls.

s/Reports/Report/

> 	* win32-low.c (struct target_ops win32_target_op): Sets NULL
> 	for supports_catch_syscall.

	* win32-low.c (win32_target_op): Set supports_catch_syscall to
	NULL.


> 
> testsuite/ChangeLog
> 2013-xx-yy  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.base/catch-syscall.exp: Enables test for x86 and amd64
> 	gdbserver.

s/Enables/Enable/.

> 
> 
> Index: NEWS
> ===================================================================
> RCS file: /cvs/src/src/gdb/NEWS,v
> retrieving revision 1.616
> diff -u -p -r1.616 NEWS
> --- NEWS	25 Sep 2013 23:17:11 -0000	1.616
> +++ NEWS	29 Sep 2013 14:20:27 -0000
> @@ -157,11 +157,20 @@ qXfer:libraries-svr4:read's annex
>    necessary for library list updating, resulting in significant
>    speedup.
>  
> +QCatchSyscalls:1 [;SYSNO]...
> +QCatchSyscalls:0
> +  Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
> +  catching syscalls from the inferior process.

So, "catch syscall" is per-inferior/process on the GDB side, but
this always sets the catchpoints on all processes.  Was that
intended?

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

Really, Valgrind news don't belong here.

> +
>    ** GDBserver now adds element 'tvar' in the XML in the reply to
>       'qXfer:traceframe-info:read'.  It has the id of the collected
>       trace state variables.
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.579
> diff -u -p -r1.579 remote.c
> --- remote.c	27 Sep 2013 15:29:06 -0000	1.579
> +++ remote.c	29 Sep 2013 14:20:29 -0000
> @@ -1340,6 +1340,7 @@ enum {
>    PACKET_qSupported,
>    PACKET_qTStatus,
>    PACKET_QPassSignals,
> +  PACKET_QCatchSyscalls,
>    PACKET_QProgramSignals,

Odd place to put this.  Doesn't see to follow any logic?
(suspect merge-conflict resolution).  PACKET_QPassSignals
and QProgramSignals are related.  At least please leave them
together.

>    PACKET_qSearch_memory,
>    PACKET_vAttach,
> @@ -1728,6 +1729,98 @@ remote_pass_signals (int numsigs, unsign
>      }
>  }
>  
> +/* If 'QCatchSyscalls' is supported, tell the remote stub
> +   to report syscalls to GDB.  */
> +
> +static int
> +remote_set_syscall_catchpoint (int pid, int needed, int any_count,
> +			       int table_size, int *table)
> +{
> +  char *catch_packet, *p;
> +  enum packet_result result;
> +  int n_sysno = 0;
> +
> +  if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
> +    {
> +      /* Not supported.  */
> +      return 1;
> +    }
> +
> +  if (needed && !any_count)

  if (needed && any_count == 0)


> +    {
> +      int i;
> +
> +      /* Count how many syscalls are to be caught (table[sysno] != 0).  */
> +      for (i = 0; i < table_size; i++)
> +	if (table[i])

You got the style right in the comment, but not in the code.
You don't really need that bit in the comment though, it's clear
already from the code:

     /* Count how many syscalls are to be caught.  */
      for (i = 0; i < table_size; i++)
	if (table[i] != 0)

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

This creates a string, and then a const pointer that points
to the string.  If you declare it like this:

      const char q1[] = "QCatchSyscalls:1";

you spare creating a pointer.

> +      int i;
> +      const int maxpktsz = strlen (q1) + n_sysno * 9 + 1;

And then, Sergio's (IIRC) suggestion would work:

      const int maxpktsz = sizeof (q1) - 1 + n_sysno * 9 + 1;

> +
> +      catch_packet = xmalloc (maxpktsz);
> +      strcpy (catch_packet, q1);
> +      if (!any_count)

      if (any_count == 0)


> +	{
> +	  char *p;
> +
> +	  p = catch_packet;
> +	  p += strlen (p);
> +
> +	  /* Add in catch_packet each syscall to be caught (table[i] != 0).  */
> +	  for (i = 0; i < table_size; i++)
> +	    {
> +	      if (table[i])

Same comment as above.

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

(
	  catch_packet[sizeof (q1) - 1] = 0;
etc.
)

> +	}
> +    }
> +  else
> +    {
> +      catch_packet = xmalloc (strlen ("QCatchSyscalls:0") + 1);
> +      strcpy (catch_packet, "QCatchSyscalls:0");

Just write:

      catch_packet = xstrdup ("QCatchSyscalls:0");

> +    }
> +
> +  {
> +    struct remote_state *rs = get_remote_state ();
> +    char *buf = rs->buf;
> +
> +    putpkt (catch_packet);
> +    getpkt (&rs->buf, &rs->buf_size, 0);
> +    result = packet_ok (buf,
> +			&remote_protocol_packets[PACKET_QCatchSyscalls]);
> +    xfree (catch_packet);

This should be done in a cleanup instead.  putpkt/getpkt can throw.

> +    if (result == PACKET_OK)
> +      return 0;
> +    else
> +      return -1;
> +  }
> +}
> +
> +
> +
>  /* If 'QProgramSignals' is supported, tell the remote stub what
>     signals it should pass through to the inferior when detaching.  */
>  
> @@ -4016,6 +4109,8 @@ static const struct protocol_feature rem
>      PACKET_qXfer_traceframe_info },
>    { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QPassSignals },
> +  { "QCatchSyscalls", PACKET_DISABLE, remote_supported_packet,
> +    PACKET_QCatchSyscalls },
>    { "QProgramSignals", PACKET_DISABLE, remote_supported_packet,
>      PACKET_QProgramSignals },
>    { "QStartNoAckMode", PACKET_DISABLE, remote_supported_packet,
> @@ -5591,6 +5686,22 @@ Packet: '%s'\n"),
>  		       p, buf);
>  	      if (strncmp (p, "thread", p1 - p) == 0)
>  		event->ptid = read_ptid (++p1, &p);
> +	      else if (strncmp (p, "syscall_entry", p1 - p) == 0)
> +		{
> +		  ULONGEST sysno;
> +
> +		  event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> +		  p = unpack_varlen_hex (++p1, &sysno);
> +		  event->ws.value.syscall_number = (int) sysno;
> +		}
> +	      else if (strncmp (p, "syscall_return", p1 - p) == 0)
> +		{
> +		  ULONGEST sysno;
> +
> +		  event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
> +		  p = unpack_varlen_hex (++p1, &sysno);
> +		  event->ws.value.syscall_number = (int) sysno;
> +		}
>  	      else if ((strncmp (p, "watch", p1 - p) == 0)
>  		       || (strncmp (p, "rwatch", p1 - p) == 0)
>  		       || (strncmp (p, "awatch", p1 - p) == 0))
> @@ -11445,6 +11556,7 @@ Specify the serial device it is connecte
>    remote_ops.to_load = generic_load;
>    remote_ops.to_mourn_inferior = remote_mourn;
>    remote_ops.to_pass_signals = remote_pass_signals;
> +  remote_ops.to_set_syscall_catchpoint = remote_set_syscall_catchpoint;
>    remote_ops.to_program_signals = remote_program_signals;
>    remote_ops.to_thread_alive = remote_thread_alive;
>    remote_ops.to_find_new_threads = remote_threads_info;
> @@ -11939,6 +12051,9 @@ Show the maximum size of the address (in
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
>  			 "QPassSignals", "pass-signals", 0);
>  
> +  add_packet_config_cmd (&remote_protocol_packets[PACKET_QCatchSyscalls],
> +			 "QCatchSyscalls", "catch-syscalls", 0);
> +
>    add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
>  			 "QProgramSignals", "program-signals", 0);
>  
> Index: common/linux-ptrace.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/common/linux-ptrace.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 linux-ptrace.c
> --- common/linux-ptrace.c	28 Aug 2013 14:09:31 -0000	1.12
> +++ common/linux-ptrace.c	29 Sep 2013 14:20:29 -0000
> @@ -361,16 +361,18 @@ linux_check_ptrace_features (void)
>        return;
>      }
>  
> -#ifdef GDBSERVER
> -  /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
> -     PTRACE_O_TRACEVFORKDONE yet.  */
> -#else
> -  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
> +  /* Check if the target supports PTRACE_O_TRACESYSGOOD.
> +     We keep PTRACE_O_TRACEFORK option activated as a fork
> +     event notification is expected after my_waitpid below.  */
>    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> -		(PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> +		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> +				    | PTRACE_O_TRACESYSGOOD));

This was discussed in a separate email.

>    if (ret == 0)
>      current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
>  
> +#ifdef GDBSERVER
> +  /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet.  */
> +#else
>    /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
>    ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
>  		(PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
> @@ -474,6 +476,11 @@ linux_enable_event_reporting (pid_t pid)
>  static int
>  ptrace_supports_feature (int ptrace_options)
>  {
> +  /* Check if we have initialized the ptrace features for this
> +     target.  If not, do it now.  */
> +  if (current_ptrace_options == -1)
> +    linux_check_ptrace_features ();
> +
>    gdb_assert (current_ptrace_options >= 0);
>  
>    return ((current_ptrace_options & ptrace_options) == ptrace_options);
> Index: doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.1113
> diff -u -p -r1.1113 gdb.texinfo
> --- doc/gdb.texinfo	25 Sep 2013 23:17:12 -0000	1.1113
> +++ doc/gdb.texinfo	29 Sep 2013 14:20:34 -0000
> @@ -18752,6 +18752,10 @@ are:
>  @tab @code{qSupported}
>  @tab Remote communications parameters
>  
> +@item @code{catch-syscalls}
> +@tab @code{QCatchSyscalls}
> +@tab @code{catch syscall}
> +
>  @item @code{pass-signals}
>  @tab @code{QPassSignals}
>  @tab @code{handle @var{signal}}
> @@ -38153,6 +38157,11 @@ The currently defined stop reasons are:
>  The packet indicates a watchpoint hit, and @var{r} is the data address, in
>  hex.
>  
> +@item syscall_entry
> +@itemx syscall_return
> +The packet indicates a syscall entry or return, and @var{r} is the 
> +syscall number, in hex.
> +
>  @cindex shared library events, remote reply
>  @item library
>  The packet indicates that the loaded libraries have changed.
> @@ -38523,6 +38532,44 @@ by supplying an appropriate @samp{qSuppo
>  Use of this packet is controlled by the @code{set non-stop} command; 
>  @pxref{Non-Stop Mode}.
>  
> +@item QCatchSyscalls:1 @r{[};@var{sysno}@r{]}@dots{}
> +@itemx QCatchSyscalls:0
> +@cindex catch syscalls from inferior, remote request
> +@cindex @samp{QCatchSyscalls} packet
> +@anchor{QCatchSyscalls}
> +Enable (@samp{QCatchSyscalls:1}) or disable (@samp{QCatchSyscalls:0})
> +catching syscalls from the inferior process.
> +
> +For @samp{QCatchSyscalls:1}, each listed syscall @var{sysno} (encoded
> +in hex) should be reported to @value{GDBN}.  If no syscall @var{sysno}
> +is listed, every system call should be reported.
> +
> +Note that if a syscall not member of the list is reported, @value{GDBN}
> +will filter it if this syscall is not caught.  It is however more efficient
> +to only report the needed syscalls.

I think "if this syscall is not caught" is a little confusing.  I'd suggest:

"Note that if a syscall not member of the list is reported, @value{GDBN}
 will still filter it out.  It is however more efficient to only report
the requested syscalls."

> +
> +Multiple @samp{QCatchSyscalls:1} packets do not
> +combine; any earlier @samp{QCatchSyscalls:1} list is completely replaced by the
> +new list.
> +
> +Reply:
> +@table @samp
> +@item OK
> +The request succeeded.
> +
> +@item E @var{nn}
> +An error occurred.  @var{nn} are hex digits.
> +
> +@item @w{}
> +An empty reply indicates that @samp{QCatchSyscalls} is not supported by
> +the stub.
> +@end table
> +
> +Use of this packet is controlled by the @code{set remote catch-syscalls}
> +command (@pxref{Remote Configuration, set remote catch-syscalls}).
> +This packet is not probed by default; the remote stub must request it,
> +by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
> +
>  @item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
>  @cindex pass signals to inferior, remote request
>  @cindex @samp{QPassSignals} packet
> @@ -38886,6 +38933,11 @@ These are the currently defined stub fea
>  @tab @samp{-}
>  @tab Yes
>  
> +@item @samp{QCatchSyscalls}
> +@tab No
> +@tab @samp{-}
> +@tab Yes
> +
>  @item @samp{QPassSignals}
>  @tab No
>  @tab @samp{-}
> @@ -39046,6 +39098,10 @@ packet (@pxref{qXfer fdpic loadmap read}
>  The remote stub understands the @samp{QNonStop} packet
>  (@pxref{QNonStop}).
>  
> +@item QCatchSyscalls
> +The remote stub understands the @samp{QCatchSyscalls} packet
> +(@pxref{QCatchSyscalls}).
> +
>  @item QPassSignals
>  The remote stub understands the @samp{QPassSignals} packet
>  (@pxref{QPassSignals}).
> Index: gdbserver/linux-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
> retrieving revision 1.248
> diff -u -p -r1.248 linux-low.c
> --- gdbserver/linux-low.c	5 Sep 2013 20:45:39 -0000	1.248
> +++ gdbserver/linux-low.c	29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,11 @@
>  #define W_STOPCODE(sig) ((sig) << 8 | 0x7f)
>  #endif
>  
> +/* Unlike other extended result codes, WSTOPSIG (status) on
> +   PTRACE_O_TRACESYSGOOD syscall events doesn't return SIGTRAP, but
> +   instead SIGTRAP with bit 7 set.  */
> +#define SYSCALL_SIGTRAP (SIGTRAP | 0x80)
> +
>  /* This is the kernel's hard limit.  Not to be confused with
>     SIGRTMIN.  */
>  #ifndef __SIGRTMIN
> @@ -481,6 +486,38 @@ get_pc (struct lwp_info *lwp)
>    return pc;
>  }
>  
> +/* This function should only be called if LWP got a SIGTRAP_SYSCALL.
> +   Fill *SYSNO with the syscall nr trapped.  Fill *SYSRET with the
> +   return code.  */
> +
> +static void
> +get_syscall_trapinfo (struct lwp_info *lwp, int *sysno, int *sysret)
> +{
> +  struct thread_info *saved_inferior;
> +  struct regcache *regcache;
> +
> +  if (the_low_target.get_syscall_trapinfo == NULL)
> +    {
> +      /* If we cannot get the syscall trapinfo, report an unknown
> +	 system call using -1 values.  */
> +      *sysno = -1;
> +      *sysret = -1;
> +      return;
> +    }
> +
> +  saved_inferior = current_inferior;
> +  current_inferior = get_lwp_thread (lwp);
> +
> +  regcache = get_thread_regcache (current_inferior, 1);
> +  (*the_low_target.get_syscall_trapinfo) (regcache, sysno, sysret);
> +
> +  if (debug_threads)
> +    fprintf (stderr, "get_syscall_trapinfo sysno %d sysret %d\n",
> +	     *sysno, *sysret);
> +
> +  current_inferior = saved_inferior;
> +}
> +
>  /* This function should only be called if LWP got a SIGTRAP.
>     The SIGTRAP could mean several things.
>  
> @@ -2248,6 +2285,29 @@ linux_stabilize_threads (void)
>      }
>  }
>  
> +/* Returns 1 if GDB is interested in the event_child syscall.

> +/* Returns 1 if GDB is interested in EVENT_CHILD's syscall.

> +   Only to be called when stopped reason is SIGTRAP_SYSCALL.  */
> +
> +static int
> +gdb_catch_this_syscall_p (struct lwp_info *event_child)

"this" looks odd, as a number is not passed as argument.
"gdb catch this syscall" doesn't really parse correctly.
I'd suggest: gdb_catching_syscall, gdb_catch_syscall_p,
or, gdb_interested_in_syscall_p.  (the latter as there's
no need to describe this with "interested" everywhere
in comments, and then use another term in the function
name).

> +{
> +  int i;
> +  int sysno, sysret;
> +
> +  if (!catching_syscalls_p)
> +    return 0;
> +
> +  if (syscalls_to_catch_size == 0)
> +    return 1;
> +
> +  get_syscall_trapinfo (event_child, &sysno, &sysret);
> +  for (i = 0; i < syscalls_to_catch_size; i++)
> +    if (syscalls_to_catch[i] == sysno)
> +      return 1;
> +
> +  return 0;
> +}
> +
>  /* Wait for process, returns status.  */
>  
>  static ptid_t
> @@ -2529,6 +2589,19 @@ Check if we're already there.\n",
>  
>    /* Check whether GDB would be interested in this event.  */
>  
> +  /* Check if GDB is interested in this syscall.  */
> +  if (WIFSTOPPED (w)
> +      && WSTOPSIG (w) == SYSCALL_SIGTRAP 
> +      && !gdb_catch_this_syscall_p (event_child))
> +    {
> +      if (debug_threads)
> +	fprintf (stderr, "Ignored syscall for LWP %ld.\n",
> +		 lwpid_of (event_child));
> +      linux_resume_one_lwp (event_child, event_child->stepping,
> +			    0, NULL);
> +      goto retry;
> +    }
> +
>    /* If GDB is not interested in this signal, don't stop other
>       threads, and don't report it to GDB.  Just resume the inferior
>       right away.  We do this for threading-related signals as well as
> @@ -2707,7 +2780,24 @@ Check if we're already there.\n",
>  
>    ourstatus->kind = TARGET_WAITKIND_STOPPED;
>  
> -  if (current_inferior->last_resume_kind == resume_stop
> +  if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
> +    {
> +      int sysret;
> +
> +      get_syscall_trapinfo (event_child,
> +			    &ourstatus->value.syscall_number, &sysret);
> +      /* Differentiate entry from return using the return code
> +	 -ENOSYS.  This is not ideal as a syscall return from a not
> +	 implemented syscall will be seen as an entry.  However, this
> +	 resists well to enabling/disabling catch syscall at various
> +	 moment.  A better way to differentiate entry/return in GDB
> +         and GDBSERVER would be nice.  */

Indentation is wrong.

> +      if (sysret == -ENOSYS)
> +	ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> +      else
> +	ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;

The whole 'SIGTRAP | 0x80' thing is super silly.  The ideal way
would be for the kernel to tell us.  See:

 https://sourceware.org/gdb/wiki/LinuxKernelWishList

"A PTRACE_O_SYSCALL(_ENTER|_EXIT) ptrace option, along with
 PTRACE_EVENT_SYSCALL(_ENTER|_EXIT).  PTRACE_SYSCALL/sysgood doesn't leave
 any way to trace syscalls when single-stepping.  Separate syscall enter
 and exit events would just be nice, to avoid having to keep track of
 enter/exit - the kernel knows, might as well give us the info. (While
 at it, a way to filter which syscalls trigger events would be nice to
 avoid unnecessary slowing down the inferior until it calls the syscall the user
 is interested in (you can catch specific syscalls with "catch syscall foosyscall";
 gdb currently filters after the fact), though that's obviously a
 bit more work.)"

Want to work on that?  It'd be great...

Another idea would be, in addition to supporting syscall_entry
and syscall_exit stop replies, also support plain "syscall" (and
a new TARGET_WAITKIND_SYSCALL), meaning, "I don't know if it was
an entry or an exit, you decide." (just like SYSCALL_SIGTRAP), and then
we'd leave GDB to decide.  GDB will end up fetching registers
off the target anyway, so I think I'd end up costing 0 in terms of
performance.

Then when we have real PTRACE_EVENT_SYSCALL(_ENTER|_EXIT), we'd
use syscall_entry/syscall_exit.

(not completely sure about this).

How portable is this -ENOSYS thing?  What does strace do?


BTW, it'd be very very nice if GDB and GDBserver didn't end
up with different mechanisms for this.  We've been trying to
make the gdb and gdbserver backends more similar, with the end
goal of merging them, and more points of divergence don't really
help that.

> +    }
> +  else if (current_inferior->last_resume_kind == resume_stop
>        && WSTOPSIG (w) == SIGSTOP)
>      {
>        /* A thread that has been requested to stop by GDB with vCont;t,
> @@ -3098,6 +3188,7 @@ linux_resume_one_lwp (struct lwp_info *l
>  {
>    struct thread_info *saved_inferior;
>    int fast_tp_collecting;
> +  int ptrace_request;
>  
>    if (lwp->stopped == 0)
>      return;
> @@ -3269,7 +3360,14 @@ lwp %ld wants to get out of fast tracepo
>    lwp->stopped = 0;
>    lwp->stopped_by_watchpoint = 0;
>    lwp->stepping = step;
> -  ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (lwp),
> +  if (step)
> +    ptrace_request = PTRACE_SINGLESTEP;
> +  else if (catching_syscalls_p)
> +    ptrace_request =  PTRACE_SYSCALL;
> +  else
> +    ptrace_request =  PTRACE_CONT;
> +  ptrace (ptrace_request, 
> +	  lwpid_of (lwp),
>  	  (PTRACE_TYPE_ARG3) 0,
>  	  /* Coerce to a uintptr_t first to avoid potential gcc warning
>  	     of coercing an 8 byte integer to a 4 byte pointer.  */
> @@ -5108,6 +5206,13 @@ linux_process_qsupported (const char *qu
>  }
>  
>  static int
> +linux_supports_catch_syscall (void)
> +{
> +  return (the_low_target.get_syscall_trapinfo != NULL
> +	  && linux_supports_tracesysgood());

Missing space "linux_supports_tracesysgood()".

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

Spare the reader unnecessary effort and spell out "number".

> +     return code.  Only to be called when inferior is stopped
> +     due to SYSCALL_SIGTRAP.  */
> +  void (*get_syscall_trapinfo) (struct regcache *regcache,
> +				int *sysno, int *sysret);
> +
>    /* Returns true if the low target supports tracepoints.  */
>    int (*supports_tracepoints) (void);
>  
> Index: gdbserver/linux-x86-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/linux-x86-low.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 linux-x86-low.c
> --- gdbserver/linux-x86-low.c	5 Sep 2013 20:40:58 -0000	1.51
> +++ gdbserver/linux-x86-low.c	29 Sep 2013 14:20:35 -0000
> @@ -1474,6 +1474,36 @@ x86_arch_setup (void)
>    current_process ()->tdesc = x86_linux_read_description ();
>  }
>  
> +/* Fill *SYSNO and *SYSRET with the syscall nr trapped and the syscall return

Likewise.

> +   code.  This should only be called if LWP got a SIGTRAP_SYSCALL.  */

Write "if the LWP".  "if LWP" kind of implies there's an LWP
parameter, while there's no such thing.

> +
> +static void
> +x86_get_syscall_trapinfo (struct regcache *regcache, int *sysno, int *sysret)
> +{
> +  int use_64bit = register_size (regcache->tdesc, 0) == 8;
> +
> +  if (use_64bit)
> +    {
> +      long l_sysno;
> +      long l_sysret;
> +
> +      collect_register_by_name (regcache, "orig_rax", &l_sysno);
> +      collect_register_by_name (regcache, "rax", &l_sysret);
> +      *sysno = (int) l_sysno;
> +      *sysret = (int) l_sysret;
> +    }
> +  else
> +    {
> +      int l_sysno;
> +      int l_sysret;
> +
> +      collect_register_by_name (regcache, "orig_eax", &l_sysno);
> +      collect_register_by_name (regcache, "eax", &l_sysret);
> +      *sysno = (int) l_sysno;
> +      *sysret = (int) l_sysret;
> +    }

This else branch can be simplified to:

   {
      collect_register_by_name (regcache, "orig_eax", sysno);
      collect_register_by_name (regcache, "eax", sysret);
   }

> +}
> +
>  static int
>  x86_supports_tracepoints (void)
>  {
> @@ -3323,6 +3353,7 @@ struct linux_target_ops the_low_target =
>    x86_linux_new_thread,
>    x86_linux_prepare_to_resume,
>    x86_linux_process_qsupported,
> +  x86_get_syscall_trapinfo,
>    x86_supports_tracepoints,
>    x86_get_thread_area,
>    x86_install_fast_tracepoint_jump_pad,
> Index: gdbserver/remote-utils.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 remote-utils.c
> --- gdbserver/remote-utils.c	5 Sep 2013 20:41:55 -0000	1.99
> +++ gdbserver/remote-utils.c	29 Sep 2013 14:20:35 -0000
> @@ -1321,12 +1321,17 @@ prepare_resume_reply (char *buf, ptid_t 
>    switch (status->kind)
>      {
>      case TARGET_WAITKIND_STOPPED:
> +    case TARGET_WAITKIND_SYSCALL_ENTRY:
> +    case TARGET_WAITKIND_SYSCALL_RETURN:
>        {
>  	struct thread_info *saved_inferior;
>  	const char **regp;
>  	struct regcache *regcache;
>  
> -	sprintf (buf, "T%02x", status->value.sig);
> +	if (status->kind == TARGET_WAITKIND_STOPPED)
> +	  sprintf (buf, "T%02x", status->value.sig);
> +	else
> +	  sprintf (buf, "T%02x", SIGTRAP);

SIGTRAP is host-dependent, while this is an RSP signal number.
Write:
	  sprintf (buf, "T%02x", GDB_SIGNAL_TRAP);

In practice, SIGTRAP==5==GDB_SIGNAL_TRAP just about
everywhere, but it's always better to write what we mean.

>  	buf += strlen (buf);
>  
>  	saved_inferior = current_inferior;
> @@ -1337,6 +1342,16 @@ prepare_resume_reply (char *buf, ptid_t 
>  
>  	regcache = get_thread_regcache (current_inferior, 1);
>  
> +	if (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +	    || status->kind == TARGET_WAITKIND_SYSCALL_RETURN)
> +	  {
> +	    sprintf (buf, "%s:%x;",
> +		     status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +		     ? "syscall_entry" : "syscall_return",
> +		     status->value.syscall_number);
> +	    buf += strlen (buf);
> +	  }
> +	  
>  	if (the_target->stopped_by_watchpoint != NULL
>  	    && (*the_target->stopped_by_watchpoint) ())
>  	  {
> Index: gdbserver/server.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
> retrieving revision 1.201
> diff -u -p -r1.201 server.c
> --- gdbserver/server.c	18 Sep 2013 01:59:59 -0000	1.201
> +++ gdbserver/server.c	29 Sep 2013 14:20:35 -0000
> @@ -74,6 +74,9 @@ int debug_threads;
>  int debug_hw_points;
>  
>  int pass_signals[GDB_SIGNAL_LAST];
> +int catching_syscalls_p;

No need for the "_p" suffix.  "catching_syscalls" is
already clearly a predicate.

> +int syscalls_to_catch_size;
> +int *syscalls_to_catch;
>  int program_signals[GDB_SIGNAL_LAST];
>  int program_signals_p;

Odd positioning...  I suspect you got that by resolving
a conflict.  Please leave pass_signals and program_signals
together, as they're related.

> @@ -511,6 +514,46 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (strncmp ("QCatchSyscalls:1", own_buf, strlen ("QCatchSyscalls:1")) == 0)
> +    {
> +      int i;
> +      const char *p;
> +      CORE_ADDR sysno;
> +
> +      catching_syscalls_p = 1;
> +      if (syscalls_to_catch != NULL)
> +	{
> +	  xfree (syscalls_to_catch);
> +	  syscalls_to_catch = NULL;
> +	}
> +      syscalls_to_catch_size = 0;

      catching_syscalls_p = 1;
      xfree (syscalls_to_catch);
      syscalls_to_catch = NULL;
      syscalls_to_catch_size = 0;

> +      p = own_buf + strlen("QCatchSyscalls:1");

Missing space.

> +      if (syscalls_to_catch_size > 0)
> +	{
> +	  syscalls_to_catch = xmalloc (syscalls_to_catch_size * sizeof (int));
> +	  p = strchr(own_buf, ';') + 1;

Above you used 'p = own_buf + strlen ("QCatchSyscalls:1")'.
Please do the same here so we're consistent.

> +	  for (i = 0; i < syscalls_to_catch_size; i++)
> +	    {
> +	      p = decode_address_to_semicolon (&sysno, p);
> +	      syscalls_to_catch[i] = (int) sysno;
> +	    }
> +	}
> +      strcpy (own_buf, "OK");
> +      return;
> +    }
> +
> +  if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> +    {
> +      catching_syscalls_p = 0;
> +      strcpy (own_buf, "OK");
> +      return;
> +    }
> +
>    if (strncmp ("QProgramSignals:", own_buf, strlen ("QProgramSignals:")) == 0)
>      {
>        int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -1744,6 +1787,9 @@ handle_query (char *own_buf, int packet_
>  	       "PacketSize=%x;QPassSignals+;QProgramSignals+",
>  	       PBUFSIZ - 1);
>  
> +      if (target_supports_catch_syscall ())
> +	strcat (own_buf, ";QCatchSyscalls+");
> +
>        if (the_target->qxfer_libraries_svr4 != NULL)
>  	strcat (own_buf, ";qXfer:libraries-svr4:read+"
>  		";augmented-libraries-svr4-read+");
> Index: gdbserver/server.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
> retrieving revision 1.118
> diff -u -p -r1.118 server.h
> --- gdbserver/server.h	5 Sep 2013 20:45:39 -0000	1.118
> +++ gdbserver/server.h	29 Sep 2013 14:20:35 -0000
> @@ -115,6 +115,15 @@ extern int server_waiting;
>  extern int debug_threads;
>  extern int debug_hw_points;
>  extern int pass_signals[];
> +
> +/* Set to 1 if GDB is catching some (or all) syscalls, zero otherwise.  */
> +extern int catching_syscalls_p;
> +/* syscalls_to_catch is the list of syscalls to report to GDB.
> +   If catching_syscalls_p and syscalls_to_catch == NULL, it means
> +   all syscalls must be reported.  */
> +extern int syscalls_to_catch_size;
> +extern int *syscalls_to_catch;
> +
>  extern int program_signals[];
>  extern int program_signals_p;

Likewise, keep pass_signals and program_signals together please.

>  
> Index: gdbserver/target.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
> retrieving revision 1.71
> diff -u -p -r1.71 target.h
> --- gdbserver/target.h	5 Sep 2013 20:41:22 -0000	1.71
> +++ gdbserver/target.h	29 Sep 2013 14:20:35 -0000
> @@ -268,6 +268,10 @@ struct target_ops
>    /* Target specific qSupported support.  */
>    void (*process_qsupported) (const char *);
>  
> +  /* Return 1 if the target supports catch syscall, 0 (or leave the
> +     callback NULL) otherwise.  */
> +  int (*supports_catch_syscall) (void);
> +
>    /* Return 1 if the target supports tracepoints, 0 (or leave the
>       callback NULL) otherwise.  */
>    int (*supports_tracepoints) (void);
> @@ -414,6 +418,10 @@ int kill_inferior (int);
>  	the_target->process_qsupported (query);		\
>      } while (0)
>  
> +#define target_supports_catch_syscall()              	\
> +  (the_target->supports_catch_syscall ?			\
> +   (*the_target->supports_catch_syscall) () : 0)
> +
>  #define target_supports_tracepoints()			\
>    (the_target->supports_tracepoints			\
>     ? (*the_target->supports_tracepoints) () : 0)
> Index: gdbserver/win32-low.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 win32-low.c
> --- gdbserver/win32-low.c	5 Sep 2013 20:45:39 -0000	1.69
> +++ gdbserver/win32-low.c	29 Sep 2013 14:20:35 -0000
> @@ -1824,6 +1824,7 @@ static struct target_ops win32_target_op
>    NULL, /* core_of_thread */
>    NULL, /* read_loadmap */
>    NULL, /* process_qsupported */
> +  NULL, /* supports_catch_syscall */
>    NULL, /* supports_tracepoints */
>    NULL, /* read_pc */
>    NULL, /* write_pc */
> Index: testsuite/gdb.base/catch-syscall.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/catch-syscall.exp,v
> retrieving revision 1.19
> diff -u -p -r1.19 catch-syscall.exp
> --- testsuite/gdb.base/catch-syscall.exp	22 Aug 2013 20:32:54 -0000	1.19
> +++ testsuite/gdb.base/catch-syscall.exp	29 Sep 2013 14:20:36 -0000
> @@ -19,7 +19,7 @@
>  # It was written by Sergio Durigan Junior <sergiodj@linux.vnet.ibm.com>
>  # on September/2008.
>  
> -if { [is_remote target] || ![isnative] } then {
> +if { ![isnative] } then {
>      continue
>  }
>  
> @@ -28,6 +28,14 @@ if {![istarget "hppa*-hp-hpux*"] && ![is
>      continue
>  }
>  
> +# This shall be updated whenever QCatchSyscalls packet support is implemented
> +# on some gdbserver architecture.
> +if { [is_remote target]
> +     && ![istarget "x86_64-*-linux*"] 
> +     && ![istarget "i\[34567\]86-*-linux*"] } {
> +    continue
> +}

This isn't strictly correct, as it's quite concievable that
e.g., a non-x86 valgrind (or some other random target) gets
support for these packets before our own corresponding gdbserver
port gets it.  Ideally we'd probe for support somehow, like by
looking for some GDB error/warning in the first "catch syscall", or
some such.

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

git diff tells me the patch adds a bunch of trailing whitespace.
Please fix that up throughout.

Other issues:

- I think that if you set a catch syscall, and then disconnect
in a way that gdbserver leaves the target running (disconnected
tracing, or if we have any persistent commands, see server.c), we'll end up
with the program held stopped as soon as the program calls a
syscall.  I think at least handle_target_event should be updated.

- linux-nat.c:linux_handle_syscall_trap has some hairy stuff
when "stopping" is true, due to syscall restarts.  I was
a little surprised that the gdbserver patch didn't need
anything of the sort, but then again, I don't recall whether
that bits is covered by the testsuite.
I'm wondering whether this -ENOSYS mechanism would allow
getting rid of all that, of if something is missing on the
gdbserver side.

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]