This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3] Target remote mode fork and exec events
- From: Pedro Alves <palves at redhat dot com>
- To: Don Breazeal <donb at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Fri, 20 Nov 2015 13:04:42 +0000
- Subject: Re: [PATCH 1/3] Target remote mode fork and exec events
- Authentication-results: sourceware.org; auth=none
- References: <1446854188-496-1-git-send-email-donb at codesourcery dot com> <1446854188-496-2-git-send-email-donb at codesourcery dot com>
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 <donb@sourceware.org>
>
> * 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 <donb@sourceware.org>
>
> * 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 <main>");
> - 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