Re: [PATCH 04/10] Enhance extended ptrace event setup

Breazeal, Don
2014-08-13T17:50:18Z

Hi
It turns out that this patch doesn't work well for remote exec
catchpoints. In the event anybody is reviewing this patch series,
please don't spend any time on patch 4 until I post an updated version.
The rest of the patch series will remain relatively unchanged as a
result of this.

Sorry about that.
--Don

On 8/7/2014 10:59 AM, Don Breazeal wrote:
> This patch revises nat/linux-ptrace.c:linux_test_for_tracefork and linux_test_for_syscallgood. The change implements a mechanism for modifying the ptrace options after initialization is complete.
>
> This is a variation on the patch that Gary Benson implemented to reduce the use of #ifdef GDBSERVER in linux-ptrace.c here:
>
> https://sourceware.org/ml/gdb-patches/2014-07/msg00633.html.
>
> This is needed because gdbserver doesn't always know whether it should enable extended-mode features until GDB connects. There is a new function that allows the caller to specify both "base" options and "additional" options, where base options are set at initialization, and additional options can be added later at the caller's discretion using another new function. The additional options can be used to enable options only available in extended-mode.
>
> Tested on x64 Ubuntu Lucid, native only. The 'additional option' mechanism was tested as part of the patch 6, the follow-fork patch.
>
> Thanks,
> --Don
>
> gdb/
> 2014-08-06 Don Breazeal <donb@codesourcery.com>
> * linux-nat.c (_initialize_linux_nat): Replace call to
> linux_ptrace_set_additional_flags with call to
> linux_ptrace_set_desired_options.
> * nat/linux-ptrace.c (additional_flags): Delete static variable.
> (base_ptrace_options): New static variable.
> (additional_ptrace_options): New static variable.
> (linux_test_for_tracesysgood, linux_test_for_tracefork): Use
> base_ptrace_options and additional_ptrace_options.
> (linux_ptrace_set_additional_options): Deleted function.
> (linux_ptrace_set_desired_options): New function.
> (linux_enable_additional_options): New function.
> * nat/linux-ptrace.h (linux_ptrace_set_additional_flags):
> Delete function declaration.
> (linux_ptrace_set_desired_options): Declare new function.
> (linux_enable_additional_options): Declare new function.
>
> gdb/gdbserver/
> 2014-08-06 Don Breazeal <donb@codesourcery.com>
> * linux-low.c (linux_enable_extended_features): New function.
> (initialize_low): Call linux_ptrace_set_desired_options.
> (linux_target_ops): Initialize new member.
> * server.c (process_serial_event): Call new target routine
> enable_extended_features.
> * target.h (struct target_ops) <enable_extended_features>: New
> member.
> * lynx-low.c (lynx_target_ops): Initialize new member.
> * nto-low.c (nto_target_ops): Ditto.
> * spu-low.c (spu_target_ops): Ditto.
> * win32-low.c (win32_target_ops): Ditto.
>
> ---
> gdb/gdbserver/linux-low.c | 27 ++++++++++++++
> gdb/gdbserver/lynx-low.c | 1 +
> gdb/gdbserver/nto-low.c | 1 +
> gdb/gdbserver/server.c | 4 ++
> gdb/gdbserver/spu-low.c | 1 +
> gdb/gdbserver/target.h | 5 +++
> gdb/gdbserver/win32-low.c | 1 +
> gdb/linux-nat.c | 13 +++----
> gdb/nat/linux-ptrace.c | 86 ++++++++++++++++++++++++++++++--------------
> gdb/nat/linux-ptrace.h | 4 ++-
> 10 files changed, 107 insertions(+), 36 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index db22e92..2a346d9 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -1177,6 +1177,30 @@ linux_detach (int pid)
> return 0;
> }
>
> +/* Enable extended mode features by enabling extended ptrace events. */
> +
> +static void
> +linux_enable_extended_features (void)
> +{
> + struct thread_info *tp = (struct thread_info *) current_inferior;
> +
> + if (tp == NULL)
> + tp = get_first_thread ();
> +
> + if (tp == NULL)
> + {
> + /* There is no process yet, so include extended options in the
> + base options for subsequent ptrace configuration. */
> + linux_ptrace_set_desired_options (PTRACE_O_TRACECLONE, 0);
> + }
> + else
> + {
> + /* Re-initialize ptrace options with extended options. */
> + pid_t pid = ptid_get_lwp (tp->entry.id);
> + linux_enable_additional_options (pid);
> + }
> +}
> +
> /* Remove all LWPs that belong to process PROC from the lwp list. */
>
> static int
> @@ -6018,6 +6042,7 @@ static struct target_ops linux_target_ops = {
> linux_attach,
> linux_kill,
> linux_detach,
> + linux_enable_extended_features,
> linux_mourn,
> linux_join,
> linux_thread_alive,
> @@ -6132,4 +6157,6 @@ initialize_low (void)
> sigaction (SIGCHLD, &sigchld_action, NULL);
>
> initialize_low_arch ();
> +
> + linux_ptrace_set_desired_options (PTRACE_O_TRACECLONE, 0);
> }
> diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
> index 0b0ff47..750503d 100644
> --- a/gdb/gdbserver/lynx-low.c
> +++ b/gdb/gdbserver/lynx-low.c
> @@ -722,6 +722,7 @@ static struct target_ops lynx_target_ops = {
> lynx_attach,
> lynx_kill,
> lynx_detach,
> + NULL, /* enable_extended_features */
> lynx_mourn,
> lynx_join,
> lynx_thread_alive,
> diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
> index 3587156..729328f 100644
> --- a/gdb/gdbserver/nto-low.c
> +++ b/gdb/gdbserver/nto-low.c
> @@ -929,6 +929,7 @@ static struct target_ops nto_target_ops = {
> nto_attach,
> nto_kill,
> nto_detach,
> + NULL, /* enable_extended_features */
> nto_mourn,
> NULL, /* nto_join */
> nto_thread_alive,
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index cf1dffe..e42e596 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -3542,6 +3542,10 @@ process_serial_event (void)
> break;
> case '!':
> extended_protocol = 1;
> +
> + if (the_target->enable_extended_features != NULL)
> + (*the_target->enable_extended_features) ();
> +
> write_ok (own_buf);
> break;
> case '?':
> diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
> index 9bb0c40..cfd33b0 100644
> --- a/gdb/gdbserver/spu-low.c
> +++ b/gdb/gdbserver/spu-low.c
> @@ -645,6 +645,7 @@ static struct target_ops spu_target_ops = {
> spu_attach,
> spu_kill,
> spu_detach,
> + NULL, /* enable_extended_features */
> spu_mourn,
> spu_join,
> spu_thread_alive,
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index f5eda8a..e09e885 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -92,11 +92,16 @@ struct target_ops
>
> int (*detach) (int pid);
>
> + /* Enable target specific features for extended mode. */
> +
> + void (*enable_extended_features) (void);
> +
> /* The inferior process has died. Do what is right. */
>
> void (*mourn) (struct process_info *proc);
>
> /* Wait for inferior PID to exit. */
> +
> void (*join) (int pid);
>
> /* Return 1 iff the thread with process ID PID is alive. */
> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
> index bc2506c..d09cec3 100644
> --- a/gdb/gdbserver/win32-low.c
> +++ b/gdb/gdbserver/win32-low.c
> @@ -1768,6 +1768,7 @@ static struct target_ops win32_target_ops = {
> win32_attach,
> win32_kill,
> win32_detach,
> + NULL, /* enable_extended_features */
> win32_mourn,
> win32_join,
> win32_thread_alive,
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index b72c066..bda535c 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -4849,13 +4849,12 @@ Enables printf debugging output."),
>
> sigemptyset (&blocked_mask);
>
> - /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to
> - support read-only process state. */
> - linux_ptrace_set_additional_flags (PTRACE_O_TRACESYSGOOD
> - | PTRACE_O_TRACEVFORKDONE
> - | PTRACE_O_TRACEVFORK
> - | PTRACE_O_TRACEFORK
> - | PTRACE_O_TRACEEXEC);
> + linux_ptrace_set_desired_options (PTRACE_O_TRACECLONE
> + | PTRACE_O_TRACESYSGOOD
> + | PTRACE_O_TRACEVFORKDONE
> + | PTRACE_O_TRACEVFORK
> + | PTRACE_O_TRACEFORK
> + | PTRACE_O_TRACEEXEC, 0);
> }
> \f
>
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 69eb83c..a9b80a0 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -37,9 +37,14 @@
> there are no supported features. */
> static int current_ptrace_options = -1;
>
> -/* Additional flags to test. */
> +/* Stores the ptrace options that the debugger wants enabled if
> + the operating system supports them. */
> +static int base_ptrace_options;
>
> -static int additional_flags;
> +/* Stores ptrace options that the ptrace caller may wish to enable
> + after initial ptrace option checking is complete, e.g. for
> + gdbserver extended-remote targets. */
> +static int additional_ptrace_options;
>
> /* Find all possible reasons we could fail to attach PID and append
> these as strings to the already initialized BUFFER. '\0'
> @@ -357,22 +362,27 @@ linux_check_ptrace_features (void)
> while (WIFSTOPPED (status));
> }
>
> -/* Determine if PTRACE_O_TRACESYSGOOD can be used to catch
> - syscalls. */
> +/* Determine if PTRACE_O_TRACESYSGOOD can be used to catch syscalls.
> + Enable if it is in the base options, otherwise save it. */
>
> static void
> linux_test_for_tracesysgood (int child_pid)
> {
> int ret;
>
> - if ((additional_flags & PTRACE_O_TRACESYSGOOD) == 0)
> + /* Check if the caller want PTRACE_O_TRACESYSGOOD under any
> + circumstances. */
> + if (((base_ptrace_options | additional_ptrace_options)
> + & PTRACE_O_TRACESYSGOOD) == 0)
> return;
>
> ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0,
> - (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
> + (PTRACE_TYPE_ARG4) PTRACE_O_TRACESYSGOOD);
>
> if (ret == 0)
> - current_ptrace_options |= PTRACE_O_TRACESYSGOOD;
> + current_ptrace_options |= (PTRACE_O_TRACESYSGOOD & base_ptrace_options);
> + else
> + additional_ptrace_options &= ~PTRACE_O_TRACESYSGOOD;
> }
>
> /* Determine if PTRACE_O_TRACEFORK can be used to follow fork
> @@ -392,15 +402,14 @@ linux_test_for_tracefork (int child_pid)
> if (ret != 0)
> return;
>
> - 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)
> - current_ptrace_options |= PTRACE_O_TRACEVFORKDONE;
> - }
> + /* 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 & base_ptrace_options);
> + else
> + additional_ptrace_options &= ~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
> @@ -432,14 +441,19 @@ linux_test_for_tracefork (int child_pid)
> (PTRACE_TYPE_ARG4) &second_pid);
> if (ret == 0 && second_pid != 0)
> {
> + /* We got the PID from the grandchild, which means fork
> + tracing and related options are supported by the OS. */
> + int supported_options = (PTRACE_O_TRACEFORK
> + | PTRACE_O_TRACEVFORK
> + | PTRACE_O_TRACECLONE
> + | PTRACE_O_TRACEEXEC);
> int second_status;
>
> - /* We got the PID from the grandchild, which means fork
> - tracing is supported. */
> - current_ptrace_options |= PTRACE_O_TRACECLONE;
> - current_ptrace_options |= (additional_flags & (PTRACE_O_TRACEFORK
> - | PTRACE_O_TRACEVFORK
> - | PTRACE_O_TRACEEXEC));
> + if (ret == 0)
> + current_ptrace_options |= (supported_options
> + & base_ptrace_options);
> + else
> + additional_ptrace_options &= ~supported_options;
>
> /* Do some cleanup and kill the grandchild. */
> my_waitpid (second_pid, &second_status, 0);
> @@ -547,15 +561,31 @@ 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. */
> +/* Set masks to be used for initializing ptrace options. Options in BASE
> + will always be enabled if the OS supports them. Options in ADDITIONAL
> + will only be enabled if linux_ptrace_enable_additional_options is
> + called. This function must be called before testing for ptrace
> + option support; otherwise the options may not be enabled. */
>
> void
> -linux_ptrace_set_additional_flags (int flags)
> +linux_ptrace_set_desired_options (int base, int additional)
> {
> - additional_flags = flags;
> + base_ptrace_options = base;
> + additional_ptrace_options = additional;
> +}
> +
> +/* Enable the additional ptrace options requested by a previous call to
> + linux_ptrace_set_desired_options. */
> +
> +void
> +linux_enable_additional_options (pid_t pid)
> +{
> + if (additional_ptrace_options != 0)
> + {
> + current_ptrace_options |= additional_ptrace_options;
> + ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0,
> + (PTRACE_TYPE_ARG4) (uintptr_t) current_ptrace_options);
> + }
> }
>
> /* Extract extended ptrace event from wait status. */
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 31a77cd..140a86b 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -91,7 +91,9 @@ 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);
> +extern void linux_ptrace_set_desired_options (int base_options,
> + int additional_options);
> +extern void linux_enable_additional_options (pid_t pid);
> extern int linux_ptrace_get_extended_event (int wstat);
> extern int linux_is_extended_waitstatus (int wstat);
>
>