This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

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

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.

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

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.  */
+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.  */
+  /* Disable these options for now since gdbserver does not properly support
+     them.  */
+  current_ptrace_options &= ~(PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK

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
+   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 <>.  */
+#include "server.h"
+#include "defs.h"
+#include "signal.h"
+#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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]