Re: [PATCH, v2] Share ptrace options discovery/linux native code between GDB and gdbserver

[Pedro Alves <palves at redhat dot com>]

On 08/20/2013 12:27 AM, Luis Machado wrote:

> diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
> index d5ac061..4198d1d 100644
> --- a/gdb/common/linux-ptrace.c
> +++ b/gdb/common/linux-ptrace.c
> @@ -25,10 +25,16 @@
>  
>  #include "linux-ptrace.h"
>  #include "linux-procfs.h"
> +#include "nat/linux-waitpid.h"
>  #include "buffer.h"
>  #include "gdb_assert.h"
>  #include "gdb_wait.h"
>  
> +/* Stores the currently supported ptrace options.  A value of
> +   -1 means we did not check for features yet.  A value of 0 means
> +   there are no supported features.  */
> +static int current_ptrace_options = -1;
> +
>  /* Find all possible reasons we could fail to attach PID and append these
>     newline terminated reason strings to initialized BUFFER.  '\0' termination
>     of BUFFER must be done by the caller.  */
> @@ -222,6 +228,284 @@ linux_ptrace_test_ret_to_nx (void)
>  #endif /* defined __i386__ || defined __x86_64__ */
>  }
>  
> +/* Helper function to fork a process and make the child process call
> +   the function FUNCTION passing ARG as parameter.  */
> +
> +static int
> +linux_fork_to_function (void *arg, void (*function) (void *))

The describing comment and the function prototype imply some
sort of generity.  But ...

> +{
> +  int child_pid;
> +
> +  gdb_byte *stack = (gdb_byte *) arg;

The arg has a definite particular use.

> +
> +  /* Sanity check the function pointer.  */
> +  gdb_assert (function != NULL);
> +
> +#if defined(__UCLIBC__) && defined(HAS_NOMMU)
> +#define STACK_SIZE 4096
> +
> +    if (arg == NULL)
> +      stack = xmalloc (STACK_SIZE * 4);

Etc.  Something's odd with the abstration.

> +
> +    /* Use CLONE_VM instead of fork, to support uClinux (no MMU).  */
> +    #ifdef __ia64__
> +      child_pid = __clone2 (function, stack, STACK_SIZE,
> +			    CLONE_VM | SIGCHLD, stack + STACK_SIZE * 2);
> +    #else /* !__ia64__ */
> +      child_pid = clone (function, stack + STACK_SIZE,
> +			 CLONE_VM | SIGCHLD, stack + STACK_SIZE * 2);
> +  #endif /* !__ia64__ */
> +#else /* !defined(__UCLIBC) && defined(HAS_NOMMU) */
> +  child_pid = fork ();
> +
> +  if (child_pid == 0)
> +    function (stack);
> +#endif /* defined(__UCLIBC) && defined(HAS_NOMMU) */
> +
> +  if (child_pid == -1)
> +    perror_with_name (("fork"));
> +
> +  return child_pid;
> +}
> +


> +
> +/* Determine ptrace features available on this target.  */
> +
> +static void
> +linux_check_ptrace_features (void)
> +{
> +  int child_pid, ret, status;
> +  long second_pid;
> +
> +  /* Initialize the options.  */
> +  current_ptrace_options = 0;
> +
> +  /* Fork a child so we can do some testing.  The child will call
> +     linux_child_function and will get traced.  The child will
> +     eventually fork a grandchild so we can test fork event
> +     reporting.  */
> +  child_pid = linux_fork_to_function (NULL, linux_child_function);
> +
> +  ret = my_waitpid (child_pid, &status, 0);
> +  if (ret == -1)
> +    perror_with_name (("waitpid"));
> +  else if (ret != child_pid)
> +    error (_("linux_check_ptrace_features: waitpid: unexpected result %d."),
> +	   ret);
> +  if (! WIFSTOPPED (status))
> +    error (_("linux_check_ptrace_features: waitpid: unexpected status %d."),
> +	   status);
> +
> +#ifdef GDBSERVER
> +  /* gdbserver does not support PTRACE_O_TRACEFORK yet.  */


This here doesn't look right ...

> +#else
> +  /* First, set the PTRACE_O_TRACEFORK option.  If this fails, we
> +     know for sure that it is not supported.  */
> +  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		PTRACE_O_TRACEFORK);
> +#endif
> +
> +  if (ret != 0)

... as this looks like will always be reached for gdbserver.
The PTRACE_O_TRACEFORK testing is being used in gdbserver as
proxy for close tracing support.

> +    {
> +      ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) 0);
> +      if (ret != 0)
> +	{
> +	  warning (_("linux_check_ptrace_features: failed to kill child"));
> +	  return;
> +	}
> +
> +      ret = my_waitpid (child_pid, &status, 0);
> +      if (ret != child_pid)
> +	warning (_("linux_check_ptrace_features: failed "
> +		   "to wait for killed child"));
> +      else if (!WIFSIGNALED (status))
> +	warning (_("linux_check_ptrace_features: unexpected "
> +		   "wait status 0x%x from killed child"), status);
> +
> +      return;
> +    }
> +
> +#ifdef GDBSERVER
> +  /* gdbserver does not support PTRACE_O_TRACESYSGOOD or
> +     PTRACE_O_TRACEVFORKDONE yet.  */
> +#else
> +  /* Check if the target supports PTRACE_O_TRACESYSGOOD.  */
> +  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		PTRACE_O_TRACESYSGOOD);
> +  current_ptrace_options |= (ret == 0)? PTRACE_O_TRACESYSGOOD : 0;

Space before '?'.

Arguably,

  if (ret == 0)
    current_ptrace_options |= PTRACE_O_TRACESYSGOOD;

would be more straightforward to read.

> +
> +  /* Check if the target supports PTRACE_O_TRACEVFORKDONE.  */
> +  ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORKDONE);
> +  current_ptrace_options |= (ret == 0)? PTRACE_O_TRACEVFORKDONE : 0;

Ditto missing space.

> +#endif
> +
> +  /* Setting PTRACE_O_TRACEFORK did not cause an error, however we
> +     don't know for sure that the feature is available; old
> +     versions of PTRACE_SETOPTIONS ignored unknown options.
> +     Therefore, we attach to the child process, use PTRACE_SETOPTIONS
> +     to enable fork tracing, and let it fork.  If the process exits,
> +     we assume that we can't use PTRACE_O_TRACEFORK; if we get the
> +     fork notification, and we can extract the new child's PID, then
> +     we assume that we can.
> +
> +     We do not explicitly check for vfork tracing here.  It is
> +     assumed that vfork tracing is available whenever fork tracing
> +     is available.  */
> +  ret = ptrace (PTRACE_CONT, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		(PTRACE_TYPE_ARG4) 0);
> +  if (ret != 0)
> +    warning (_("linux_check_ptrace_features: failed to resume child"));
> +
> +  ret = my_waitpid (child_pid, &status, 0);
> +
> +  /* Check if we received a fork event notification.  */
> +  if (ret == child_pid && WIFSTOPPED (status)
> +      && status >> 16 == PTRACE_EVENT_FORK)
> +    {
> +      /* We did receive a fork event notification.  Make sure its PID
> +	 is reported.  */
> +      second_pid = 0;
> +      ret = ptrace (PTRACE_GETEVENTMSG, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) &second_pid);
> +      if (ret == 0 && second_pid != 0)
> +	{
> +	  int second_status;
> +
> +	  /* We got the PID from the grandchild, which means fork
> +	     tracing is supported.  */
> +#ifdef GDBSERVER
> +	  /* Do not enable all the options for now since gdbserver does not
> +	     properly support them.  This restriction will be lifted when
> +	     gdbserver is augmented to support them.  */
> +	  current_ptrace_options |= PTRACE_O_TRACECLONE;
> +#else
> +	  current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> +	    | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
> +#endif
> +
> +	  /* Do some cleanup and kill the grandchild.  */
> +	  my_waitpid (second_pid, &second_status, 0);
> +	  ret = ptrace (PTRACE_KILL, second_pid, (PTRACE_TYPE_ARG3) 0,
> +			(PTRACE_TYPE_ARG4) 0);
> +	  if (ret != 0)
> +	    warning (_("linux_check_ptrace_features: "
> +		       "failed to kill second child"));
> +	  my_waitpid (second_pid, &status, 0);
> +	}
> +    }
> +  else
> +    warning (_("linux_check_ptrace_features: unexpected result from waitpid "
> +	     "(%d, status 0x%x)"), ret, status);
> +
> +  /* Clean things up and kill any pending childs.  */

s/childs/children/.

> +  do
> +    {
> +      ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
> +		    (PTRACE_TYPE_ARG4) 0);
> +      if (ret != 0)
> +	warning ("linux_check_ptrace_features: failed to kill child");
> +      my_waitpid (child_pid, &status, 0);
> +    }
> +  while (WIFSTOPPED (status));
> +}
> +
> +/* Enable reporting of all currently supported ptrace events.  */
> +
> +void
> +linux_enable_event_reporting (ptid_t ptid)

Could you preserve gdbserver's prototype here, please?  That
is, take a single integer pid rather than a ptid.

> +{
> +  int pid = ptid_get_lwp (ptid);
> +
> +  if (pid == 0)
> +    pid = ptid_get_pid (ptid);

This dance is only really necessary for GDB.  Given
this is working at the ptrace level, passing in the pid
only is more to the point.  (E.g., passing in a ptid makes
the reader wonder whether PTID can be represent a whole
inferior, and thus event reporting enablement gets applied to
all threads).

> +
> +  /* 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 ();
> +
> +  /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
> +     support read-only process state.  */

This comment really belongs close to where current_ptrace_options
is set.  That in, in the original code, read it as attached to
the "current_ptrace_options |= " bits above, not the ptrace call
below.

> +
> +  /* Set the options.  */
> +  ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0,
> +	  current_ptrace_options);
> +}
> +
> +/* Returns non-zero if PTRACE_OPTIONS is contained within
> +   CURRENT_PTRACE_OPTIONS, therefore supported.  Returns 0
> +   otherwise.  */
> +
> +static int
> +ptrace_supports_feature (int ptrace_options)
> +{
> +  gdb_assert (current_ptrace_options >= 0);
> +
> +  return ((current_ptrace_options & ptrace_options) == ptrace_options);
> +}
> +
> +/* Returns non-zero if PTRACE_O_TRACEFORK, PTRACE_O_TRACEVFORK,
> +   PTRACE_O_TRACECLONE and PTRACE_O_TRACEEXEC are supported by
> +   ptrace, 0 otherwise  */

Missing period.  Given the name of the function, I'd suggest instead:

/* Returns non-zero if PTRACE_EVENT_FORK is supported by ptrace,
   0 otherwise.  Note that if PTRACE_EVENT_FORK is supported so is
   PTRACE_EVENT_CLONE, PTRACE_EVENT_EXEC and PTRACE_EVENT_VFORK,
   since they were all added to the kernel at the same time.

> +
> +int
> +linux_supports_tracefork (void)
> +{
> +  return ptrace_supports_feature (PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> +				  | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC);

Well, this is now wrong given gdbserver will only set PTRACE_O_TRACECLONE
in the current_ptrace_options.  Given the revised comment above,
this can all be replaced with:

  return ptrace_supports_feature (PTRACE_O_TRACECLONE);

and so it'll work for both gdb and gdbserver.  When gdbserver supports
tracing forks, we can go back here and check PTRACE_O_TRACEFORK instead,
just for clarity of the code.  WDYT?

BTW, these function are extern, and the same describing comment is
duplicated in the declarations in the header file.  Best leave only one
copy, near the declarations.

> +}
> +
> +/* Returns non-zero if PTRACE_O_TRACEVFORKDONE is supported by
> +   ptrace, 0 otherwise  */
> +
> +int
> +linux_supports_tracevforkdone (void)
> +{
> +  return ptrace_supports_feature (PTRACE_O_TRACEVFORKDONE);
> +}
> +
> +/* Returns non-zero if PTRACE_O_TRACESYSGOOD is supported by ptrace,
> +   0 otherwise  */
> +
> +int
> +linux_supports_tracesysgood (void)
> +{
> +  return ptrace_supports_feature (PTRACE_O_TRACESYSGOOD);
> +}
> +
>  /* Display possible problems on this system.  Display them only once per GDB
>     execution.  */
>  
> diff --git a/gdb/common/linux-ptrace.h b/gdb/common/linux-ptrace.h
> index 8f02c82..005da3d 100644
> --- a/gdb/common/linux-ptrace.h
> +++ b/gdb/common/linux-ptrace.h
> @@ -22,6 +22,24 @@ struct buffer;
>  

> +#ifdef GDBSERVER
> +#if !defined (PTRACE_TYPE_ARG3)

Why the #if !defines if the PTRACE_TYPE_... definitions
have been removed from linux-low.h ?

> +#define PTRACE_TYPE_ARG3 void *
> +#endif
> +
> +#if !defined (PTRACE_TYPE_ARG4)
> +#define PTRACE_TYPE_ARG4 void *
> +#endif
> +#endif /* GDBSERVER */


> +
>  #ifndef PTRACE_GETSIGINFO
>  # define PTRACE_GETSIGINFO 0x4202
>  # define PTRACE_SETSIGINFO 0x4203
> @@ -70,4 +88,19 @@ struct buffer;
>  extern void linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer);
>  extern void linux_ptrace_init_warnings (void);
>  
> +/* Enable reporting of all currently supported ptrace events.  */
> +extern void linux_enable_event_reporting (ptid_t ptid);
> +
> +/* Returns non-zero if PTRACE_EVENT_FORK, PTRACE_EVENT_CLONE, PTRACE_EVENT_EXEC
> +   and PTRACE_EVENT_VFORK are supported by ptrace, 0 otherwise  */
> +extern int linux_supports_tracefork (void);
> +
> +/* Returns non-zero if PTRACE_O_TRACEVFORKDONE is supported by ptrace,
> +   0 otherwise  */
> +extern int linux_supports_tracevforkdone (void);
> +
> +/* Returns non-zero if PTRACE_O_TRACESYSGOOD is supported by ptrace,
> +   0 otherwise  */
> +extern int linux_supports_tracesysgood (void);
> +
>  #endif /* COMMON_LINUX_PTRACE_H */
> diff --git a/gdb/config.in b/gdb/config.in
> index 76abd04..5a80001 100644
> --- a/gdb/config.in
> +++ b/gdb/config.in
> @@ -659,6 +659,9 @@
>  /* Define to the type of arg 3 for ptrace. */
>  #undef PTRACE_TYPE_ARG3
>  
> +/* Define to the type of arg 3 for ptrace. */
> +#undef PTRACE_TYPE_ARG4

Off by one in comment...

> +
>  /* Define to the type of arg 5 for ptrace. */
>  #undef PTRACE_TYPE_ARG5
>  

> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 667821f..0982cac 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1207,7 +1207,7 @@ AC_CACHE_CHECK([types of arguments for ptrace], gdb_cv_func_ptrace_args, [
>  for gdb_arg1 in 'int' 'long'; do
>   for gdb_arg2 in 'pid_t' 'int' 'long'; do
>    for gdb_arg3 in 'int *' 'caddr_t' 'int' 'long' 'void *'; do
> -   for gdb_arg4 in 'int' 'long'; do
> +   for gdb_arg4 in 'int' 'long' 'void *'; do
>       AC_TRY_COMPILE($gdb_ptrace_headers, [
>  extern $gdb_cv_func_ptrace_ret
>    ptrace ($gdb_arg1, $gdb_arg2, $gdb_arg3, $gdb_arg4);
> @@ -1234,6 +1234,8 @@ IFS=$ac_save_IFS
>  shift
>  AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG3, $[3],
>    [Define to the type of arg 3 for ptrace.])
> +AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG4, $[4],
> +  [Define to the type of arg 4 for ptrace.])

... but here the comment looks right, so I think you just
forgot to regenerate config.in.

>  if test -n "$[5]"; then
>    AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG5, $[5],
>      [Define to the type of arg 5 for ptrace.])
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index 2cdbf47..04c061a 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -100,6 +100,11 @@ GNULIB_H = $(GNULIB_BUILDDIR)/import/string.h @GNULIB_STDINT_H@
>  # -I. for config files.
>  # -I${srcdir} for our headers.
>  # -I$(srcdir)/../regformats for regdef.h.
> +#
> +# We do not include ../target or ../nat in here because headers
> +# in those directories should be included with the subdirectory.
> +# e.g.: "target/wait.h
> +#

Missing final quote, and probably period:

  # E.g.: "target/wait.h".

>  INCLUDE_CFLAGS = -I. -I${srcdir} -I$(srcdir)/../common \
>  	-I$(srcdir)/../regformats -I$(srcdir)/../ -I$(INCLUDE_DIR) \
>  	$(INCGNU)
> @@ -157,8 +162,10 @@ SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
>  	$(srcdir)/common/common-utils.c $(srcdir)/common/xml-utils.c \
>  	$(srcdir)/common/linux-osdata.c $(srcdir)/common/ptid.c \
>  	$(srcdir)/common/buffer.c $(srcdir)/common/linux-btrace.c \
> -	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
> -    $(srcdir)/common/mips-linux-watch.c
> +	$(srcdir)/common/filestuff.c $(srcdir)/common/linux-ptrace.c \
> +	$(srcdir)/common/mips-linux-watch.c \
> +	$(srcdir)/target/waitstatus.c \
> +	$(srcdir)/nat/linux-waitpid.c \

Hmm, so linux-ptrace.c was missing?  (It's a bit harder than expected to
see what's doing on in this hunk, given the order's been changed.)
It seems linux-procfs.c is missing too, and probably others.  Please do that
as separate patch...  This is penance for forgetting that backslash in the
last line, and forcing me to look closer.  ;-)

>  
>  DEPFILES = @GDBSERVER_DEPFILES@
>  
> @@ -447,7 +454,8 @@ server_h = $(srcdir)/server.h $(regcache_h) $(srcdir)/target.h \
>  		$(generated_files)
>  
>  gdbthread_h = $(srcdir)/gdbthread.h $(target_h) $(srcdir)/server.h
> -linux_low_h = $(srcdir)/linux-low.h $(gdbthread_h)
> +linux_low_h = $(srcdir)/linux-low.h $(srcdir)/../nat/linux-nat.h \
> +	      $(srcdir)/../nat/linux-waitpid.h $(gdbthread_h)

Is this really necessary, given we now have automatic dependencies
in gdbserver?  $linux_low_h even used anywhere at all.  Looks
like these variables are just waiting to be garbage collected...

>  
>  linux_ptrace_h = $(srcdir)/../common/linux-ptrace.h
>  
> @@ -562,6 +570,12 @@ linux-btrace.o: ../common/linux-btrace.c $(linux_btrace_h) $(server_h)
>  mips-linux-watch.o: ../common/mips-linux-watch.c $(mips_linux_watch_h) $(server_h)
>  	$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $<
>  
> +# Shared native object files rules from ../nat
> +
> +linux-waitpid.o: ../nat/linux-waitpid.c
> +	$(COMPILE) $<
> +	$(POSTCOMPILE)
> +
>  # We build vasprintf with -DHAVE_CONFIG_H because we want that unit to
>  # include our config.h file.  Otherwise, some system headers do not get
>  # included, and the compiler emits a warning about implicitly defined
> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index b9dfd6c..b5b878c 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -39,15 +39,18 @@ srv_amd64_xmlfiles="i386/amd64.xml i386/amd64-avx.xml i386/x32.xml i386/x32-avx.
>  srv_i386_linux_xmlfiles="i386/i386-linux.xml i386/i386-avx-linux.xml i386/i386-mmx-linux.xml i386/32bit-linux.xml $srv_i386_32bit_xmlfiles"
>  srv_amd64_linux_xmlfiles="i386/amd64-linux.xml i386/amd64-avx-linux.xml i386/64bit-linux.xml i386/x32-linux.xml i386/x32-avx-linux.xml $srv_i386_64bit_xmlfiles"
>  
> +
> +# Native linux object files.  This is so we don't have to repeat
> +# these files over and over again.
> +srv_native_linux_obj="linux-waitpid.o linux-low.o linux-osdata.o linux-procfs.o"

Let's just drop the "native" bit.  All objects in this file are native:

 # Linux object files.  This is so we don't have to repeat
 # these files over and over again.
 srv_linux_obj="linux-waitpid.o linux-low.o linux-osdata.o linux-procfs.o"

Thanks,
-- 
Pedro Alves