This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 04/16 v3] Determine supported extended-remote features
- From: "Breazeal, Don" <donb at codesourcery dot com>
- To: <gdb-patches at sourceware dot org>, <palves at redhat dot com>
- Date: Wed, 21 Jan 2015 13:02:05 -0800
- Subject: Re: [PATCH 04/16 v3] Determine supported extended-remote features
- Authentication-results: sourceware.org; auth=none
- References: <5464FA50 dot 1000707 at codesourcery dot com> <1421102187-6125-1-git-send-email-donb at codesourcery dot com>
Ping.
The two patches from this series that are currently under review are:
https://sourceware.org/ml/gdb-patches/2015-01/msg00320.html
https://sourceware.org/ml/gdb-patches/2015-01/msg00321.html
The second of these depends on this:
https://sourceware.org/ml/gdb-patches/2014-10/msg00870.html
Also, I have made some progress on follow fork for target 'remote'.
I'll post that as a separate patch once I have it all working and
cleaned up.
thanks
--Don
On 1/12/2015 2:36 PM, Don Breazeal wrote:
>> As I mentioned, you'll need to make old GDB against new
>> GDBserver work correctly, that is, make gdbserver not send these new
>> events if debugging with old GDB, which is not expecting them.
>> That can be handled by making GDB send a "fork-events+" in its
>> own qSupported packet, so that GDBserver knows its talking to a
>> new GDB. See how the "multiprocess+" feature is handled in both
>> GDB and GDBserver. For a while, GDB didn't
>> broadcast "multiprocess+" when connecting with "target remote".
>
> Hi Pedro,
> This patch implements fork-events+ and vfork-events+ in the
> qSupported packet as you described.
>
>> One of the reasons I wanted that on "remote" was exactly to
>> make it possible to have fork events without caring for
>> remote vs extended-remote.
>
> I share this goal.
>
>>
>> If there are bigger problems, we can make GDB not broadcast
>> the support with plain remote for starters. But I'd be curious
>> to hear what the problems are.
>
> My previous response included two patches that "almost worked" for
> target remote. Please disregard those. As I went through the remaining
> issues, it became clear to me that there are enough underlying changes
> needed in the target remote multiprocess support that they deserve a
> separate patch. I would like to proceed with the extended-remote support
> and address target remote issue in a subsequent patch.
>
> The issues that I ran into with target remote and follow fork
> were mostly related to detach and kill in making decisions about
> when and where to mourn an inferior and keeping things in sync
> between GDB and gdbserver. The issues aren't insurmountable, but
> I'm proposing that try to make progress by taking things in smaller
> bites: first the extended-remote support, then the underlying target
> remote multiprocess additions, then target remote follow fork.
>
> Note that the remote vs. extended-remote issue is moot for this
> patch except in the construction of the qSupported packet.
>
> Let me know what you think.
> Thanks,
> --Don
>
> 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 for fork and vfork events. Other
> events like vfork-done may be added later in the patch series if
> needed.
>
> These packets are used just like PACKET_multiprocess_feature to denote
> whether the corresponding event is supported by inquiring about packet
> support in the qSupported packet, then enabling or disabling the packet
> based on the qSupported response. Target functions used to query for
> support are included for 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 events are reported. 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 #ifdef that have been added temporarily to allow the code for
> managing the events to be included in this patch without enabling the
> events. See PTRACE_EVENT_SUPPORT. This #ifdef will be removed later in
> the patch series as the events are enabled.
>
> Tested on Ubuntu x64, native/remote/extended-remote, and as part of
> subsequent patches in the series.
>
> gdb/gdbserver/
> 2015-01-12 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,
> report_exec_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-12 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.
> * 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.c (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 | 13 +++++++------
> gdb/nat/linux-ptrace.h | 1 +
> gdb/remote.c | 41 +++++++++++++++++++++++++++++++++++++++--
> 8 files changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 65f72a2..b1201b3 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -5129,6 +5129,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)
> {
> @@ -6040,6 +6056,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
> @@ -6115,4 +6133,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);
> + linux_test_for_event_reporting ();
> }
> diff --git a/gdb/gdbserver/lynx-low.c b/gdb/gdbserver/lynx-low.c
> index 3b83669..e3fb788 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 173a22e..b092020 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 5e29b7f..e25e5f8 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 *);
> @@ -390,6 +396,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 e714933..a35ff91 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 a0e0c32..542e762 100644
> --- a/gdb/nat/linux-ptrace.c
> +++ b/gdb/nat/linux-ptrace.c
> @@ -311,8 +311,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;
>
> @@ -433,9 +433,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));
>
> /* Do some cleanup and kill the grandchild. */
> my_waitpid (second_pid, &second_status, 0);
> @@ -477,7 +478,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)
> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index 588d38a..edbacfd 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -90,6 +90,7 @@ struct buffer;
>
> extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer);
> 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 4b9b099..891329a 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:
> /* Additions to this list need to be well justified:
> pre-existing packets are OK; new packets are not. */
> excepted = 1;
>