From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25010 invoked by alias); 10 Feb 2015 16:34:53 -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 24994 invoked by uid 89); 10 Feb 2015 16:34:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 10 Feb 2015 16:34:50 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1AGYl9C017039 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 10 Feb 2015 11:34:47 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t1AGYjIn018175; Tue, 10 Feb 2015 11:34:46 -0500 Message-ID: <54DA3325.7090802@redhat.com> Date: Tue, 10 Feb 2015 16:34:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Don Breazeal , gdb-patches@sourceware.org Subject: Re: [PATCH v4 1/7] Identify remote fork event support References: <54C236B9.8060200@redhat.com> <1422222420-25421-1-git-send-email-donb@codesourcery.com> <1422222420-25421-2-git-send-email-donb@codesourcery.com> In-Reply-To: <1422222420-25421-2-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-02/txt/msg00260.txt.bz2 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 > > * 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) : > 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-25 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. > (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_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