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

[Josh Stone <jistone at redhat dot com>]

On 12/04/2015 05:18 AM, Pedro Alves wrote:
> 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.

Yes, I think it should be cleared to avoid any assumption about the
architecture.  I'll add a note in the description codifying this.

> 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.)

I'll see if I can find out.

Something else I just realized - my syscall_state accounting gets lost
when an exec creates a new lwp_info.  GDB is sending another
QCatchSyscalls for the "new" process, and then the exec return is
thought to be a syscall_entry.  That's easy enough to correct, but I
should write a new test for this too. (and see what gdb alone does...)


On a total tangent, is it ever possible that GDB/GDBserver might try to
read and modify registers from a PTRACE_EVENT stop?  If so, you should
beware that registers may actually be in flux.  I ran into this with
Dyninst, which I fixed here[1] though I can't find the discussion now.

The gist was that in a PTRACE_EVENT, the kernel may not have written the
return register yet.  Dyninst wanted to save registers, resume in a bit
of instrumentation code, then restore registers and resume the normal
program.  So the saved registers got an intermediate RAX, and when it
resumed into instrumentation the kernel finally wrote the good RAX
return value to complete the syscall (which the instrumentation
ignored).  Then when dyninst restored registers the bad RAX was written
back, and thus the normal program code didn't get the correct value for
its fork return.  My solution was to step out of the event with
PTRACE_SYSCALL before doing anything else.

[1]
http://git.dyninst.org/?p=dyninst.git;a=commit;h=b89ea1d19677fa0dd9c605ef492c5f6dabf15752

> 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.

OK.  Looking at "exec" for a model, I guess I should also mention the
syscall_entry/return stop reasons and the qSupported addition.

>> +@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:

It's already added in the first patch hunk of this file, no?

Related, I just noticed the code was checking only for remote support,
not the local enabled state.  I'll fix that to use packet_support().

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

OK, I'll expand this.

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

Ah, indeed.  It seems more convenient to put the new field at the end,
but right now it roughly mirrors the placement of the new target_ops
field.  Maybe that should be at the end too, then I can touch fewer
files overall.  (i.e. no change in targets I'm not implementing.)

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

OK, removed.

>> +	  {
>> +	    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.

OK, I will merge these and make it parse more strictly.

>> +
>> +      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)

OK

>> +	    {
>> +	      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.  :-)

OK

>> +	    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"

OK

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

OK

>> +		{
>> +		  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;

OK, I see, since xsnprintf asserts that the return count did fit, it can
be used directly.  I guess "p += xsnprintf (...)" should do fine.

>> +		}
>> +	    }
>> +	}
>> +      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;

OK

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

OK

>> +    }
>> +
>> +  {
>> +    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.

OK

>> +    xfree (catch_packet);
> 
> putpkt/getpkt may throw.  This should be freed with a cleanup instead.

OK

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

OK, I'll just remove it then.

>>  
>> @@ -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...

Yeah, well this one is "vfork", but it still conceptually overlaps with
what foll-vfork.exp checks.

For the moment, I guess I can just remove the latter clause, since you
pointed out that native-extended-gdbserver is already not "is_remote".

>> +	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
>