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

[Josh Stone <jistone@redhat.com>]

Thanks for the review!

I'll go ahead and correct the style issues without further comment.
Other replies are inline.

I'll try to rebase and send updates tomorrow.

On 11/22/2015 08:18 PM, Doug Evans wrote:
> Josh Stone <jistone@redhat.com> writes:
>> 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.
>>
>> Based on work from Philippe Waroquiers <philippe.waroquiers@skynet.be>.
>>
>> Beyond simple rebasing, I've also updated it to store the syscall catch
>> lists uniquely to each target process, and updated entry/return logic
>> from checking ENOSYS to now matching gdb's current Linux logic.
>>
>> gdb/ChangeLog:
>>
>> 2015-10-29  Josh Stone  <jistone@redhat.com>
>>
>> 	* 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.
>>
>> gdb/doc/ChangeLog:
>>
>> 2015-10-29  Josh Stone  <jistone@redhat.com>
>>
>> 	* 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-10-29  Josh Stone  <jistone@redhat.com>
>>
>> 	* 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_resume 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 (SYSCALL_SIGTRAP): Define.
>> 	(handle_extended_wait): Mark syscall_state like an entry.
>> 	(get_syscall_trapinfo): New function, proxy to the_low_target.
>> 	(linux_low_ptrace_options): Enable PTRACE_O_TRACESYSGOOD.
>> 	(linux_low_filter_event): Set ptrace options even before arch-specific
>> 	setup.  Either toggle syscall_state entry/return or set ignored.
>> 	(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-10-29  Josh Stone  <jistone@redhat.com>
>>
>> 	* 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                | 158 +++++++++++++++++++++++++++++--
>>  gdb/gdbserver/linux-low.h                |  13 +++
>>  gdb/gdbserver/linux-x86-low.c            |  31 ++++++
>>  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/remote.c                             | 116 +++++++++++++++++++++++
>>  gdb/testsuite/gdb.base/catch-syscall.exp |  15 ++-
>>  16 files changed, 472 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index b2b1e99d58c6..9c9b4b04a146 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -77,6 +77,11 @@ exec-events feature in qSupported
>>    response can contain the corresponding 'stubfeature'.  Set and
>>    show commands can be used to display whether these features are enabled.
>>  
>> +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.
>> @@ -87,6 +92,11 @@ set remote exec-event-feature-packet
>>  show remote exec-event-feature-packet
>>    Set/show the use of the remote exec event feature.
>>  
>> +* 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
>>  
>>  * Support for process record-replay and reverse debugging on aarch64*-linux*
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 3c1f7857393c..d8ed630da3fc 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -20121,6 +20121,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}}
>> @@ -35418,6 +35422,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.
>> @@ -35878,6 +35887,44 @@ by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
>>  Use of this packet is controlled by the @code{set non-stop} command; 
>>  @pxref{Non-Stop Mode}.
>>  
>> +@item QCatchSyscalls:1 @r{[};@var{sysno}@r{]}@dots{}
>> +@itemx QCatchSyscalls:0
>> +@cindex catch syscalls from inferior, remote request
>> +@cindex @samp{QCatchSyscalls} packet
>> +@anchor{QCatchSyscalls}
>> +Enable (@samp{QCatchSyscalls:1}) or disable (@samp{QCatchSyscalls:0})
>> +catching syscalls from the inferior process.
>> +
>> +For @samp{QCatchSyscalls:1}, each listed syscall @var{sysno} (encoded
>> +in hex) should be reported to @value{GDBN}.  If no syscall @var{sysno}
>> +is listed, every system call should be reported.
>> +
>> +Note that if a syscall not member of the list is reported, @value{GDBN}
>> +will filter it if this syscall is not caught.  It is however more efficient
>> +to only report the needed syscalls.
>> +
>> +Multiple @samp{QCatchSyscalls:1} packets do not
>> +combine; any earlier @samp{QCatchSyscalls:1} list is completely replaced by the
>> +new list.
>> +
>> +Reply:
>> +@table @samp
>> +@item OK
>> +The request succeeded.
>> +
>> +@item E @var{nn}
>> +An error occurred.  @var{nn} are hex digits.
>> +
>> +@item @w{}
>> +An empty reply indicates that @samp{QCatchSyscalls} is not supported by
>> +the stub.
>> +@end table
>> +
>> +Use of this packet is controlled by the @code{set remote catch-syscalls}
>> +command (@pxref{Remote Configuration, set remote catch-syscalls}).
>> +This packet is not probed by default; the remote stub must request it,
>> +by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
>> +
>>  @item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
>>  @cindex pass signals to inferior, remote request
>>  @cindex @samp{QPassSignals} packet
>> @@ -36296,6 +36343,11 @@ These are the currently defined stub features and their properties:
>>  @tab @samp{-}
>>  @tab Yes
>>  
>> +@item @samp{QCatchSyscalls}
>> +@tab No
>> +@tab @samp{-}
>> +@tab Yes
>> +
>>  @item @samp{QPassSignals}
>>  @tab No
>>  @tab @samp{-}
>> @@ -36489,6 +36541,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}).
>> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
>> index 72a3ef1b1b61..a8263656a4af 100644
>> --- a/gdb/gdbserver/inferiors.c
>> +++ b/gdb/gdbserver/inferiors.c
>> @@ -316,6 +316,7 @@ remove_process (struct process_info *process)
>>    free_all_breakpoints (process);
>>    gdb_assert (find_thread_process (process) == NULL);
>>    remove_inferior (&all_processes, &process->entry);
>> +  VEC_free (int, process->syscalls_to_catch);
>>    free (process);
>>  }
>>  
>> diff --git a/gdb/gdbserver/inferiors.h b/gdb/gdbserver/inferiors.h
>> index d7226163c0e8..43fc869f6612 100644
>> --- a/gdb/gdbserver/inferiors.h
>> +++ b/gdb/gdbserver/inferiors.h
>> @@ -19,6 +19,8 @@
>>  #ifndef INFERIORS_H
>>  #define INFERIORS_H
>>  
>> +#include "gdb_vecs.h"
>> +
>>  /* 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;
>> +
>>    const struct target_desc *tdesc;
>>  
>>    /* Private target data.  */
>> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
>> index 41ab510fa4ac..c2c513130997 100644
>> --- a/gdb/gdbserver/linux-low.c
>> +++ b/gdb/gdbserver/linux-low.c
>> @@ -70,6 +70,11 @@
>>  #define O_LARGEFILE 0
>>  #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 already defined in nat/linux-nat.h.

Ah, indeed, and that's already included too.  I'll nuke this.

>>  /* Some targets did not define these ptrace constants from the start,
>>     so gdbserver defines them locally here.  In the future, these may
>>     be removed after they are added to asm/ptrace.h.  */
>> @@ -447,6 +452,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>>    struct thread_info *event_thr = get_lwp_thread (event_lwp);
>>    struct lwp_info *new_lwp;
>>  
>> +  /* All extended events we currently use are mid-syscall.  Only
>> +     PTRACE_EVENT_STOP is delivered more like a signal-stop, but
>> +     you have to be using PTRACE_SEIZE to get that.  */
>> +  event_lwp->syscall_state = TARGET_WAITKIND_SYSCALL_ENTRY;
>> +
>>    if ((event == PTRACE_EVENT_FORK) || (event == PTRACE_EVENT_VFORK)
>>        || (event == PTRACE_EVENT_CLONE))
>>      {
>> @@ -662,6 +672,40 @@ get_pc (struct lwp_info *lwp)
>>    return pc;
>>  }
>>  
>> +/* This function should only be called if LWP got a SYSCALL_SIGTRAP.
>> +   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_thread;
>> +  struct regcache *regcache;
>> +
>> +  if (the_low_target.get_syscall_trapinfo == NULL)
>> +    {
>> +      /* If we cannot get the syscall trapinfo, report an unknown
>> +	 system call number and -ENOSYS return value.  */
>> +      *sysno = UNKNOWN_SYSCALL;
>> +      *sysret = -ENOSYS;
>> +      return;
>> +    }
>> +
>> +  saved_thread = current_thread;
>> +  current_thread = get_lwp_thread (lwp);
>> +
>> +  regcache = get_thread_regcache (current_thread, 1);
>> +  (*the_low_target.get_syscall_trapinfo) (regcache, sysno, sysret);
>> +
>> +  if (debug_threads)
>> +    {
>> +      debug_printf ("get_syscall_trapinfo sysno %d sysret %d\n",
>> +		    *sysno, *sysret);
>> +    }
>> +
>> +  current_thread = saved_thread;
>> +}
>> +
>>  /* This function should only be called if LWP got a SIGTRAP.
>>     The SIGTRAP could mean several things.
>>  
>> @@ -2154,6 +2198,8 @@ linux_low_ptrace_options (int attached)
>>    if (report_exec_events)
>>      options |= PTRACE_O_TRACEEXEC;
>>  
>> +  options |= PTRACE_O_TRACESYSGOOD;
>> +
>>    return options;
>>  }
>>  
>> @@ -2249,6 +2295,16 @@ linux_low_filter_event (int lwpid, int wstat)
>>  
>>    gdb_assert (WIFSTOPPED (wstat));
>>  
>> +  /* Set ptrace flags ASAP, so no events can be missed.  */
>> +  if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
> 
> It's a bit weird to check WIFSTOPPED after we just asserted it's true,
> but I realize all the subsequent "if"s checks WIFSTOPPED too.

I agree it's weird, but as you say it's the local pattern.

>> +    {
>> +      struct process_info *proc = find_process_pid (pid_of (thread));
>> +      int options = linux_low_ptrace_options (proc->attached);
>> +
>> +      linux_enable_event_reporting (lwpid, options);
>> +      child->must_set_ptrace_flags = 0;
>> +    }
>> +
> 
> Is moving the must_set_ptrace_flags check up to here good in general,
> or is it only necessary for this patch?
> I see that gdb/linux-nat.c does the must_set_ptrace_flags check early.
> [ref: gdb/linux.c line 2312: if (lp->must_set_ptrace_flags)]
> If this part of patch can be separated out, I think that'd be helpful.

Before I moved this, I was getting my first syscall trap as a plain
SIGTRAP, indicating PTRACE_O_TRACESYSGOOD wasn't set as desired.  I
finally figured out that the following block with child->status_pending
was returning early, so must_set_ptrace_flags wasn't even checked.

It might even by my own peculiar setup though, as I was also testing
this with my own strace-over-gdbserver hack.  I'm sure there are ways
that doesn't behave like a normal gdb client.

Anyway, I think it's probably a good change in general, especially to
mirror gdb's own behavior.  In practice it may only matter for syscalls
or the weird way I was using it, but that's ok.  It'll be easy to make
this piece a patch preceding the rest.

>>    if (WIFSTOPPED (wstat))
>>      {
>>        struct process_info *proc;
>> @@ -2276,13 +2332,19 @@ linux_low_filter_event (int lwpid, int wstat)
>>  	}
>>      }
>>  
>> -  if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
>> +  /* Always update syscall_state, even if it will be filtered later.  */
>> +  if (WIFSTOPPED (wstat) && WSTOPSIG (wstat) == SYSCALL_SIGTRAP)
>>      {
>> -      struct process_info *proc = find_process_pid (pid_of (thread));
>> -      int options = linux_low_ptrace_options (proc->attached);
>> -
>> -      linux_enable_event_reporting (lwpid, options);
>> -      child->must_set_ptrace_flags = 0;
>> +      child->syscall_state =
>> +	child->syscall_state == TARGET_WAITKIND_SYSCALL_ENTRY
>> +	? TARGET_WAITKIND_SYSCALL_RETURN
>> +	: TARGET_WAITKIND_SYSCALL_ENTRY;
> 
> Our convention when wrapping on "=" is to put the "=" on the next line.
> Also, I'd put the expression in parens like this:
> 
>       child->syscall_state
> 	= (child->syscall_state == TARGET_WAITKIND_SYSCALL_ENTRY
> 	   ? TARGET_WAITKIND_SYSCALL_RETURN
> 	   : TARGET_WAITKIND_SYSCALL_ENTRY);
> 
>> +    }
>> +  else
>> +    {
>> +      /* Almost all other ptrace-stops are known to be outside of system
>> +	 calls, with further exceptions in handle_extended_wait.  */
>> +      child->syscall_state = TARGET_WAITKIND_IGNORE;
>>      }
>>  
>>    /* Be careful to not overwrite stop_pc until
>> @@ -2885,6 +2947,44 @@ ignore_event (struct target_waitstatus *ourstatus)
>>    return null_ptid;
>>  }
>>  
>> +/* Returns 1 if GDB is interested in any event_child syscalls.  */
>> +
>> +static int
>> +gdb_catching_syscalls_p (struct lwp_info *event_child)
>> +{
>> +  struct thread_info *thread = get_lwp_thread (event_child);
>> +  struct process_info *proc = get_thread_process (thread);
>> +
>> +  return !VEC_empty (int, proc->syscalls_to_catch);
>> +}
>> +
>> +/* Returns 1 if GDB is interested in the event_child syscall.
>> +   Only to be called when stopped reason is SYSCALL_SIGTRAP.  */
>> +
>> +static int
>> +gdb_catch_this_syscall_p (struct lwp_info *event_child)
>> +{
>> +  int i, iter;
>> +  int sysno, sysret;
>> +  struct thread_info *thread = get_lwp_thread (event_child);
>> +  struct process_info *proc = get_thread_process (thread);
>> +
>> +  if (VEC_empty (int, proc->syscalls_to_catch))
>> +    return 0;
>> +
>> +  if (VEC_index (int, proc->syscalls_to_catch, 0) == ANY_SYSCALL)
>> +    return 1;
>> +
>> +  get_syscall_trapinfo (event_child, &sysno, &sysret);
>> +  for (i = 0;
>> +       VEC_iterate (int, proc->syscalls_to_catch, i, iter);
>> +       i++)
>> +    if (iter == sysno)
>> +      return 1;
>> +
>> +  return 0;
>> +}
>> +
>>  /* Wait for process, returns status.  */
>>  
>>  static ptid_t
>> @@ -3195,6 +3295,22 @@ linux_wait_1 (ptid_t ptid,
>>  
>>    /* 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)
>> +	{
>> +	  debug_printf ("Ignored syscall for LWP %ld.\n",
>> +			lwpid_of (current_thread));
>> +	}
>> +
>> +      linux_resume_one_lwp (event_child, event_child->stepping,
>> +			    0, NULL);
>> +      return ignore_event (ourstatus);
>> +    }
>> +
>>    /* 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
>> @@ -3449,8 +3565,16 @@ linux_wait_1 (ptid_t ptid,
>>  	}
>>      }
>>  
>> -  if (current_thread->last_resume_kind == resume_stop
>> -      && WSTOPSIG (w) == SIGSTOP)
>> +  if (WSTOPSIG (w) == SYSCALL_SIGTRAP)
>> +    {
>> +      int sysret;
>> +
>> +      get_syscall_trapinfo (event_child,
>> +			    &ourstatus->value.syscall_number, &sysret);
>> +      ourstatus->kind = event_child->syscall_state;
>> +    }
>> +  else if (current_thread->last_resume_kind == resume_stop
>> +	   && WSTOPSIG (w) == SIGSTOP)
>>      {
>>        /* A thread that has been requested to stop by GDB with vCont;t,
>>  	 and it stopped cleanly, so report as SIG0.  The use of
>> @@ -3867,6 +3991,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>    struct thread_info *thread = get_lwp_thread (lwp);
>>    struct thread_info *saved_thread;
>>    int fast_tp_collecting;
>> +  int ptrace_request;
>>    struct process_info *proc = get_thread_process (thread);
>>  
>>    /* Note that target description may not be initialised
>> @@ -4052,7 +4177,14 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>    regcache_invalidate_thread (thread);
>>    errno = 0;
>>    lwp->stepping = step;
>> -  ptrace (step ? PTRACE_SINGLESTEP : PTRACE_CONT, lwpid_of (thread),
>> +  if (step)
>> +    ptrace_request = PTRACE_SINGLESTEP;
>> +  else if (gdb_catching_syscalls_p (lwp))
>> +    ptrace_request =  PTRACE_SYSCALL;
> 
> Unnecessary extra space after =.
> 
>> +  else
>> +    ptrace_request =  PTRACE_CONT;
> 
> Ditto.
> 
>> +  ptrace (ptrace_request,
>> +	  lwpid_of (thread),
>>  	  (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.  */
>> @@ -6135,6 +6267,13 @@ linux_process_qsupported (const char *query)
>>  }
>>  
>>  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)
>> @@ -7010,6 +7149,7 @@ static struct target_ops linux_target_ops = {
>>    linux_common_core_of_thread,
>>    linux_read_loadmap,
>>    linux_process_qsupported,
>> +  linux_supports_catch_syscall,
>>    linux_supports_tracepoints,
>>    linux_read_pc,
>>    linux_write_pc,
>> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
>> index ccf4c9421d87..71d72f48ea3a 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) (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);
>>  
>> @@ -271,6 +277,13 @@ struct lwp_info
>>       event already received in a wait()).  */
>>    int stopped;
>>  
>> +  /* Signal wether 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 89ec4e54b3b8..6e3c589de8fb 100644
>> --- a/gdb/gdbserver/linux-x86-low.c
>> +++ b/gdb/gdbserver/linux-x86-low.c
>> @@ -1432,6 +1432,36 @@ x86_arch_setup (void)
>>    current_process ()->tdesc = x86_linux_read_description ();
>>  }
>>  
>> +/* Fill *SYSNO and *SYSRET with the syscall nr trapped and the syscall return
>> +   code.  This should only be called if LWP got a SYSCALL_SIGTRAP.  */
>> +
>> +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;
> 
> These casts are confusing, especially when coupled with the l_ prefix
> ("l" for "long" is the first thing that comes to mind, but these are ints).
> I can appreciate them when casting from long to int, but here we
> already have ints.

Ah, yeah, I think this is simply duped from the 64bit block.

> How about changing the parameters to sysno_ptr, sysret_ptr,
> and change l_sysno,l_sysret to sysno,sysret?
> Or some such.

I think we can just drop the locals entirely for this block and just
collect the registers directly into the sysno/sysret parameters.

>> +    }
>> +}
>> +
>>  static int
>>  x86_supports_tracepoints (void)
>>  {
>> @@ -3292,6 +3322,7 @@ struct linux_target_ops the_low_target =
>>    x86_linux_new_fork,
>>    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,
>> diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
>> index d72c46515d13..1301acea49b7 100644
>> --- a/gdb/gdbserver/nto-low.c
>> +++ b/gdb/gdbserver/nto-low.c
>> @@ -979,6 +979,7 @@ static struct target_ops nto_target_ops = {
>>    NULL, /* core_of_thread */
>>    NULL, /* read_loadmap */
>>    NULL, /* process_qsupported */
>> +  NULL, /* supports_catch_syscall */
>>    NULL, /* supports_tracepoints */
>>    NULL, /* read_pc */
>>    NULL, /* write_pc */
>> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
>> index e36609176afa..19eed77d4874 100644
>> --- a/gdb/gdbserver/remote-utils.c
>> +++ b/gdb/gdbserver/remote-utils.c
>> @@ -1119,6 +1119,8 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>      case TARGET_WAITKIND_VFORKED:
>>      case TARGET_WAITKIND_VFORK_DONE:
>>      case TARGET_WAITKIND_EXECD:
>> +    case TARGET_WAITKIND_SYSCALL_ENTRY:
>> +    case TARGET_WAITKIND_SYSCALL_RETURN:
>>        {
>>  	struct thread_info *saved_thread;
>>  	const char **regp;
>> @@ -1161,6 +1163,16 @@ prepare_resume_reply (char *buf, ptid_t ptid,
>>  	    status->value.execd_pathname = NULL;
>>  	    buf += strlen (buf);
>>  	  }
>> +	else if ((status->kind == TARGET_WAITKIND_SYSCALL_ENTRY)
>> +		 || (status->kind == TARGET_WAITKIND_SYSCALL_RETURN))
>> +	  {
>> +	    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 fd2804beaa4e..72621f261dfa 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -592,6 +592,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;
>> +	}
>> +
>> +      process = current_process ();
>> +
>> +      VEC_truncate (int, process->syscalls_to_catch, 0);
>> +
>> +      p = own_buf + strlen("QCatchSyscalls:1");
>> +      if (*p == ';')
>> +	{
>> +	  p += 1;
>> +	  while (*p)
>> +	    {
>> +	      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;
>> @@ -2140,6 +2186,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 96ad4fa58b7c..a8780ebfbc65 100644
>> --- a/gdb/gdbserver/server.h
>> +++ b/gdb/gdbserver/server.h
>> @@ -133,4 +133,10 @@ extern void discard_queued_stop_replies (ptid_t ptid);
>>     as large as the largest register set supported by gdbserver.  */
>>  #define PBUFSIZ 16384
>>  
>> +/* Definition for an unknown syscall, used basically in error-cases.  */
>> +#define UNKNOWN_SYSCALL (-1)
>> +
>> +/* Definition for any syscall, used for unfiltered syscall reporting.  */
>> +#define ANY_SYSCALL (-2)
>> +
>>  #endif /* SERVER_H */
>> diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
>> index 89bed7a02dd1..55ec0caef296 100644
>> --- a/gdb/gdbserver/spu-low.c
>> +++ b/gdb/gdbserver/spu-low.c
>> @@ -699,6 +699,7 @@ static struct target_ops spu_target_ops = {
>>    NULL, /* core_of_thread */
>>    NULL, /* read_loadmap */
>>    NULL, /* process_qsupported */
>> +  NULL, /* supports_catch_syscall */
>>    NULL, /* supports_tracepoints */
>>    NULL, /* read_pc */
>>    NULL, /* write_pc */
>> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
>> index 769c876ede49..ef8a7680ccca 100644
>> --- a/gdb/gdbserver/target.h
>> +++ b/gdb/gdbserver/target.h
>> @@ -309,6 +309,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);
>> @@ -526,6 +530,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)
>> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
>> index 6e33509c0b50..4314513876fa 100644
>> --- a/gdb/gdbserver/win32-low.c
>> +++ b/gdb/gdbserver/win32-low.c
>> @@ -1844,6 +1844,7 @@ static struct target_ops win32_target_ops = {
>>    NULL, /* core_of_thread */
>>    NULL, /* read_loadmap */
>>    NULL, /* process_qsupported */
>> +  NULL, /* supports_catch_syscall */
>>    NULL, /* supports_tracepoints */
>>    NULL, /* read_pc */
>>    NULL, /* write_pc */
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index fed397affeab..41751e9d93b1 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -1388,6 +1388,7 @@ enum {
>>    PACKET_qSupported,
>>    PACKET_qTStatus,
>>    PACKET_QPassSignals,
>> +  PACKET_QCatchSyscalls,
>>    PACKET_QProgramSignals,
>>    PACKET_qCRC,
>>    PACKET_qSearch_memory,
>> @@ -1973,6 +1974,99 @@ remote_pass_signals (struct target_ops *self,
>>      }
>>  }
>>  
>> +/* If 'QCatchSyscalls' is supported, tell the remote stub
>> +   to report syscalls to GDB.  */
>> +
>> +static int
>> +remote_set_syscall_catchpoint (struct target_ops *self,
>> +			       int pid, int needed, int any_count,
>> +			       int table_size, int *table)
>> +{
>> +  char *catch_packet, *p;
>> +  enum packet_result result;
>> +  int n_sysno = 0;
>> +
>> +  if (remote_protocol_packets[PACKET_QCatchSyscalls].support == PACKET_DISABLE)
>> +    {
>> +      /* Not supported.  */
>> +      return 1;
>> +    }
>> +
>> +  if (needed && !any_count)
>> +    {
>> +      int i;
>> +
>> +      /* Count how many syscalls are to be caught (table[sysno] != 0).  */
>> +      for (i = 0; i < table_size; i++)
>> +	{
>> +	  if (table[i])
>> +	    n_sysno++;
>> +	}
>> +    }
>> +
>> +  if (remote_debug)
>> +    {
>> +      fprintf_unfiltered (gdb_stdlog,
>> +			  "remote_set_syscall_catchpoint "
>> +			  "pid %d needed %d any_count %d n_sysno %d\n",
>> +			  pid, needed, any_count, n_sysno);
>> +    }
>> +
>> +  if (needed)
>> +    {
>> +      /* Prepare a packet with the sysno list, assuming max 8+1
>> +	 characters for a sysno.  If the resulting packet size is too
>> +	 big, fallback on the non selective packet.  */
>> +      const 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])
>> +		{
>> +		  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 ("QCatchSyscalls:1")] = 0;
>> +	}
>> +    }
>> +  else
>> +    {
>> +      catch_packet = xmalloc (strlen ("QCatchSyscalls:0") + 1);
>> +      strcpy (catch_packet, "QCatchSyscalls:0");
>> +    }
>> +
>> +  {
>> +    struct remote_state *rs = get_remote_state ();
>> +    char *buf = rs->buf;
>> +
>> +    putpkt (catch_packet);
>> +    getpkt (&rs->buf, &rs->buf_size, 0);
>> +    result = packet_ok (buf, &remote_protocol_packets[PACKET_QCatchSyscalls]);
>> +    xfree (catch_packet);
>> +    if (result == PACKET_OK)
>> +      return 0;
>> +    else
>> +      return -1;
>> +  }
>> +}
>> +
>>  /* If 'QProgramSignals' is supported, tell the remote stub what
>>     signals it should pass through to the inferior when detaching.  */
>>  
>> @@ -4328,6 +4422,8 @@ static const struct protocol_feature remote_protocol_features[] = {
>>      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,
>> @@ -6159,6 +6255,22 @@ Packet: '%s'\n"),
>>  
>>  	  if (strprefix (p, p1, "thread"))
>>  	    event->ptid = read_ptid (++p1, &p);
>> +	  else if (strprefix (p, p1, "syscall_entry"))
>> +	    {
>> +	      ULONGEST sysno;
>> +
>> +	      event->ws.kind = TARGET_WAITKIND_SYSCALL_ENTRY;
>> +	      p = unpack_varlen_hex (++p1, &sysno);
>> +	      event->ws.value.syscall_number = (int) sysno;
>> +	    }
>> +	  else if (strprefix (p, p1, "syscall_return"))
>> +	    {
>> +	      ULONGEST sysno;
>> +
>> +	      event->ws.kind = TARGET_WAITKIND_SYSCALL_RETURN;
>> +	      p = unpack_varlen_hex (++p1, &sysno);
>> +	      event->ws.value.syscall_number = (int) sysno;
>> +	    }
>>  	  else if (strprefix (p, p1, "watch")
>>  		   || strprefix (p, p1, "rwatch")
>>  		   || strprefix (p, p1, "awatch"))
>> @@ -12734,6 +12846,7 @@ Specify the serial device it is connected to\n\
>>    remote_ops.to_load = remote_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_update_thread_list = remote_update_thread_list;
>> @@ -13263,6 +13376,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
>>    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);
>>  
>> 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
>>  }
>>  
>> @@ -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" } {
>> +	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).