Re: [PATCH] Add support for catching system calls to native FreeBSD targets.

[Pedro Alves <palves at redhat dot com>]

Hi John,

This looks good to me.  Just some minor nits below.

On 06/14/2016 09:57 PM, John Baldwin wrote:
> All platforms on FreeBSD use a shared system call table, so use a
> single XML file to describe the system calls available on each FreeBSD
> platform.
> 
> xRecent 

typo.

> versions of FreeBSD include the identifier of the current
> system call when reporting a system call entry or exit event in the
> ptrace_lwpinfo structure obtained via PT_LWPINFO in fbsd_wait.  As
> such, FreeBSD native targets do not use the gdbarch method to fetch
> the system call code.  In addition, FreeBSD register sets fetched via
> ptrace do not include an equivalent of 'orig_rax' (on amd64 for
> example), so the system call code cannot be extracted from the
> available registers during a system call exit.  However, GDB assumes
> that system call catch points are not supported if the gdbarch method
> is not present.  As a workaround, FreeBSD ABIs install a dummy gdbarch
> method that throws an internal_error if it is ever invoked.
> 

We should probably get rid of this gdbarch method, by making linux-nat.c
(the only caller) call an arch-specific target_ops override instead of
a gdbarch method, like gdbserver's equivalent code does.

To replace the break-catch-syscall.c error, I think that it'd be reasonable
to remove it altogether, and for Linux targets that don't implement
the gdbarch hook yet, instead just always intercept all syscalls, reporting
an <unknown> syscall number.

But what you did seems like a reasonable thing to do as long as do
have the gdbarch hook.  

> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 6a72f72..4ed706a 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1526,6 +1526,11 @@ fi
>  AC_CHECK_MEMBERS([struct ptrace_lwpinfo.pl_tdname], [], [],
>                   [#include <sys/ptrace.h>])
>  
> +# See if <sys/ptrace.h> supports syscall fields on FreeBSD

Missing period.

> +# Older FreeBSD versions don't have the pl_syscall_code member of
> +# `struct ptrace_lwpinfo'.

Can you replace "Older" with a non-relative version reference?

> +AC_CHECK_MEMBERS([struct ptrace_lwpinfo.pl_syscall_code], [], [],
> +                 [#include <sys/ptrace.h>])
>  

> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index b582abe..741a96d 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -707,6 +707,40 @@ fbsd_wait (struct target_ops *ops,
>  	      return wptid;
>  	    }
>  #endif
> +
> +	  /* Note that PL_FLAG_SCE is set for any event reported while
> +	     a thread is executing a system call in the kernel.  In
> +	     particular, signals that interrupt a sleep in a system
> +	     call will report this flag as part of their event.  Stops
> +	     explicitly for system call entry and exit always use
> +	     SIGTRAP, so only treat SIGTRAP events as system call
> +	     entriy/exit events.  */

Typo "entriy".  

> +	  if (pl.pl_flags & (PL_FLAG_SCE | PL_FLAG_SCX)
> +	      && ourstatus->value.sig == SIGTRAP)
> +	    {
> +#ifdef HAVE_STRUCT_PTRACE_LWPINFO_PL_SYSCALL_CODE
> +	      if (catch_syscall_enabled ())
> +		{
> +		  if (catching_syscall_number (pl.pl_syscall_code))
> +		    {
> +		      if (pl.pl_flags & PL_FLAG_SCE)
> +			ourstatus->kind = TARGET_WAITKIND_SYSCALL_ENTRY;
> +		      else
> +			ourstatus->kind = TARGET_WAITKIND_SYSCALL_RETURN;
> +		      ourstatus->value.syscall_number = pl.pl_syscall_code;
> +		      return wptid;
> +		    }
> +		}
> +#endif
> +	      /* If the core isn't interested in this event, just
> +		 continue the process explicitly and wait for another
> +		 event.  Note that PT_SYSCALL is "sticky" on FreeBSD
> +		 and once system call stops are enabled on a process
> +		 it stops for all system call entries and exits.  */
> +	      if (ptrace (PT_CONTINUE, pid, (caddr_t) 1, 0) == -1)
> +		perror_with_name (("ptrace"));
> +	      continue;
> +	    }
>  	}
>        return wptid;
>      }

>  #include "elf-bfd.h"
>  #include "fbsd-tdep.h"
> @@ -283,6 +284,20 @@ fbsd_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
>    return note_data;
>  }
>  
> +static LONGEST
> +fbsd_get_syscall_number (struct gdbarch *gdbarch,
> +			 ptid_t ptid)

Add the usual "implement foo gdbarch method" or some such comment.

> +{

Thanks,
Pedro Alves