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

[Pedro Alves <palves at redhat dot com>]

On 08/21/2013 04:08 AM, Luis Machado wrote:
> Hi,
> 
> On 08/20/2013 01:39 PM, Pedro Alves wrote:
>> 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.
>>
> 
> 
> I've improved this now. We have a special case for MMU-less targets 
> (gdbserver's side), though it is not quite clear due to the lack of 
> comments in that code.

IMO, we're still not there.  

> /* Helper function to fork a process and make the child process call
>    the function FUNCTION passing ARG as parameter.
> 
>    For MMU-less targets, if ARG is NULL, stack space is allocated
>    via malloc and passed to FUNCTION.  */
> 
> static int
> linux_fork_to_function (void *arg, void (*function) (void *))

This still suggests that ARG is something generic, while it is
not.  I'm thinking this would make things clearer:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
---
 gdb/common/linux-ptrace.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index bcd3c94..2fb6dd8 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -229,18 +229,17 @@ linux_ptrace_test_ret_to_nx (void)
 }
 
 /* Helper function to fork a process and make the child process call
-   the function FUNCTION passing ARG as parameter.
+   the function FUNCTION, passing CHILD_STACK as parameter.
 
-   For MMU-less targets, if ARG is NULL, stack space is allocated
-   via malloc and passed to FUNCTION.  */
+   For MMU-less targets, clone is used instead of fork, and
+   CHILD_STACK is used as stack space for the cloned child.  If NULL,
+   stack space is allocated via malloc (and subsequently passed to
+   FUNCTION).  For MMU targets, CHILD_STACK is ignored.  */
 
 static int
-linux_fork_to_function (void *arg, void (*function) (void *))
+linux_fork_to_function (gdb_byte *child_stack, void (*function) (gdb_byte *))
 {
   int child_pid;
-#if defined(__UCLIBC__) && defined(HAS_NOMMU)
-  gdb_byte *stack;
-#endif
 
   /* Sanity check the function pointer.  */
   gdb_assert (function != NULL);
@@ -248,24 +247,22 @@ linux_fork_to_function (void *arg, void (*function) (void *))
 #if defined(__UCLIBC__) && defined(HAS_NOMMU)
 #define STACK_SIZE 4096
 
-    if (arg == NULL)
-      stack = xmalloc (STACK_SIZE * 4);
-    else
-      stack = (gdb_byte *) arg;
+    if (child_stack == NULL)
+      child_stack = xmalloc (STACK_SIZE * 4);
 
     /* 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);
+      child_pid = __clone2 (function, child_stack, STACK_SIZE,
+			    CLONE_VM | SIGCHLD, child_stack + STACK_SIZE * 2);
     #else /* !__ia64__ */
-      child_pid = clone (function, stack + STACK_SIZE,
-			 CLONE_VM | SIGCHLD, stack + STACK_SIZE * 2);
+      child_pid = clone (function, child_stack + STACK_SIZE,
+			 CLONE_VM | SIGCHLD, child_stack + STACK_SIZE * 2);
   #endif /* !__ia64__ */
 #else /* !defined(__UCLIBC) && defined(HAS_NOMMU) */
   child_pid = fork ();
 
   if (child_pid == 0)
-    function (arg);
+    function (NULL);
 #endif /* defined(__UCLIBC) && defined(HAS_NOMMU) */
 
   if (child_pid == -1)
@@ -278,10 +275,10 @@ linux_fork_to_function (void *arg, void (*function) (void *))
    the child forks a grandchild.  */
 
 static void
-linux_grandchild_function (void *arg)
+linux_grandchild_function (gdb_byte *child_stack)
 {
   /* Free any allocated stack.  */
-  xfree (arg);
+  xfree (child_stack);
 
   /* This code is only reacheable by the grandchild (child's child)
      process.  */
@@ -293,13 +290,13 @@ linux_grandchild_function (void *arg)
    be traced by its parent.  */
 
 static void
-linux_child_function (void *arg)
+linux_child_function (gdb_byte *child_stack)
 {
   ptrace (PTRACE_TRACEME, 0, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0);
   kill (getpid (), SIGSTOP);
 
   /* Fork a grandchild.  */
-  linux_fork_to_function (arg, linux_grandchild_function);
+  linux_fork_to_function (child_stack, linux_grandchild_function);
 
   /* This code is only reacheable by the child (grandchild's parent)
      process.  */
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(BTW, this is a case of mixing code motion with code changes.  It'd
have been easier if this refactoring on the gdbserver side was made before
sharing code with GDB.  As is, it's hard to compare the new version with
either current GDB's or GDBserver's.)

>>> +
>>> +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?
> 
> I don't think it looks great, but it should do the job for the moment 
> while we teach gdbserver to support the other features. Good enough maybe?

Well we could do better.  If you look at gdbserver's usage of
linux_supports_tracefork:

  /* If the kernel supports tracing forks then it also supports tracing
     clones, and then we don't need to use the magic thread event breakpoint
     to learn about threads.  */
  thread_db_init (!linux_supports_tracefork ());

... that suggests PTRACE_O_TRACECLONE is really the right
flag to check, and actually we should rename the function:

int
linux_supports_traceclone (void)
{
  return ptrace_supports_feature (PTRACE_O_TRACECLONE);
}

... but then, if we look at GDB's use of linux_supports_tracefork,
then we see it's really used to check for fork support.  So,
what we really need is both:

int
linux_supports_traceclone (void)
{
  return ptrace_supports_feature (PTRACE_O_TRACECLONE);
}

int
linux_supports_tracefork (void)
{
  return ptrace_supports_feature (PTRACE_O_TRACEFORK);
}

gdbserver's linux_look_up_symbols would then be made to use
the former.  

(I don't think there's any real need to check more
than PTRACE_O_TRACEFORK in the latter function, and honestly,
checking for PTRACE_O_TRACEEXEC looked odd, given the function
is called ..._tracefork.  I think just the comment mentioning
these flags were added at the same time it sufficient).

> 
>>
>> 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.
>>
> 
> Done.
> 
>>> 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 ?
>>
> 
> I've replaced these with a configure-time check in configure.ac as 
> Tromey hinted at, similar to GDB's. Looks cleaner this way.

GDB does this at configure time because of shared modules like
inf-ptrace.c.  gdbserver doesn't have this, so hardcoding the
ptrace types is sufficient.  I have no issue with going the
configure.ac route, but if going that direction, then please
don't duplicate a bunch of code like that.  Especially not
in a patch that whose main purpose is to reduce duplication (!).
We should instead move all that ptrace autoconf code to say, a
new gdb/nat/ptrace.m4 file, included by both gdb's and gdbserver's
acinclude.m4.  I think this actually changes types of the 3rd
and 4th arguments to long, from the current void*.  Shouldn't
affect anything in practice (as long as the PTRACE_TYPE_FOO casts are
in place), but it's yet another change that could be done
separately.

> I've also included linux-ptrace.o by default in the linux object files 
> list (in configure.srv).
> 
> All the other small nits are hopefully fixed now as well.
> 
> Regression-tested again. Results look good.


> +/* 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);
> +
> +  /* 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);

BTW, I've now noticed several places miss the PTRACE_TYPE_ARG4 cast.

> +
> +/* Enable reporting of all currently supported ptrace events.  */
> +
> +void
> +linux_enable_event_reporting (pid_t pid)
> +{
> +  /* 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 ();
> +
> +  /* 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_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_TRACECLONE);
> +}
> +
> +/* Returns non-zero if PTRACE_O_TRACEVFORKDONE is supported by
> +   ptrace, 0 otherwise  */

Missing period.

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

Ditto.

> diff --git a/gdb/config/powerpc/spu-linux.mh b/gdb/config/powerpc/spu-linux.mh
> index 1bc279a..200fe6b 100644
> --- a/gdb/config/powerpc/spu-linux.mh
> +++ b/gdb/config/powerpc/spu-linux.mh
> @@ -3,6 +3,6 @@
>  # This implements a 'pseudo-native' GDB running on the
>  # PPU side of the Cell BE and debugging the SPU side.
>  
> -NATDEPFILES = spu-linux-nat.o fork-child.o inf-ptrace.o \
> +NATDEPFILES = spu-linux-nat.o fork-child.o linux-waitpid.o linux-waitpid.o inf-ptrace.o \
>  	      linux-procfs.o linux-ptrace.o

"linux-waitpid.o linux-waitpid.o" added twice.  (It's a pity
linux-waitpid.o right next to the other linux-foo.o objects, here
and elsewhere, maintaining the somewhat existing order...)  I wouldn't
say anything if it weren't for this duplication here.  ;-)

> diff --git a/gdb/gdbserver/config.in b/gdb/gdbserver/config.in
> index dada2fb..c00d0e4 100644
> --- a/gdb/gdbserver/config.in
> +++ b/gdb/gdbserver/config.in
> @@ -26,6 +26,10 @@
>     */
>  #undef HAVE_DECL_PERROR
>  
> +/* Define to 1 if you have the declaration of `ptrace', and to 0 if you don't.
> +   */
> +#undef HAVE_DECL_PTRACE
> +
>  /* Define to 1 if you have the declaration of `strerror', and to 0 if you
>     don't. */
>  #undef HAVE_DECL_STRERROR
> @@ -76,6 +80,9 @@
>  /* Define to 1 if you have the `dl' library (-ldl). */
>  #undef HAVE_LIBDL
>  
> +/* Define to 1 if you have the `mcheck' library (-lmcheck). */
> +#undef HAVE_LIBMCHECK

Hmm.  Guess I forgot to update in config.in when adding
libmcheck support to gdbserver...  Should be done separately,
in case it causes trouble.  I'll fix that.

> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
> index b9dfd6c..29f89c4 100644
> --- a/gdb/gdbserver/configure.srv
> +++ b/gdb/gdbserver/configure.srv
> @@ -39,16 +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"
>  
> +
> +# 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 linux-ptrace.o"

"native" here stuck behind.  Please, s/srv_native_linux_obj/srv_linux_obj/.

> @@ -316,15 +304,14 @@ case "${target}" in
>  			srv_xmlfiles="${srv_xmlfiles} tic6x-core.xml"
>  			srv_xmlfiles="${srv_xmlfiles} tic6x-gp.xml"
>  			srv_xmlfiles="${srv_xmlfiles} tic6x-c6xp.xml"
> -  			srv_tgtobj="linux-low.o linux-osdata.o linux-tic6x-low.o linux-procfs.o"
> -			srv_tgtobj="${srv_tgtobj} linux-ptrace.o"
> +  			srv_tgtobj="$srv_native_linux_obj linux-tic6x-low.o"

Spaces vs tabs mixup here.

>  			srv_linux_regsets=yes
>  			srv_linux_usrregs=yes
>  			srv_linux_thread_db=yes
>  			;;


> @@ -1998,7 +1916,7 @@ linux_wait_for_event (ptid_t ptid, int *wstat, int options)
>  
>        if (event_child->must_set_ptrace_flags)
>  	{
> -	  linux_enable_event_reporting (lwpid_of (event_child));
> +	  linux_enable_event_reporting (pid_of (event_child));

This change is wrong.  We really want the lwpid of event_child,
not the overall process id.


> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index db23433..0aac73a 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c

...

> -/* Determine if PTRACE_O_TRACEFORK can be used to follow fork events.
> -
> -   First, we try to enable fork tracing on ORIGINAL_PID.  If this fails,
> -   we know that the feature is not available.  This may change the tracing
> -   options for ORIGINAL_PID, but we'll be setting them shortly anyway.
> -
> -   However, if it succeeds, we don't know for sure that the feature is
> -   available; old versions of PTRACE_SETOPTIONS ignored unknown options.  We
> -   create a child process, attach to it, use PTRACE_SETOPTIONS to enable
> -   fork tracing, and let it fork.  If the process exits, we assume that we
> -   can't use TRACEFORK; if we get the fork notification, and we can extract
> -   the new child's PID, then we assume that we can.  */
> +/* Initialize ptrace warnings and check for supported ptrace
> +   features given PTID.  */

given PID.

> @@ -1429,7 +1190,7 @@ lin_lwp_attach_lwp (ptid_t ptid)
>  
>        if (ptrace (PTRACE_ATTACH, lwpid, 0, 0) < 0)
>  	{
> -	  if (linux_supports_tracefork_flag)
> +	  if (linux_supports_tracefork ())
>  	    {
>  	      /* If we haven't stopped all threads when we get here,
>  		 we may have seen a thread listed in thread_db's list,
> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
> index cb8f1da..855c802 100644
> --- a/gdb/linux-nat.h
> +++ b/gdb/linux-nat.h
> @@ -128,8 +128,6 @@ void linux_proc_pending_signals (int pid, sigset_t *pending,
>  				 sigset_t *blocked, sigset_t *ignored);
>  
>  /* linux-nat functions for handling fork events.  */
> -extern void linux_enable_event_reporting (ptid_t ptid);
> -

Looks like that comment ends up stale.

>  extern int lin_lwp_attach_lwp (ptid_t ptid);
>  
>  extern void linux_stop_lwp (struct lwp_info *lwp);
-- 
Pedro Alves