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

[Luis Machado <lgustavo at codesourcery dot com>]

On 07/30/2013 03:58 PM, Pedro Alves wrote:
> On 07/24/2013 10:01 PM, Luis Machado wrote:
> Maybe the easiest is something like a
>
> static inline void
> linux_debug (const char *, ...)
> {
>    ...
> }
>
> function whose body is implemented differently for gdb and gdbserver?

This is now linux_debug and it prints no output for GDB for now, only 
for gdbserver.

> > 2 - I had to come up with a little bit of magic to merge the
> > gdbserver-specific code to fork childs and grandchilds
> > (fork/clone-handling) with GDB's. gdbserver supports MMU-less targets
> > and we have a few defines there to either use fork or clone based on
> > that condition. ia64 also has its own little thing going on, so that is
> > honored as well for both GDB and gdbserver. This can be seen in
> > linux_fork_to_function.
>
> > diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
> > index d5ac061..0f1c256 100644
> > --- a/gdb/common/linux-ptrace.c
> > +++ b/gdb/common/linux-ptrace.c
> ...
> > +/* 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 *))
> > +{
> ...
> > +
> > +#if defined(__UCLIBC__) && defined(HAS_NOMMU)
> > +#define STACK_SIZE 4096
>
> But how does this work, given HAS_NOMMU is defined only in linux-low.c?
>
> $ grep -rn "define\( \|\t\)*HAS_NOMMU" * | grep -v testsuite
> gdbserver/linux-low.c:83:#define HAS_NOMMU

Fixed. The definition of HAS_NOMMU has been moved to linux-ptrace.h.

> > 3 - gdbserver explicitly casts the arguments for a ptrace call to cope
> > with odd architectures that require such a cast to be able to pass
> > parameters correctly.
>
> But you didn't point out exactly where the problem was, but I assume
> you're talking about PTRACE_ARG3_TYPE, etc.  GDB has them too, but
> they're called PTRACE_TYPE_ARG3, etc. ...  Guess if we should
> rename the gdbserver ones to follow...

I've renamed all the occurrences of PTRACE_ARG3_TYPE and 
PTRACE_ARG4_TYPE to their GDB counterparts.

I've used PTRACE_TYPE_ARG3/PTRACE_TYPE_ARG4 throughout and made 
gdb/configure.ac define PTRACE_TYPE_ARG4 as well now.


> > +
> > +/* 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) != 0);
>
> Should really be:
>
>    return ((current_ptrace_options & ptrace_options) == ptrace_options);
>
> > +  return ptrace_supports_feature (PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> > +				  | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC);
>
> Won't happen today, as all these were added at the same time to the kernel,
> but with a != 0 check above, if the kernel supported any of those but not
> all, ptrace_supports_feature would still return true with the "!= 0" check.
> You want it to return true when _all_ the requested options are supported.

Well spotted. This is bogus and has been fixed now.

> +/* 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);
>
> Can you wrap comments around 70 columns please?

Done.

> > -/* 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.  */
>
> It seems we lost this comment?

In reality this comment was replaced by a comment inside the function, 
closer to the point where we test this feature...

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

> > -
> > -static void
> > -linux_test_for_tracefork (int original_pid)
>
> > +/* Enable reporting of all currently supported ptrace events.  */
> > +
> > +void
> > +linux_enable_event_reporting (ptid_t ptid)
> > +{
> > +  int pid = ptid_get_lwp (ptid);
> > +
> > +  if (pid == 0)
> > +    pid = ptid_get_pid (ptid);
> > +
> > +  /* 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.  */
> > +
> > +#ifdef GDBSERVER
> > +  /* Disable these options for now since gdbserver does not properly support
> > +     them.  */
> > +  current_ptrace_options &= ~(PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
> > +			      | PTRACE_O_TRACEEXEC | PTRACE_O_TRACEVFORKDONE
> > +			      | PTRACE_O_TRACESYSGOOD);
> > +#endif
>
> Can we just not test/enable them in GDBserver in the first place,
> instead of hacking them away here?  Seems pointless to test for
> options that won't be used for now, and if we ever add a new
> option to GDB, ISTM there's higher risk of forgetting to disable
> it here than if we instead #ifdef GDBSERVER where we test for
> the feature upfront.

Sure. The features are not reported as supported for gdbserver anymore 
as i guarded their enablement with #ifdef blocks inside 
linux_check_ptrace_features.

> > --- /dev/null
> > +++ b/gdb/common/linux-nat-common.c
> > @@ -0,0 +1,134 @@
> > +/* GNU/Linux native-dependent code common to multiple platforms.
> > +
> > +   Copyright (C) 2001-2013 Free Software Foundation, Inc.
> > +
> > +   This file is part of GDB.
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; either version 3 of the License, or
> > +   (at your option) any later version.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > +
> > +#ifdef GDBSERVER
> > +#include "server.h"
> > +#else
> > +#include "defs.h"
> > +#include "signal.h"
> > +#endif
> > +
> > +#include "linux-nat-common.h"
> > +#include "linux-ptrace.h"
> > +#include "gdb_wait.h"
> > +
> > +/* Signals to block to make that sigsuspend work.  */
> > +static sigset_t blocked_mask;
>
> This isn't right.  On linux-nat.c, this is a global because
> it holds the LinuxThreads signals too, initialized by
> lin_thread_get_thread_signals.  Now the block_child_signals
> in linux-nat.c will no longer block the right signals, as
> linux-nat.c:blocked_mask still exists, and that is the
> one that gets the LinuxThreads signals added, not this
> one, but it's this one that block_child_signals operates
> on ...
>
> On gdbserver, the my_waitpid wrapper does:
>
>    if (flags & __WALL)
>      {
>        sigset_t block_mask, org_mask, wake_mask;
>        ^^^^^^^^^^^^^^^^^^^
>
> It's a local, for a reason.  It's because ...
>
>        int wnohang;
>
>        wnohang = (flags & WNOHANG) != 0;
>        flags &= ~(__WALL | __WCLONE);
>        flags |= WNOHANG;
>
>        /* Block all signals while here.  This avoids knowing about
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 	 LinuxThread's signals.  */
>           ^^^^^^^^^^^^^^^^^^^^^^^^^
>        sigfillset (&block_mask);
>        ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> ... we block all signals.  The comment alludes exactly to
> the avoiding of GDBserver doing things differently, and
> avoiding the need for the block_mask global.
>
>        sigprocmask (SIG_BLOCK, &block_mask, &org_mask);

Ok. But what about its use on 
common/linux-ptrace.c:linux_check_ptrace_features. Why does GDB check 
for features differently? Or is this shared mask not needed there at all 
and thus we don't need to call block_child_signals and 
restore_child_signals_mask at all?

I'll send an updated patch once i fully understand the details on 
signal-blocking.

Thanks,
Luis