From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31906 invoked by alias); 21 Jan 2015 21:02:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31882 invoked by uid 89); 21 Jan 2015 21:02:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Jan 2015 21:02:18 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1YE2Pm-0007EK-ML from Don_Breazeal@mentor.com ; Wed, 21 Jan 2015 13:02:14 -0800 Received: from [172.30.9.85] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 21 Jan 2015 13:02:14 -0800 Message-ID: <54C013CD.7040708@codesourcery.com> Date: Wed, 21 Jan 2015 21:02:00 -0000 From: "Breazeal, Don" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: , Subject: Re: [PATCH 04/16 v3] Determine supported extended-remote features References: <5464FA50.1000707@codesourcery.com> <1421102187-6125-1-git-send-email-donb@codesourcery.com> In-Reply-To: <1421102187-6125-1-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-01/txt/msg00578.txt.bz2 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 > > * 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) : > New member. > : 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 > > * 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_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; >