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.

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

I've removed the guards now. gdbserver should be able to run this chunk 
of code and lie about not supporting PTRACE_O_TRACEFORK later on.

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

I've used your suggestion now.

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

Done.

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

Moved closer to where it belongs.


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

Done. Thanks.

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

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

Thanks Tom.

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

These are gone now.

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

Fixed by regenerating the required files.

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

The identation problem with the mips-linux-watch.c entry bothered me, 
but turns out the inclusion of linux-ptrace.c and linux-waitpid.c isn't 
needed at all. I have a successful build without any change to SFILES.

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

Probably not. I removed this change now.

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.

Thanks!
Luis