From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36468 invoked by alias); 20 Nov 2015 13:04:49 -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 36443 invoked by uid 89); 20 Nov 2015 13:04:47 -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,RP_MATCHES_RCVD,SPF_HELO_PASS 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; Fri, 20 Nov 2015 13:04:45 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id A9466C0A15F2; Fri, 20 Nov 2015 13:04:44 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAKD4gJo018619; Fri, 20 Nov 2015 08:04:43 -0500 Message-ID: <564F1A6A.3030301@redhat.com> Date: Fri, 20 Nov 2015 13:04:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Don Breazeal , gdb-patches@sourceware.org Subject: Re: [PATCH 1/3] Target remote mode fork and exec events References: <1446854188-496-1-git-send-email-donb@codesourcery.com> <1446854188-496-2-git-send-email-donb@codesourcery.com> In-Reply-To: <1446854188-496-2-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-11/txt/msg00422.txt.bz2 Hi Don, Thanks for doing this. Starting to look at the series. On 11/06/2015 11:56 PM, Don Breazeal wrote: > This patch implements support for fork and exec events with target remote > mode Linux targets. For such targets with Linux kernels 2.5.46 and later, > this enables follow-fork-mode, detach-on-fork and fork and exec > catchpoints. > Note that follow-exec-mode is not supported, because target > remote mode does not support the 'run' command. Not sure I don't understand this part/comment. > > The changes required to implement this included: > > * Don't exit from gdbserver if there are still active inferiors. > > * Allow changing the active process in remote mode. > > * Enable fork and exec events in remote mode. > > * Print "Ending remote debugging" when detaching only if in remote > mode and there is only one inferior left. > > (As I write this up I'm concluding that this is incorrect. We > don't care how many active inferiors there are, yes? Not sure I understand the question. But I'd say what matters is whether we're disconnecting/closing the connection. > Perhaps we > need a test that detaches when there are multiple inferiors. I've > added that to my list.) > > * Combine remote_kill and extended_remote_kill into a single function > that can handle the multiple inferior case for target remote. Also, > the same thing for remote_mourn and extended_remote_mourn. > > * Enable process-style ptids in target remote. > > * Remove restriction on multiprocess mode in target remote. > > Thanks > --Don > > gdb/gdbserver/ > 2015-11-06 Don Breazeal > > * server.c (process_serial_event): Don't exit from gdbserver > in remote mode if there are still active inferiors. > > gdb/ > 2015-11-06 Don Breazeal > > * remote.c (set_general_process): Remove restriction on target > remote mode. > (remote_query_supported): Likewise. > (remote_detach_1): Change restriction to target remote mode to > restriction to last inferior left. > (remote_disconnect): Unpush the target directly instead of > calling remote_mourn. > (remote_kill, extended_remote_kill): Combine functions into one, > remote_kill, and enable extended functionality for target remote. > (remote_mourn, extended_remote_mourn): Combine functions into > one, remote_mourn, and enable extended functionality for target > remote. > (remote_pid_to_str): Enable "process" style ptid string for > target remote. > (remote_supports_multi_process): Remove restriction on target > remote mode. > > --- > gdb/gdbserver/server.c | 6 +- > gdb/remote.c | 166 ++++++++++++++++++++++++------------------------- > 2 files changed, 84 insertions(+), 88 deletions(-) > > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index fd2804b..0f57237 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -3865,9 +3865,11 @@ process_serial_event (void) > discard_queued_stop_replies (pid_to_ptid (pid)); > write_ok (own_buf); > > - if (extended_protocol) > + if (extended_protocol || get_first_inferior (&all_threads) != NULL) > { > - /* Treat this like a normal program exit. */ > + /* There is still at least one inferior remaining, so > + don't terminate gdbserver and treat this like a > + normal program exit. */ > last_status.kind = TARGET_WAITKIND_EXITED; > last_status.value.integer = 0; > last_ptid = pid_to_ptid (pid); > diff --git a/gdb/remote.c b/gdb/remote.c > index fed397a..60da26c 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -123,8 +123,6 @@ static void remote_mourn (struct target_ops *ops); > > static void extended_remote_restart (void); > > -static void extended_remote_mourn (struct target_ops *); > - > static void remote_send (char **buf, long *sizeof_buf_p); > > static int readchar (int timeout); > @@ -2084,7 +2082,7 @@ set_general_process (void) > struct remote_state *rs = get_remote_state (); > > /* If the remote can't handle multiple processes, don't bother. */ > - if (!rs->extended || !remote_multi_process_p (rs)) > + if (!remote_multi_process_p (rs)) > return; > > /* We only need to change the remote current thread if it's pointing > @@ -4472,18 +4470,15 @@ remote_query_supported (void) > > q = remote_query_supported_append (q, "qRelocInsn+"); > > - if (rs->extended) > - { > - if (packet_set_cmd_state (PACKET_fork_event_feature) > - != AUTO_BOOLEAN_FALSE) > - q = remote_query_supported_append (q, "fork-events+"); > - if (packet_set_cmd_state (PACKET_vfork_event_feature) > - != AUTO_BOOLEAN_FALSE) > - q = remote_query_supported_append (q, "vfork-events+"); > - if (packet_set_cmd_state (PACKET_exec_event_feature) > - != AUTO_BOOLEAN_FALSE) > - q = remote_query_supported_append (q, "exec-events+"); > - } > + if (packet_set_cmd_state (PACKET_fork_event_feature) > + != AUTO_BOOLEAN_FALSE) > + q = remote_query_supported_append (q, "fork-events+"); > + if (packet_set_cmd_state (PACKET_vfork_event_feature) > + != AUTO_BOOLEAN_FALSE) > + q = remote_query_supported_append (q, "vfork-events+"); > + if (packet_set_cmd_state (PACKET_exec_event_feature) > + != AUTO_BOOLEAN_FALSE) > + q = remote_query_supported_append (q, "exec-events+"); > > if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE) > q = remote_query_supported_append (q, "vContSupported+"); > @@ -4827,7 +4822,8 @@ remote_detach_1 (const char *args, int from_tty) > /* Tell the remote target to detach. */ > remote_detach_pid (pid); > > - if (from_tty && !rs->extended) > + /* Exit only if this is the only active inferior. */ > + if (from_tty && !rs->extended && number_of_inferiors () == 1) > puts_filtered (_("Ending remote debugging.\n")); > > /* Check to see if we are detaching a fork parent. Note that if we > @@ -4923,10 +4919,11 @@ remote_disconnect (struct target_ops *target, const char *args, int from_tty) > if (args) > error (_("Argument given to \"disconnect\" when remotely debugging.")); > > - /* Make sure we unpush even the extended remote targets; mourn > - won't do it. So call remote_mourn directly instead of > - target_mourn_inferior. */ > - remote_mourn (target); > + /* Make sure we unpush even the extended remote targets. Calling > + target_mourn_inferior won't unpush, and remote_mourn won't > + unpush if there is more than one inferior left. */ > + unpush_target (target); > + generic_mourn_inferior (); Note: this generic_mourn_inferior call here looks wrong, because we may be debugging more than one inferior. But remote_mourn was already wrong for the same reason, so, not a problem added by this patch. > > if (from_tty) > puts_filtered ("Ending remote debugging.\n"); > @@ -8562,42 +8559,6 @@ kill_new_fork_children (int pid, struct remote_state *rs) > } > > > -static void > -remote_kill (struct target_ops *ops) > -{ Please rename this (making it a helper function) instead of inlining it in the new remote_kill. E.g., remote_kill_k. > - > - /* Catch errors so the user can quit from gdb even when we > - aren't on speaking terms with the remote system. */ > - TRY > - { > - putpkt ("k"); > - } > - CATCH (ex, RETURN_MASK_ERROR) > - { > - if (ex.error == TARGET_CLOSE_ERROR) > - { > - /* If we got an (EOF) error that caused the target > - to go away, then we're done, that's what we wanted. > - "k" is susceptible to cause a premature EOF, given > - that the remote server isn't actually required to > - reply to "k", and it can happen that it doesn't > - even get to reply ACK to the "k". */ > - return; > - } > - > - /* Otherwise, something went wrong. We didn't actually kill > - the target. Just propagate the exception, and let the > - user or higher layers decide what to do. */ > - throw_exception (ex); > - } > - END_CATCH > - > - /* We've killed the remote end, we get to mourn it. Since this is > - target remote, single-process, mourning the inferior also > - unpushes remote_ops. */ > - target_mourn_inferior (); Maybe do still move this mourn out into new remote_kill. > -} > - > static int > remote_vkill (int pid, struct remote_state *rs) > { > @@ -8624,19 +8585,58 @@ remote_vkill (int pid, struct remote_state *rs) > } > > static void > -extended_remote_kill (struct target_ops *ops) > +remote_kill (struct target_ops *ops) > { > int res; > int pid = ptid_get_pid (inferior_ptid); > struct remote_state *rs = get_remote_state (); > > + /* If we are in 'target remote' mode and we are killing the only > + inferior, then we will tell gdbserver to exit and unpush the > + target. */ > + if (!rs->extended && number_of_inferiors () <= 1) It's number of inferiors, or number of _live_ inferiors that matters? I'd think the latter. Likewise all other new number_of_inferiors calls should be audited. > + { > + /* Catch errors so the user can quit from gdb even when we > + aren't on speaking terms with the remote system. */ > + TRY > + { > + putpkt ("k"); > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + if (ex.error == TARGET_CLOSE_ERROR) > + { > + /* If we got an (EOF) error that caused the target > + to go away, then we're done, that's what we wanted. > + "k" is susceptible to cause a premature EOF, given > + that the remote server isn't actually required to > + reply to "k", and it can happen that it doesn't > + even get to reply ACK to the "k". */ > + return; > + } > + > + /* Otherwise, something went wrong. We didn't actually kill > + the target. Just propagate the exception, and let the > + user or higher layers decide what to do. */ > + throw_exception (ex); > + } > + END_CATCH > + > + /* We've killed the remote end, we get to mourn it. Since this is > + target remote, single-process, mourning the inferior also > + unpushes remote_ops. */ Mentioning "single-process," here no longer looks right to me. > + target_mourn_inferior (); > + > + return; > + } > + > /* If we're stopped while forking and we haven't followed yet, kill the > child task. We need to do this before killing the parent task > because if this is a vfork then the parent will be sleeping. */ > kill_new_fork_children (pid, rs); > > res = remote_vkill (pid, rs); > - if (res == -1 && !(rs->extended && remote_multi_process_p (rs))) > + if (res == -1 && !(remote_multi_process_p (rs))) Drop the now-unnecessary parens. > { > /* Don't try 'k' on a multi-process aware stub -- it has no way > to specify the pid. */ I wonder about re-writing this function in top-down approach? #0 - if vkill is supported: - kill the new fork children first. - then kill the current process with remote_vkill. #1 - then if not multi-process, and there are still live processes, call the new remote_kill_k helper, to kill with "k". #2 - then, if !extended, and there are no live processes left, disconnect. Note this also gets rid of the putpkt("k") call, which currently misses handling the EOF issue already handled by the (new) remote_kill_k. > @@ -8662,16 +8662,17 @@ extended_remote_kill (struct target_ops *ops) > static void > remote_mourn (struct target_ops *target) > { > - unpush_target (target); > + struct remote_state *rs = get_remote_state (); > > - /* remote_close takes care of doing most of the clean up. */ > - generic_mourn_inferior (); > -} > + /* In 'target remote' mode with one inferior, we shut down gdbserver. */ I'd rather say: /* In 'target remote' mode with one inferior, we close the connection. */ > + if (!rs->extended && number_of_inferiors () <= 1) > + { > + unpush_target (target); > > -static void > -extended_remote_mourn (struct target_ops *target) > -{ > - struct remote_state *rs = get_remote_state (); > + /* remote_close takes care of doing most of the clean up. */ > + generic_mourn_inferior (); > + return; > + } > > /* In case we got here due to an error, but we're going to stay > connected. */ > @@ -8702,8 +8703,9 @@ extended_remote_mourn (struct target_ops *target) > current thread. */ > record_currthread (rs, minus_one_ptid); > > - /* Unlike "target remote", we do not want to unpush the target; then > - the next time the user says "run", we won't be connected. */ > + /* Unlike 'target remote' with no more inferiors, we do not want to > + unpush the target. If we do then the next time the user says > + "run", we won't be connected. */ > > /* Call common code to mark the inferior as not running. */ > generic_mourn_inferior (); > @@ -10224,7 +10226,7 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid) > { > if (ptid_equal (magic_null_ptid, ptid)) > xsnprintf (buf, sizeof buf, "Thread
"); > - else if (rs->extended && remote_multi_process_p (rs)) > + else if (remote_multi_process_p (rs)) > if (ptid_get_lwp (ptid) == 0) > return normal_pid_to_str (ptid); > else > @@ -11398,7 +11400,7 @@ remote_supports_multi_process (struct target_ops *self) > processes, even though plain remote can use the multi-process > thread id extensions, so that GDB knows the target process's > PID. */ Comment above needs updating. > - return rs->extended && remote_multi_process_p (rs); > + return remote_multi_process_p (rs); > } > Thanks, Pedro Alves