This is the mail archive of the
mailing list for the GDB project.
Re: [PATCH] Share ptrace options discovery/linux native code between GDB and gdbserver
- From: Luis Machado <lgustavo at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "'gdb-patches at sourceware dot org'" <gdb-patches at sourceware dot org>
- Date: Tue, 06 Aug 2013 12:28:42 -0300
- Subject: Re: [PATCH] Share ptrace options discovery/linux native code between GDB and gdbserver
- References: <51F04092 dot 2070008 at codesourcery dot com> <51F80CDB dot 3050106 at redhat dot com>
- Reply-to: 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
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
+/* Helper function to fork a process and make the child process call the
+ function FUNCTION passing ARG as parameter. */
+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
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
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. */
+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?
-/* 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. */
-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
+ | PTRACE_O_TRACEEXEC | PTRACE_O_TRACEVFORKDONE
+ | PTRACE_O_TRACESYSGOOD);
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
@@ -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/>. */
+/* 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 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 ...
wnohang = (flags & WNOHANG) != 0;
flags &= ~(__WALL | __WCLONE);
flags |= WNOHANG;
/* Block all signals while here. This avoids knowing about
LinuxThread's signals. */
... 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