[02/15] Remove some GDBSERVER checks from linux-ptrace
Commit Message
This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
Currently the code uses a compile-time check to decide whether some
flags should be used. This changes the code to instead let users of
the module specify an additional set of flags; and then changes gdb's
linux-nat.c to call this function. At some later date, when the back
ends are fully merged, we will be able to remove this function again.
gdb/
2014-07-09 Tom Tromey <tromey@redhat.com>
Gary Benson <gbenson@redhat.com>
* nat/linux-ptrace.c (additional_flags): New global.
(linux_test_for_tracesysgood, linux_test_for_tracefork): Use
additional_flags; don't check GDBSERVER.
(linux_ptrace_set_additional_flags): New function.
* nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
Declare.
* linux-nat.c (_initialize_linux_nat): Call
linux_ptrace_set_additional_flags.
---
gdb/ChangeLog | 12 +++++++++
gdb/linux-nat.c | 6 ++++
gdb/nat/linux-ptrace.c | 64 ++++++++++++++++++++++++-----------------------
gdb/nat/linux-ptrace.h | 1 +
4 files changed, 52 insertions(+), 31 deletions(-)
Comments
Hi,
On 7/9/2014 3:37 AM, Gary Benson wrote:
> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> Currently the code uses a compile-time check to decide whether some
> flags should be used. This changes the code to instead let users of
> the module specify an additional set of flags; and then changes gdb's
> linux-nat.c to call this function. At some later date, when the back
> ends are fully merged, we will be able to remove this function again.
I just want to point out that I'm doing something similar as part of my
work on follow-fork for extended remote, based on Tom's suggestion here:
https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html
In my case it adds "base" options and the capability to set the ptrace
options to the "additional" options at the caller's discretion, such as
when gdbserver receives the enable-extended-mode packet. So, no
objection, just wanted to let you know that I hope to submit somewhat
different changes to the same code soon.
--Don
>
> gdb/
> 2014-07-09 Tom Tromey <tromey@redhat.com>
> Gary Benson <gbenson@redhat.com>
>
> * nat/linux-ptrace.c (additional_flags): New global.
> (linux_test_for_tracesysgood, linux_test_for_tracefork): Use
> additional_flags; don't check GDBSERVER.
> (linux_ptrace_set_additional_flags): New function.
> * nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
> Declare.
> * linux-nat.c (_initialize_linux_nat): Call
> linux_ptrace_set_additional_flags.
> ---
> gdb/ChangeLog | 12 +++++++++
> gdb/linux-nat.c | 6 ++++
> gdb/nat/linux-ptrace.c | 64 ++++++++++++++++++++++++-----------------------
> gdb/nat/linux-ptrace.h | 1 +
> 4 files changed, 52 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 0ab0362..5c7a21a 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -5025,6 +5025,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);
> }
>
>
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 3ad2113..59c9bfa 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/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 as strings to the already 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);
> @@ -551,3 +542,14 @@ linux_ptrace_init_warnings (void)
>
> linux_ptrace_test_ret_to_nx ();
> }
> +
> +/* Set additional ptrace flags to use. Some such flags may be checked
> + by the implementation above. This function must be called before
> + any other function in this file; otherwise the flags may not take
> + effect appropriately. */
> +
> +void
> +linux_ptrace_set_additional_flags (int flags)
> +{
> + additional_flags = flags;
> +}
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index cffb5ce..41b3198 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -91,5 +91,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 */
>
Breazeal, Don wrote:
> On 7/9/2014 3:37 AM, Gary Benson wrote:
> > This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> > Currently the code uses a compile-time check to decide whether
> > some flags should be used. This changes the code to instead let
> > users of the module specify an additional set of flags; and then
> > changes gdb's linux-nat.c to call this function. At some later
> > date, when the back ends are fully merged, we will be able to
> > remove this function again.
>
> I just want to point out that I'm doing something similar as part
> of my work on follow-fork for extended remote, based on Tom's
> suggestion here:
>
> https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html
>
> In my case it adds "base" options and the capability to set the
> ptrace options to the "additional" options at the caller's
> discretion, such as when gdbserver receives the enable-extended-mode
> packet. So, no objection, just wanted to let you know that I hope
> to submit somewhat different changes to the same code soon.
The patch I posted is basically an updated version of the patch Tom
posted there--the only change IIRC is that linux-ptrace.c is now in
"nat" rather than "common".
Do you need me to change anything about this parch, or is it ok for
you to work with?
Thanks,
Gary
On 7/9/2014 11:20 AM, Gary Benson wrote:
> Breazeal, Don wrote:
>> On 7/9/2014 3:37 AM, Gary Benson wrote:
>>> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
>>> Currently the code uses a compile-time check to decide whether
>>> some flags should be used. This changes the code to instead let
>>> users of the module specify an additional set of flags; and then
>>> changes gdb's linux-nat.c to call this function. At some later
>>> date, when the back ends are fully merged, we will be able to
>>> remove this function again.
>>
>> I just want to point out that I'm doing something similar as part
>> of my work on follow-fork for extended remote, based on Tom's
>> suggestion here:
>>
>> https://sourceware.org/ml/gdb-patches/2014-05/msg00486.html
>>
>> In my case it adds "base" options and the capability to set the
>> ptrace options to the "additional" options at the caller's
>> discretion, such as when gdbserver receives the enable-extended-mode
>> packet. So, no objection, just wanted to let you know that I hope
>> to submit somewhat different changes to the same code soon.
>
> The patch I posted is basically an updated version of the patch Tom
> posted there--the only change IIRC is that linux-ptrace.c is now in
> "nat" rather than "common".
>
> Do you need me to change anything about this parch, or is it ok for
> you to work with?
It is OK for me.
thanks
--Don
>
> Thanks,
> Gary
>
Gary Benson writes:
> This patch removes some GDBSERVER checks from nat/linux-ptrace.c.
> Currently the code uses a compile-time check to decide whether some
> flags should be used. This changes the code to instead let users of
> the module specify an additional set of flags; and then changes gdb's
> linux-nat.c to call this function. At some later date, when the back
> ends are fully merged, we will be able to remove this function again.
>
> gdb/
> 2014-07-09 Tom Tromey <tromey@redhat.com>
> Gary Benson <gbenson@redhat.com>
>
> * nat/linux-ptrace.c (additional_flags): New global.
> (linux_test_for_tracesysgood, linux_test_for_tracefork): Use
> additional_flags; don't check GDBSERVER.
> (linux_ptrace_set_additional_flags): New function.
> * nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
> Declare.
> * linux-nat.c (_initialize_linux_nat): Call
> linux_ptrace_set_additional_flags.
Hi. Recognizing that this is a temporary hack, the patch is ok with me.
One nit below.
> @@ -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);
I can't tell if the PTRACE_O_TRACEEXIT comment was accidentally
or intentionally dropped.
I'm not sure it's important enough to keep, but it would be
good to verify its deletion was intentional.
Doug Evans wrote:
> Gary Benson writes:
> > @@ -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);
>
> I can't tell if the PTRACE_O_TRACEEXIT comment was accidentally or
> intentionally dropped. I'm not sure it's important enough to keep,
> but it would be good to verify its deletion was intentional.
I don't know, so unless Tom has any objections I will reinstate
the comment before the call to linux_ptrace_set_additional_flags
in linux-nat.c.
Thanks,
Gary
@@ -5025,6 +5025,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);
}
@@ -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 as strings to the already 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);
@@ -551,3 +542,14 @@ linux_ptrace_init_warnings (void)
linux_ptrace_test_ret_to_nx ();
}
+
+/* Set additional ptrace flags to use. Some such flags may be checked
+ by the implementation above. This function must be called before
+ any other function in this file; otherwise the flags may not take
+ effect appropriately. */
+
+void
+linux_ptrace_set_additional_flags (int flags)
+{
+ additional_flags = flags;
+}
@@ -91,5 +91,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 */