Re: [PATCH 1/4][REPOST] Remote Linux ptrace exit events

Tom Tromey
2014-05-21T15:15:45Z

>>>>> "Don" == Don Breazeal <donb@codesourcery.com> writes:

Don> This patch implements support for the extended ptrace event
Don> PTRACE_EVENT_EXIT on Linux. This is a preparatory patch for exec event
Don> support.

Thanks.

I had a few questions about this patch.

Don> * common/linux-ptrace.c (linux_test_for_tracefork)
Don> [GDBSERVER]: Add support for PTRACE_EVENT_EXIT if the OS
Don> supports it.

I'm curious why PTRACE_O_TRACEEXIT is needed by gdbserver to implement
this feature. It isn't needed by gdb. And, I think it's preferable to
try to push the two back ends closer together -- it's a long-term goal
-- so new divergences are subject to special scrutiny.

Don> +#ifdef GDBSERVER
Don> + /* PTRACE_O_FORK is supported, so now test for PTRACE_O_TRACEEXIT.
Don> + First try to set the option. If this fails, we know for sure that
Don> + it is not supported. */
Don> + ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
Don> + (PTRACE_TYPE_ARG4) PTRACE_O_TRACEEXIT);
Don> + if (ret != 0)
Don> + return;
[...]

It would be much nicer to reduce the use of #ifdef GDBSERVER, rather
than to add to it.

I think this could be done a different way, say by having the clients of
this interface specify which flags they're interested in. Then
gdbserver could ask for PTRACE_O_TRACEEXIT and gdb could refrain.

This would be just as simple but still be a good fit for the long-term
goal.

I've appended a patch I wrote along these lines -- perhaps hacky,
definitely untested -- from my experiment with moving gdbserver to
top-level. Feel free to use or ignore it.

Don> + else if (event == PTRACE_EVENT_EXIT)
Don> + {
Don> + unsigned long exit_status;
Don> + unsigned long lwpid = lwpid_of (event_thr);
Don> + int ret;
Don> +
Don> + if (debug_threads)
Don> + debug_printf ("LHEW: Got exit event from LWP %ld\n",
Don> + lwpid_of (event_thr));
Don> +
Don> + ptrace (PTRACE_GETEVENTMSG, lwpid_of (event_thr), (PTRACE_TYPE_ARG3) 0,
Don> + &exit_status);
Don> +
Don> + if (num_lwps (pid_of (event_thr)) > 1)
Don> + {
Don> + /* If there is at least one more LWP, then the program still
Don> + exists and the exit should not be reported to GDB. */

I thought, from the man page, that PTRACE_EVENT_EXIT was for a process
exit event only. So checking num_lwps doesn't seem correct here. But
after seeing your patch I am not sure; and I would like to know the
answer.

Tom

commit a1b6a7417e0e192c8f925ac94491a819c384c7e9
Author: Tom Tromey <tromey@redhat.com>
Date: Fri Jan 3 10:55:52 2014 -0700

remove some GDBSERVER checks from linux-ptrace

diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c
index 7c1b78a..d1659e0 100644
--- a/gdb/common/linux-ptrace.c
+++ b/gdb/common/linux-ptrace.c
@@ -37,6 +37,10 @@
there are no supported features. */
static int current_ptrace_options = -1;

+/* Additional flags to test. */
+
+static int additional_flags;
+
/* 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. */
@@ -359,16 +363,15 @@ linux_check_ptrace_features (void)
static void
linux_test_for_tracesysgood (int child_pid)
{
-#ifdef GDBSERVER
- /* gdbserver does not support PTRACE_O_TRACESYSGOOD. */
-#else
- int ret;
+ if ((additional_flags & PTRACE_O_TRACESYSGOOD) != 0)
+ {
+ int ret;

- ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
- if (ret == 0)
- current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
-#endif
+ ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+ (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
+ if (ret != 0)
+ additional_flags &= ~PTRACE_O_TRACESYSGOOD;
+ }
}

/* Determine if PTRACE_O_TRACEFORK can be used to follow fork
@@ -388,16 +391,15 @@ linux_test_for_tracefork (int child_pid)
if (ret != 0)
return;

-#ifdef GDBSERVER
- /* gdbserver does not support PTRACE_O_TRACEVFORKDONE yet. */
-#else
- /* Check if the target supports PTRACE_O_TRACEVFORKDONE. */
- ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
- (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
- | PTRACE_O_TRACEVFORKDONE));
- if (ret == 0)
- current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
-#endif
+ if ((additional_flags & PTRACE_O_TRACEVFORKDONE) != 0)
+ {
+ /* Check if the target supports PTRACE_O_TRACEVFORKDONE. */
+ ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
+ (PTRACE_TYPE_ARG4) (PTRACE_O_TRACEFORK
+ | PTRACE_O_TRACEVFORKDONE));
+ if (ret != 0)
+ additional_flags &= ~PTRACE_O_TRACEVFORKDONE;
+ }

/* Setting PTRACE_O_TRACEFORK did not cause an error, however we
don't know for sure that the feature is available; old
@@ -433,18 +435,7 @@ linux_test_for_tracefork (int child_pid)

/* We got the PID from the grandchild, which means fork
tracing is supported. */
-#ifdef GDBSERVER
- /* Do not enable all the options for now since gdbserver does not
- properly support them. This restriction will be lifted when
- gdbserver is augmented to support them. */
- current_ptrace_options |= PTRACE_O_TRACECLONE;
-#else
- current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK
- | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC;
-
- /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
- support read-only process state. */
-#endif
+ current_ptrace_options |= PTRACE_O_TRACECLONE | additional_flags;

/* Do some cleanup and kill the grandchild. */
my_waitpid (second_pid, &second_status, 0);
@@ -542,3 +533,9 @@ linux_ptrace_init_warnings (void)

linux_ptrace_test_ret_to_nx ();
}
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+ additional_flags = flags;
+}
diff --git a/gdb/common/linux-ptrace.h b/gdb/common/linux-ptrace.h
index 38bb9ea..e5094df 100644
--- a/gdb/common/linux-ptrace.h
+++ b/gdb/common/linux-ptrace.h
@@ -90,5 +90,6 @@ extern int linux_supports_tracefork (void);
extern int linux_supports_traceclone (void);
extern int linux_supports_tracevforkdone (void);
extern int linux_supports_tracesysgood (void);
+extern void linux_ptrace_set_additional_flags (int);

#endif /* COMMON_LINUX_PTRACE_H */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index bf6f586..289ae41 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4980,6 +4980,12 @@ Enables printf debugging output."),
sigdelset (&suspend_mask, SIGCHLD);

sigemptyset (&blocked_mask);
+
+ linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
+ | PTRACE_O_TRACEVFORKDONE
+ | PTRACE_O_TRACEVFORK
+ | PTRACE_O_TRACEFORK
+ | PTRACE_O_TRACEEXEC);
}
\f