Re: [PATCH v4 1/7] Identify remote fork event support

Pedro Alves
2015-02-10T16:34:53Z

Hi Don,

On 01/25/2015 09:46 PM, Don Breazeal wrote:
> This patch implements a mechanism for GDB to determine whether fork
> events are supported in gdbserver. This is a preparatory patch for
> remote fork and exec event support.
>
> Two new RSP packets are defined to represent fork and vfork event
> support. These packets are used just like PACKET_multiprocess_feature
> to denote whether the corresponding event is supported. GDB sends
> fork-events+ and vfork-events+ to gdbserver to inquire about fork
> event support. If the response enables these packets, then GDB
> knows that gdbserver supports the corresponding events and will
> enable them.
>
> Target functions used to query for support are included along with
> each new packet.
>
> In order for gdbserver to know whether the events are supported at the
> point where the qSupported packet arrives, the code in nat/linux-ptrace.c
> had to be reorganized. Previously it would test for fork/exec event
> support, then enable the events using the pid of the inferior. When the
> qSupported packet arrives there may not be an inferior. So the mechanism
> was split into two parts: a function that checks whether the events are
> supported, called when gdbserver starts up, and another that enables the
> events when the inferior stops for the first time.
>
> Another gdbserver change was to add some global variables similar to
> multi_process, one per new packet. These are used to control whether
> the corresponsing fork events are enabled. If GDB does not inquire
> about the event support in the qSupported packet, then gdbserver will
> not set these "report the event" flags. If the flags are not set, the
> events are ignored like they were in the past.
>
> There is an #if that has been added temporarily to allow the code for
> managing the events to be included in this patch without enabling the
> events. See PTRACE_FORK_EVENTS in gdb/remote.c. This #if will be
> removed later in the patch series as the events are enabled.

(Nit: I'd prefer that these code bits / hunks were simply moved
to the patches that make use of them.)

>
> Tested on Ubuntu x64, native/remote/extended-remote, and as part of
> subsequent patches in the series.
>
> Thanks,
> --Don
>
> gdb/gdbserver/
> 2015-01-25 Don Breazeal <donb@codesourcery.com>
>
> * linux-low.c (linux_supports_fork_events): New function.
> (linux_supports_vfork_events): New function.
> (linux_target_ops): Initialize new structure members.
> (initialize_low): Call linux_ptrace_set_additional_flags
> and linux_test_for_event_reporting.
> * lynx-low.c (lynx_target_ops): Initialize new structure
> members.
> * server.c (report_fork_events, report_vfork_events):
> New global flags.
> (handle_query): Add new features to qSupported packet.
> (captured_main): Initialize new global variables.
> * target.h (struct target_ops) <supports_fork_events>:
> New member.
> <supports_vfork_events>: New member.
> (target_supports_fork_events): New macro.
> (target_supports_vfork_events): New macro.
> * win32-low.c (win32_target_ops): Initialize new structure
> members.
>
> gdb/
> 2015-01-25 Don Breazeal <donb@codesourcery.com>
>
> * nat/linux-ptrace.c (linux_test_for_event_reporting): Rename
> from linux_check_ptrace_features and make it extern.
> (linux_test_for_tracefork): Reformat code.
> (linux_enable_event_reporting): Change name of called function
> to linux_check_ptrace_features.
> (ptrace_supports_feature): Call linux_test_for_event_reporting
> instead of linux_check_ptrace_features.
> * nat/linux-ptrace.h: Declare linux_test_for_event_reporting.
> * remote.c (anonymous enum) <PACKET_fork_event_feature,
> PACKET_vfork_event_feature>: New enumeration constants.
> (remote_fork_event_p): New function.
> (remote_vfork_event_p): New function.
> (remote_query_supported): Add new feature queries to qSupported
> packet.
> (remote_protocol_features): Add table entries for new packets.
> (_initialize_remote): Exempt new packets from the requirement
> to have 'set remote' commands.
>
> ---
> gdb/gdbserver/linux-low.c | 22 ++++++++++++++++++++++
> gdb/gdbserver/lynx-low.c | 2 ++
> gdb/gdbserver/server.c | 22 ++++++++++++++++++++++
> gdb/gdbserver/target.h | 14 ++++++++++++++
> gdb/gdbserver/win32-low.c | 2 ++
> gdb/nat/linux-ptrace.c | 25 +++++++++++++------------
> gdb/nat/linux-ptrace.h | 1 +
> gdb/remote.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 8 files changed, 115 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 5e37dd5..e31844c 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5123,6 +5123,22 @@ linux_supports_multi_process (void)
> return 1;
> }
>
> +/* Check if fork events are supported. */
> +
> +static int
> +linux_supports_fork_events (void)
> +{
> + return linux_supports_tracefork ();
> +}
> +
> +/* Check if vfork events are supported. */
> +
> +static int
> +linux_supports_vfork_events (void)
> +{
> + return linux_supports_tracefork ();
> +}
> +
> static int
> linux_supports_disable_randomization (void)
> {
> @@ -6034,6 +6050,8 @@ static struct target_ops linux_target_ops = {
> linux_async,
> linux_start_non_stop,
> linux_supports_multi_process,
> + linux_supports_fork_events,
> + linux_supports_vfork_events,
> #ifdef USE_THREAD_DB
> thread_db_handle_monitor_command,
> #else
> @@ -6108,4 +6126,8 @@ initialize_low (void)
> sigaction (SIGCHLD, &sigchld_action, NULL);
>
> initialize_low_arch ();
> +
> + /* Enable any extended ptrace events that are supported. */
> + linux_ptrace_set_additional_flags (0);

Likewise, seems pointless add this call now.

> + linux_test_for_event_reporting ();
> }
> diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
> index 98797ba..1126103 100644
> --- a/gdb/gdbserver/lynx-low.c
> +++ b/gdb/gdbserver/lynx-low.c
> @@ -754,6 +754,8 @@ static struct target_ops lynx_target_ops = {
> NULL, /* async */
> NULL, /* start_non_stop */
> NULL, /* supports_multi_process */
> + NULL, /* supports_fork_events */
> + NULL, /* supports_vfork_events */
> NULL, /* handle_monitor_command */
> };
>
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 0e72cf1..48cc363 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -57,6 +57,8 @@ static int exit_requested;
> int run_once;
>
> int multi_process;
> +int report_fork_events;
> +int report_vfork_events;
> int non_stop;
>
> /* Whether we should attempt to disable the operating system's address
> @@ -1841,6 +1843,18 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> /* GDB supports relocate instruction requests. */
> gdb_supports_qRelocInsn = 1;
> }
> + if (strcmp (p, "fork-events+") == 0)
> + {
> + /* GDB supports and wants fork events if possible. */
> + if (target_supports_fork_events ())
> + report_fork_events = 1;
> + }
> + if (strcmp (p, "vfork-events+") == 0)
> + {
> + /* GDB supports and wants vfork events if possible. */
> + if (target_supports_vfork_events ())
> + report_vfork_events = 1;
> + }
> else
> target_process_qsupported (p);
>
> @@ -1891,6 +1905,12 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
> if (target_supports_multi_process ())
> strcat (own_buf, ";multiprocess+");
>
> + if (target_supports_fork_events ())
> + strcat (own_buf, ";fork-events+");
> +
> + if (target_supports_vfork_events ())
> + strcat (own_buf, ";vfork-events+");
> +
> if (target_supports_non_stop ())
> strcat (own_buf, ";QNonStop+");
>
> @@ -3242,6 +3262,8 @@ captured_main (int argc, char *argv[])
>
> noack_mode = 0;
> multi_process = 0;
> + report_fork_events = 0;
> + report_vfork_events = 0;
> /* Be sure we're out of tfind mode. */
> current_traceframe = -1;
> cont_thread = null_ptid;
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index bbb0567..8af76d9 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -262,6 +262,12 @@ struct target_ops
> /* Returns true if the target supports multi-process debugging. */
> int (*supports_multi_process) (void);
>
> + /* Returns true if fork events are supported. */
> + int (*supports_fork_events) (void);
> +
> + /* Returns true if vfork events are supported. */
> + int (*supports_vfork_events) (void);
> +
> /* If not NULL, target-specific routine to process monitor command.
> Returns 1 if handled, or 0 to perform default processing. */
> int (*handle_monitor_command) (char *);
> @@ -387,6 +393,14 @@ void set_target_ops (struct target_ops *);
>
> int kill_inferior (int);
>
> +#define target_supports_fork_events() \
> + (the_target->supports_fork_events ? \
> + (*the_target->supports_fork_events) () : 0)
> +
> +#define target_supports_vfork_events() \
> + (the_target->supports_vfork_events ? \
> + (*the_target->supports_vfork_events) () : 0)
> +
> #define detach_inferior(pid) \
> (*the_target->detach) (pid)
>
> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
> index e3fb618..210a747 100644
> --- a/gdb/gdbserver/win32-low.c
> +++ b/gdb/gdbserver/win32-low.c
> @@ -1823,6 +1823,8 @@ static struct target_ops win32_target_ops = {
> NULL, /* async */
> NULL, /* start_non_stop */
> NULL, /* supports_multi_process */
> + NULL, /* supports_fork_events */
> + NULL, /* supports_vfork_events */
> NULL, /* handle_monitor_command */
> NULL, /* core_of_thread */
> NULL, /* read_loadmap */
> diff --git a/gdb/nat/linux-ptrace.c b/gdb/nat/linux-ptrace.c
> index 0ce258f..3e1fcd5 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -299,7 +299,7 @@ linux_fork_to_function (gdb_byte *child_stack, void (*function) (gdb_byte *))
> return child_pid;
> }
>
> -/* A helper function for linux_check_ptrace_features, called after
> +/* A helper function for linux_test_for_event_reporting, called after
> the child forks a grandchild. */
>
> static void
> @@ -313,7 +313,7 @@ linux_grandchild_function (gdb_byte *child_stack)
> _exit (0);
> }
>
> -/* A helper function for linux_check_ptrace_features, called after
> +/* A helper function for linux_test_for_event_reporting, called after
> the parent process forks a child. The child allows itself to
> be traced by its parent. */
>
> @@ -337,8 +337,8 @@ static void linux_test_for_exitkill (int child_pid);
>
> /* Determine ptrace features available on this target. */
>
> -static void
> -linux_check_ptrace_features (void)
> +void
> +linux_test_for_event_reporting (void)
> {
> int child_pid, ret, status;
>
> @@ -355,10 +355,10 @@ linux_check_ptrace_features (void)
> if (ret == -1)
> perror_with_name (("waitpid"));
> else if (ret != child_pid)
> - error (_("linux_check_ptrace_features: waitpid: unexpected result %d."),
> + error (_("linux_test_for_event_reporting: waitpid: unexpected result %d."),
> ret);
> if (! WIFSTOPPED (status))
> - error (_("linux_check_ptrace_features: waitpid: unexpected status %d."),
> + error (_("linux_test_for_event_reporting: waitpid: unexpected status %d."),
> status);
>
> linux_test_for_tracesysgood (child_pid);
> @@ -373,7 +373,7 @@ linux_check_ptrace_features (void)
> ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0,
> (PTRACE_TYPE_ARG4) 0);
> if (ret != 0)
> - warning (_("linux_check_ptrace_features: failed to kill child"));
> + warning (_("linux_test_for_event_reporting: failed to kill child"));
> my_waitpid (child_pid, &status, 0);
> }
> while (WIFSTOPPED (status));
> @@ -459,9 +459,10 @@ linux_test_for_tracefork (int child_pid)
> /* 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));
> + current_ptrace_options |= (additional_flags
> + & (PTRACE_O_TRACEFORK
> + | PTRACE_O_TRACEVFORK
> + | PTRACE_O_TRACEEXEC));

This looks like a spurious change?

>
> /* Do some cleanup and kill the grandchild. */
> my_waitpid (second_pid, &second_status, 0);
> @@ -503,7 +504,7 @@ linux_enable_event_reporting (pid_t pid, int attached)
> /* 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 ();
> + linux_test_for_event_reporting ();
>
> ptrace_options = current_ptrace_options;
> if (attached)
> @@ -535,7 +536,7 @@ static int
> ptrace_supports_feature (int ptrace_options)
> {
> if (current_ptrace_options == -1)
> - linux_check_ptrace_features ();
> + linux_test_for_event_reporting ();

I don't really understand the motivation for the renaming.
The function still tests all kinds of ptrace features, not just
event-related features. E.g., it tests PTRACE_O_EXITKILL. And the
variable it controls is called "ptrace_options". How about
leaving the function's name as is?

>
> return ((current_ptrace_options & ptrace_options) == ptrace_options);
> }
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 137b61a..d75b37c 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -98,6 +98,7 @@ extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer);
> extern char *linux_ptrace_attach_fail_reason_string (ptid_t ptid, int err);
>
> extern void linux_ptrace_init_warnings (void);
> +extern void linux_test_for_event_reporting (void);
> extern void linux_enable_event_reporting (pid_t pid, int attached);
> extern void linux_disable_event_reporting (pid_t pid);
> extern int linux_supports_tracefork (void);
> diff --git a/gdb/remote.c b/gdb/remote.c
> index c4d09ba..ec1f1d2 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1327,6 +1327,12 @@ enum {
> /* Support for qXfer:libraries-svr4:read with a non-empty annex. */
> PACKET_augmented_libraries_svr4_read_feature,
>
> + /* Support for fork events. */
> + PACKET_fork_event_feature,
> +
> + /* Support for vfork events. */
> + PACKET_vfork_event_feature,
> +
> PACKET_MAX
> };
>
> @@ -1432,6 +1438,24 @@ remote_multi_process_p (struct remote_state *rs)
> return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE;
> }
>
> +#if PTRACE_FORK_EVENTS
> +/* Returns true if fork events are supported. */
> +
> +static int
> +remote_fork_event_p (struct remote_state *rs)
> +{
> + return packet_support (PACKET_fork_event_feature) == PACKET_ENABLE;
> +}
> +
> +/* Returns true if vfork events are supported. */
> +
> +static int
> +remote_vfork_event_p (struct remote_state *rs)
> +{
> + return packet_support (PACKET_vfork_event_feature) == PACKET_ENABLE;
> +}
> +#endif
> +
> /* Tokens for use by the asynchronous signal handlers for SIGINT. */
> static struct async_signal_handler *async_sigint_remote_twice_token;
> static struct async_signal_handler *async_sigint_remote_token;
> @@ -4010,7 +4034,11 @@ static const struct protocol_feature remote_protocol_features[] = {
> { "Qbtrace:off", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace_off },
> { "Qbtrace:bts", PACKET_DISABLE, remote_supported_packet, PACKET_Qbtrace_bts },
> { "qXfer:btrace:read", PACKET_DISABLE, remote_supported_packet,
> - PACKET_qXfer_btrace }
> + PACKET_qXfer_btrace },
> + { "fork-events", PACKET_DISABLE, remote_supported_packet,
> + PACKET_fork_event_feature },
> + { "vfork-events", PACKET_DISABLE, remote_supported_packet,
> + PACKET_vfork_event_feature },
> };
>
> static char *remote_support_xml;
> @@ -4084,6 +4112,12 @@ remote_query_supported (void)
>
> q = remote_query_supported_append (q, "qRelocInsn+");
>
> + if (rs->extended)
> + {
> + q = remote_query_supported_append (q, "fork-events+");
> + q = remote_query_supported_append (q, "vfork-events+");
> + }
> +
> q = reconcat (q, "qSupported:", q, (char *) NULL);
> putpkt (q);
>
> @@ -12191,7 +12225,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
> add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace],
> "qXfer:btrace", "read-btrace", 0);
>
> - /* Assert that we've registered commands for all packet configs. */
> + /* Assert that we've registered "set remote foo-packet" commands
> + for all packet configs. */
> {
> int i;
>
> @@ -12210,6 +12245,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
> case PACKET_DisconnectedTracing_feature:
> case PACKET_augmented_libraries_svr4_read_feature:
> case PACKET_qCRC:
> + case PACKET_fork_event_feature:
> + case PACKET_vfork_event_feature:

So what's the justification for not adding the commands? :-)

> /* Additions to this list need to be well justified:
> pre-existing packets are OK; new packets are not. */
> excepted = 1;

The patch should also include a GDB manual change describing the
new RSP features, and a NEWS change mentioning them.

Thanks,
Pedro Alves