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

[Pedro Alves <palves at redhat dot com>]

On 07/24/2013 10:01 PM, Luis Machado wrote:
> Hi,
> 
> This is another cleanup/commonalization patch that serves as a base for 
> the future remote fork/exec support.
> 
> Both GDB and gdbserver check for ptrace options in similar ways, but 
> each code does its own thing.
> 
> The patch moves the ptrace option checks from 
> linux-nat.*/gdbserver/linux-low.c to common/linux-ptrace.c. The code is 
> a little entangled with the native linux stuff too, so some code gets 
> moved to common/linux-nat-common.* instead.

Again, I've rather avoid "common" in file names.  Let's name the files for
what they contain, not for the fact that they're "common" to two programs
(gdb, gdbserver).  We need to look at the end goal (with a broad,
foggy view, but still).  If we're ending up with a single linux backend,
and if it could be done in a single step, we could do it like this:

gdb/linux-nat.c, gdb/gdbserver/linux-low.c -> gdb/common/linux-low.c

(I'd rather split out all the target backend stuff into its own
directory rather than a "common" kitchen sink, but let's ignore
that for now).

But we won't do such a move in a single step.  There's be several
intermediate, baby steps.  Using your suggestion, the patch would
be, e.g.:

linux-nat.c, gdbserver/linux-low.c
   -> linux-nat.c, gdbserver/linux-low.c, common/linux-nat-common.c
      -> gdbserver/linux-low.c, common/linux-nat-common.c
         -> common/linux-low.c, common/linux-nat-common.c
             -> common/linux-low.c

That's a roundabout way of ending up with all code in a
single file anyway.  And it left us with a weird linux-nat-common.c file
in a few intermediate steps where it no longer hold code used by the
gdb side backend (because by then there's no gdb side backend!).  If the
code within is well isolated, we could even consider not merging it
back into linux-low.c in the end, keeping the backend split in smaller
modules.  But in order to do that, we best name it something actually
indicative or its contents, and avoid coming up with new kitchen sinks.
Say, this file holds the waitpid/__WALL/EINTR wrapper replacement, so
what about linux-waitpid.c ?  Then:

linux-nat.c, gdbserver/linux-low.c,
   -> linux-nat.c, gdbserver/linux-low.c, common/linux-waitpid.c
      -> gdbserver/linux-low.c, common/linux-waitpid.c
           -> common/linux-low.c, common/linux-waitpid.c

(and common/linux-ptrace.c for ptrace options and wrappers, etc.)

> 
> The variable that stores all the supported ptrace options is kept in 
> common/linux-ptrace.c now, and GDB/gdbserver can query features 
> (tracesysgood, tracevfork etc) via helper functions. It seems a bit 
> cleaner this way.
> 
> With this change GDB and gdbserver share almost the same code for those 
> checks, leading to less duplication.
> 
> Up for discussion are a few things though:
> 
> 1 - There is a bit of gdbserver-specific code that disallows 
> PTRACE_O_TRACEFORK and friends so gdbserver keeps the same behavior even 
> though those options are available for Linux. This is temporary so we 
> don't break gdbserver while remote fork is being developed. You can see 
> this block in linux_enable_event_reporting.

That's fine.

> There is also a gdbserver-specific block for gdbserver debugging 
> messages. Maybe we can get rid of those and come up with something 
> common between GDB and gdbserver as well.

Maybe the easiest is something like a

static inline void
linux_debug (const char *, ...)
{
  ...
}

function whose body is implemented differently for gdb and 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


> 
> 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'm not too sure what to do about this one. We can 
> either have more #ifdef blocks (in a way, defeating the purpose of 
> having common code)

Please don't.

> or just unconditionally use a #define for a cast and 
> create such a #define at build-time (either GDB or gdbserver) with the 
> correct casting type or no casting at all.


> 
> 4 - Is linux-nat-common.* an acceptable naming pattern? It seems to me 
> that in the future we will eventually have lots of these, one for each 
> architecture as we share more and more backend code.

We'll share more and more, until we eventually share all, and it'll no
longer be "common", but rather a single "library" with multiple
users.  :-)

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

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


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

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

> +
> +  /* Set the options.  */
> +  ptrace (PTRACE_SETOPTIONS, pid, 0, current_ptrace_options);
> +}


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


> +
> +/* Wrapper function for waitpid which handles EINTR, and emulates
> +   __WALL for systems where that is not available.  */
> +
> +int
> +my_waitpid (int pid, int *status, int flags)
> +{
> +  int ret, out_errno;
> +
> +#ifdef GDBSERVER
> +  if (debug_threads)
> +    fprintf (stderr, "my_waitpid (%d, 0x%x)\n", pid, flags);
> +#endif
> +
> +  if (flags & __WALL)
> +    {
> +      sigset_t block_mask, org_mask, wake_mask;

Here, you still have the local.  And that's fine.

> +      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);
> +      sigprocmask (SIG_BLOCK, &block_mask, &org_mask);

...

> +/* Block child signals (SIGCHLD and linux threads signals), and store
> +   the previous mask in PREV_MASK.  */
> +
> +void
> +block_child_signals (sigset_t *prev_mask)
> +{

So it seems like we can leave block_child_signals, etc.
in linux-nat.c.