Re: [PATCH v3 2/2] Implement 'catch syscall' for gdbserver

[Pedro Alves <palves at redhat dot com>]

Quick question: What is supposed to happen to the QCatchSyscalls
when the process execs?  I'm thinking of 64-bit inferior execing
32-bit inferior, etc.  The syscall numbers aren't necessarily shared
between the different architectures.  This implementation deletes discards
the previous QCatchSyscalls on exec, and I think that's what makes sense,
but, I think that this should be explicit in the packet's description.

I'm not sure gdb clears the inferior's "syscalls to the caught" VEC
on exec events, but it probably does (if it doesn't, I think it should.)

On 12/04/2015 02:26 AM, Josh Stone wrote:
> This adds a new QCatchSyscalls packet to enable 'catch syscall', and new
> stop reasons "syscall_entry" and "syscall_return" for those events.  It
> is currently only supported on Linux x86 and x86_64.
> 
> gdb/ChangeLog:
> 
> 2015-12-03  Josh Stone  <jistone@redhat.com>
> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* NEWS (Changes since GDB 7.10): Mention QCatchSyscalls and new
> 	GDBserver support for catch syscall.
> 	* remote.c (PACKET_QCatchSyscalls): New enum.
> 	(remote_set_syscall_catchpoint): New function.
> 	(remote_protocol_features): New element for QCatchSyscalls.
> 	(remote_parse_stop_reply): Parse syscall_entry/return stops.
> 	(init_remote_ops): Install remote_set_syscall_catchpoint.
> 	(_initialize_remote): Config QCatchSyscalls.
> 	* linux-nat.h (struct lwp_info) <syscall_state>: Comment typo.
> 
> gdb/doc/ChangeLog:
> 
> 2015-12-03  Josh Stone  <jistone@redhat.com>
> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.texinfo (Remote Configuration): List the QCatchSyscalls packet.
> 	(Stop Reply Packets): List the syscall entry and return stop reasons.
> 	(General Query Packets): Describe QCatchSyscalls, and add it to the
> 	table and detailed list of stub features.
> 
> gdb/gdbserver/ChangeLog:
> 
> 2015-12-03  Josh Stone  <jistone@redhat.com>
> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* inferiors.h: Include "gdb_vecs.h".
> 	(struct process_info): Add syscalls_to_catch.
> 	* inferiors.c (remove_process): Free syscalls_to_catch.
> 	* remote-utils.c (prepare_resume_reply): Report syscall_entry and
> 	syscall_return stops.
> 	* server.h (UNKNOWN_SYSCALL, ANY_SYSCALL): Define.
> 	* server.c (handle_general_set): Handle QCatchSyscalls.
> 	(handle_query): Report support for QCatchSyscalls.
> 	* target.h (struct target_ops): Add supports_catch_syscall.
> 	(target_supports_catch_syscall): New macro.
> 	* linux-low.h (struct linux_target_ops): Add get_syscall_trapinfo.
> 	(struct lwp_info): Add syscall_state.
> 	* linux-low.c (handle_extended_wait): Mark syscall_state as an entry.
> 	(get_syscall_trapinfo): New function, proxy to the_low_target.
> 	(linux_low_ptrace_options): Enable PTRACE_O_TRACESYSGOOD.
> 	(linux_low_filter_event): Toggle syscall_state entry/return for
> 	syscall traps, and set it ignored for all others.
> 	(gdb_catching_syscalls_p): New function.
> 	(gdb_catch_this_syscall_p): New function.
> 	(linux_wait_1): Handle SYSCALL_SIGTRAP.
> 	(linux_resume_one_lwp_throw): Add PTRACE_SYSCALL possibility.
> 	(linux_supports_catch_syscall): New function.
> 	(linux_target_ops): Install it.
> 	* linux-x86-low.c (x86_get_syscall_trapinfo): New function.
> 	(the_low_target): Install it.
> 	* nto-low.c (nto_target_ops): Install NULL supports_catch_syscall.
> 	* spu-low.c (spu_target_ops): Likewise.
> 	* win32-low.c (win32_target_ops): Likewise.
> 
> gdb/testsuite/ChangeLog:
> 
> 2015-12-03  Josh Stone  <jistone@redhat.com>
> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.base/catch-syscall.exp: Enable testing for x86 and x86_64 linux
> 	remote targets.
> 	(do_syscall_tests): Only test mid-vfork on local or extended-remote.
> ---
>  gdb/NEWS                                 |  10 +++
>  gdb/doc/gdb.texinfo                      |  56 +++++++++++++
>  gdb/gdbserver/inferiors.c                |   1 +
>  gdb/gdbserver/inferiors.h                |   5 ++
>  gdb/gdbserver/linux-low.c                | 140 ++++++++++++++++++++++++++++++-
>  gdb/gdbserver/linux-low.h                |  13 +++
>  gdb/gdbserver/linux-x86-low.c            |  26 ++++++
>  gdb/gdbserver/nto-low.c                  |   1 +
>  gdb/gdbserver/remote-utils.c             |  12 +++
>  gdb/gdbserver/server.c                   |  49 +++++++++++
>  gdb/gdbserver/server.h                   |   6 ++
>  gdb/gdbserver/spu-low.c                  |   1 +
>  gdb/gdbserver/target.h                   |   8 ++
>  gdb/gdbserver/win32-low.c                |   1 +
>  gdb/linux-nat.h                          |   2 +-
>  gdb/remote.c                             | 116 +++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/catch-syscall.exp |  15 +++-
>  17 files changed, 456 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a222dfb491f0..1917523d8249 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -112,6 +112,11 @@ N stop reply
>    threads are stopped).  The remote stub reports support for this stop
>    reply to GDB's qSupported query.
>  
> +QCatchSyscalls:1 [;SYSNO]...
> +QCatchSyscalls:0
> +  Enable ("QCatchSyscalls:1") or disable ("QCatchSyscalls:0")
> +  catching syscalls from the inferior process.
> +
>  * Extended-remote exec events
>  
>    ** GDB now has support for exec events on extended-remote Linux targets.
> @@ -127,6 +132,11 @@ show remote exec-event-feature-packet
>     The reply to qXfer:threads:read may now include a name attribute for each
>     thread.
>  
> +* New features in the GDB remote stub, GDBserver
> +
> +  ** GDBserver now supports catch syscall.  Currently enabled
> +     on x86/x86_64 GNU/Linux targets.
> +
>  *** Changes in GDB 7.10
>  

Please mention the new set/show commands as well.


> +@item QCatchSyscalls
> +The remote stub understands the @samp{QCatchSyscalls} packet
> +(@pxref{QCatchSyscalls}).
> +
>  @item QPassSignals
>  The remote stub understands the @samp{QPassSignals} packet
>  (@pxref{QPassSignals}).

You also need to list the new commands in the table below:

 For each packet @var{name}, the command to enable or disable the
 packet is @code{set remote @var{name}-packet}.  The available settings
 are:




>  /* Generic information for tracking a list of ``inferiors'' - threads,
>     processes, etc.  */
>  struct inferior_list
> @@ -67,6 +69,9 @@ struct process_info
>    /* The list of installed fast tracepoints.  */
>    struct fast_tracepoint_jump *fast_tracepoint_jumps;
>  
> +  /* The list of syscalls to report, or just ANY_SYSCALL.  */
> +  VEC (int) *syscalls_to_catch;

"just" here means that it's a list with a single element that
happens to be ANY_SYSCALL; I think it's helpful for the reader to
be explicit and expand the "or just ANY_SYSCALL" comment little bit.

> +
>    const struct target_desc *tdesc;
>  
>    /* Private target data.  */
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 5e2dc5857a8d..cceede6fe408 100644



> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -201,6 +201,12 @@ struct linux_target_ops
>    /* Hook to support target specific qSupported.  */
>    void (*process_qsupported) (char **, int count);
>  
> +  /* 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);
> +

As you're not putting this new callback at the end of the struct,
you need to install a NULL callback in all linux-*-low.c files (all
archs) that install linux_target_ops methods after process_qsupported.
Otherwise you'll break their builds.

>    /* Returns true if the low target supports tracepoints.  */
>    int (*supports_tracepoints) (void);
>  
> @@ -277,6 +283,13 @@ struct lwp_info
>       event already received in a wait()).  */
>    int stopped;
>  
> +  /* Signal whether we are in a SYSCALL_ENTRY or
> +     in a SYSCALL_RETURN event.
> +     Values:
> +     - TARGET_WAITKIND_SYSCALL_ENTRY
> +     - TARGET_WAITKIND_SYSCALL_RETURN */
> +  enum target_waitkind syscall_state;
> +
>    /* When stopped is set, the last wait status recorded for this lwp.  */
>    int last_status;
>  
> diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
> index 18adf5ee7d17..1caf71edf485 100644



> @@ -1172,6 +1174,16 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>  
>  	    sprintf (buf, "T%02xcreate:;", signal);
>  	  }
> +	else if ((status->kind == TARGET_WAITKIND_SYSCALL_ENTRY)
> +		 || (status->kind == TARGET_WAITKIND_SYSCALL_RETURN))

Unnecessary parens.

> +	  {
> +	    enum gdb_signal signal = GDB_SIGNAL_TRAP;
> +	    const char *event = (status->kind == TARGET_WAITKIND_SYSCALL_ENTRY
> +				 ? "syscall_entry" : "syscall_return");
> +
> +	    sprintf (buf, "T%02x%s:%x;", signal, event,
> +		     status->value.syscall_number);
> +	  }
>  	else
>  	  sprintf (buf, "T%02x", status->value.sig);
>  
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 6d151ee35d39..e63656b9c0f6 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -597,6 +597,52 @@ handle_general_set (char *own_buf)
>        return;
>      }
>  
> +  if (startswith (own_buf, "QCatchSyscalls:1"))
> +    {
> +      const char *p;
> +      CORE_ADDR sysno;
> +      struct process_info *process;
> +
> +      if (!target_running () || !target_supports_catch_syscall ())
> +	{
> +	  write_enn (own_buf);
> +	  return;
> +	}

I think you should merge this with

> +  if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> +    {
> +      if (!target_running () || !target_supports_catch_syscall ())
> +	{
> +	  write_enn (own_buf);
> +	  return;
> +	}

 into a single

 if (startswith (own_buf, "QCatchSyscalls:")
    {
      ...
      if (!target_running () || !target_supports_catch_syscall ())
	{
	  write_enn (own_buf);
	  return;
	}

See "QNonStop:" handling.  The end result is that "QCatchSyscalls:2"
is treated as an error instead of returning the empty packet.



> +
> +      process = current_process ();
> +
> +      VEC_truncate (int, process->syscalls_to_catch, 0);
> +
> +      p = own_buf + strlen("QCatchSyscalls:1");
> +      if (*p == ';')
> +	{
> +	  p += 1;
> +	  while (*p)

  while (*p != '\0)

> +	    {
> +	      p = decode_address_to_semicolon (&sysno, p);
> +	      VEC_safe_push (int, process->syscalls_to_catch, (int) sysno);
> +	    }
> +	}
> +      else
> +	VEC_safe_push (int, process->syscalls_to_catch, ANY_SYSCALL);
> +
> +      write_ok (own_buf);
> +      return;
> +    }
> +
> +  if (strcmp ("QCatchSyscalls:0", own_buf) == 0)
> +    {
> +      if (!target_running () || !target_supports_catch_syscall ())
> +	{
> +	  write_enn (own_buf);
> +	  return;
> +	}
> +
> +      VEC_free (int, current_process ()->syscalls_to_catch);
> +      write_ok (own_buf);
> +      return;
> +    }
> +
>    if (startswith (own_buf, "QProgramSignals:"))
>      {
>        int numsigs = (int) GDB_SIGNAL_LAST, i;
> @@ -2219,6 +2265,9 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  	       "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+");
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index dc0361fdcd14..bc2abd5f50c8 100644



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

	  if (table[i] != 0)

just like the comment.  :-)


> +	    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.  */

"non-selective packet"

> +      const int maxpktsz = strlen ("QCatchSyscalls:1") + n_sysno * 9 + 1;
> +
> +      catch_packet = xmalloc (maxpktsz);
> +      strcpy (catch_packet, "QCatchSyscalls:1");
> +      if (!any_count)
> +	{
> +	  int i;
> +	  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])

Likewise.

> +		{
> +		  xsnprintf (p, catch_packet + maxpktsz - p, ";%x", i);
> +		  p += strlen (p);

This looks like:

		  len = xsnprintf (p, catch_packet + maxpktsz - p, ";%x", i);
		  p += len;


> +		}
> +	    }
> +	}
> +      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 ("QCatchSyscalls:1")] = 0;

  catch_packet[sizeof ("QCatchSyscalls:1") - 1] = 0;

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

      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]);

getpkt takes a pointer to pointer because it may xrealloc 'rs->buf'.
So this 'buf' reference may be dangling.  Just drop the 'buf' local.


> +    xfree (catch_packet);

putpkt/getpkt may throw.  This should be freed with a cleanup instead.

> +    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.  */
>  



> diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
> index c1cfe23cdddb..0ba078db22ac 100644
> --- a/gdb/testsuite/gdb.base/catch-syscall.exp
> +++ b/gdb/testsuite/gdb.base/catch-syscall.exp
> @@ -19,7 +19,15 @@
>  # 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
> +}
> +
> +# 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 check won't reach the "continue" when testing
with --target_board=native-extended-gdbserver on some arch that doesn't
implement QCatchSyscall yet.  That board is not "is_remote".

I'd actually favor dropping the arch check altogether.  Otherwise it's very
likely that arch maintainers won't ever notice there's a new method they
can wire in.

>  
> @@ -390,7 +398,10 @@ proc do_syscall_tests {} {
>      if [runto_main] then { test_catch_syscall_skipping_return }
>  
>      # Testing the 'catch syscall' command starting mid-vfork.
> -    if [runto_main] then { test_catch_syscall_mid_vfork }
> +    # (Only local or extended-remote can use "catch vfork".)
> +    if { ![is_remote target] || [target_info gdb_protocol] == "extended-remote" } {

I think IWBN to add a supports_catch_fork proc to lib/gdb.exp.  These
checks are getting duplicated (and not sure they are consistent) in
several places.  E.g., here, your now catch-fork-static.exp test, the
test that was based on, etc...

> +	if [runto_main] then { test_catch_syscall_mid_vfork }
> +    }
>  
>      # Testing if the 'catch syscall' command works when switching to
>      # different architectures on-the-fly (PR gdb/10737).

Thanks,
Pedro Alves