This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v6 6/6] Remote fork catch
- From: Pedro Alves <palves at redhat dot com>
- To: Don Breazeal <donb at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Tue, 24 Mar 2015 12:47:21 +0000
- Subject: Re: [PATCH v6 6/6] Remote fork catch
- Authentication-results: sourceware.org; auth=none
- References: <1424997977-13316-1-git-send-email-donb at codesourcery dot com> <1426625788-4469-1-git-send-email-donb at codesourcery dot com> <1426625788-4469-7-git-send-email-donb at codesourcery dot com>
On 03/17/2015 08:56 PM, Don Breazeal wrote:
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 8fa6f8a..346f2c4 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -1356,6 +1356,15 @@ handle_qxfer_threads_worker (struct inferior_list_entry *inf, void *arg)
> int core = target_core_of_thread (ptid);
> char core_s[21];
>
> + /* Skip new threads created as the result of a fork if we are not done
> + handling that fork event. We won't know whether to tell GDB about
> + the new thread until we are done following the fork. */
> + if ((last_status.kind == TARGET_WAITKIND_FORKED
> + || last_status.kind == TARGET_WAITKIND_VFORKED)
> + && (ptid_get_pid (last_status.value.related_pid)
> + == ptid_get_pid (ptid)))
> + return;
This use of last_status here is really just as bad as
get_last_target_status, for the same reasons. What if a thread
forks at the same time another thread hits a breakpoint, and
we end up reporting the breakpoint first, leaving the fork
pending? Sounds like we'll end up listing the child fork
thread then.
> +
> write_ptid (ptid_s, ptid);
>
> if (core != -1)
> @@ -4144,3 +4153,12 @@ handle_target_event (int err, gdb_client_data client_data)
>
> return 0;
> }
> +
> +/* Retrieve the last waitstatus reported to GDB. */
> +
> +void
> +get_last_target_status (ptid_t *ptid, struct target_waitstatus *last)
> +{
> + *ptid = last_ptid;
> + *last = last_status;
> +}
Looks like you forgot to delete the function. :-)
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index 09a5624..8c6ec27 100644
> --- a/gdb/gdbserver/server.h
> +++ b/gdb/gdbserver/server.h
> @@ -113,6 +113,8 @@ typedef int gdb_fildes_t;
> /* Functions from server.c. */
> extern int handle_serial_event (int err, gdb_client_data client_data);
> extern int handle_target_event (int err, gdb_client_data client_data);
> +extern void get_last_target_status (ptid_t *ptid,
> + struct target_waitstatus *last);
>
> #include "remote-utils.h"
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index d1ba62d..44ee89f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8012,6 +8060,23 @@ extended_remote_kill (struct target_ops *ops)
> int res;
> int pid = ptid_get_pid (inferior_ptid);
> struct remote_state *rs = get_remote_state ();
> + struct thread_info *tp = find_thread_ptid (inferior_ptid);
> +
> + /* If we're stopped while forking and we haven't followed yet,
> + kill the child task. We need to do this first because the
> + parent will be sleeping if this is a vfork. */
> +
> + if (tp != NULL && (tp->pending_follow.kind == TARGET_WAITKIND_FORKED
> + || tp->pending_follow.kind == TARGET_WAITKIND_VFORKED))
Looks like this will miss killing the child if the user switches to
some thread other than the one that forked, in case it was a multi-threaded
program that forked.
> + {
> + ptid_t parent_ptid = inferior_ptid;
> +
> + inferior_ptid = tp->pending_follow.value.related_pid;
> + set_general_thread (inferior_ptid);
> + extended_remote_kill (ops);
> + inferior_ptid = parent_ptid;
> + set_general_thread (inferior_ptid);
We never want to use the 'k' packet here, so this could
just simply be:
int child_pid = tp->pending_follow.value.related_pid;
remote_vkill (child_pid);
> + }
>
> res = remote_vkill (pid, rs);
> if (res == -1 && !(rs->extended && remote_multi_process_p (rs)))
> @@ -12036,6 +12101,14 @@ Specify the serial device it is connected to (e.g. /dev/ttya).";
> extended_remote_ops.to_supports_disable_randomization
> = extended_remote_supports_disable_randomization;
> extended_remote_ops.to_follow_fork = remote_follow_fork;
> + extended_remote_ops.to_insert_fork_catchpoint
> + = remote_insert_fork_catchpoint;
> + extended_remote_ops.to_remove_fork_catchpoint
> + = remote_remove_fork_catchpoint;
> + extended_remote_ops.to_insert_vfork_catchpoint
> + = remote_insert_vfork_catchpoint;
> + extended_remote_ops.to_remove_vfork_catchpoint
> + = remote_remove_vfork_catchpoint;
> }
>
> static int
> diff --git a/gdb/testsuite/gdb.threads/fork-thread-pending.exp b/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> index d229232..594f376 100644
> --- a/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> +++ b/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> @@ -31,6 +31,26 @@ if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executab
> return -1
> }
>
> +# Find a thread that did not fork and is not the main thread and
> +# return its thread number. We can't just hard-code the thread
> +# number since we have no guarantee as to the ordering of the threads
> +# in gdb.
I don't understand this -- the test runs to main first, so the main
thread should always be thread 1, no?
> We know that the main thread is in pthread_join and the
> +# forking thread is in fork, so we use this rather ungainly regexp
> +# to capture an entry from 'info threads' that doesn't show one of
> +# those routines, then extract the thread number.
> +
> +proc find_unforked_thread { } {
> + gdb_test_multiple "info threads" "find unforked thread" {
> + -re "(\[^\r]*Thread\[^\r]* in \[^fp]\[^ot]\[^rh]\[^kr]\[^e]\[^a]\[^d]\[^_]\[^j]\[^\r]*\r\n)" {
> + regexp "(\[ ]*)(\[0-9]*)(\[ ]*Thread\[^\r]*\r\n)" $expect_out(0,string) ignore lead_spc threadnum rest
> + }
> + timeout {
> + set threadnum -1
> + }
> + }
> + return $threadnum
> +}
> +
> clean_restart ${binfile}
>
> if ![runto_main] then {
> @@ -46,7 +66,8 @@ gdb_test "continue" "Catchpoint.*" "1, get to the fork event"
>
> gdb_test "info threads" " Thread .* Thread .* Thread .* Thread .*" "1, multiple threads found"
>
> -gdb_test "thread 1" ".*" "1, switched away from event thread"
> +set threadnum [find_unforked_thread]
> +gdb_test "thread $threadnum" ".*" "1, switched away from event thread to thread $threadnum"
>
> gdb_test "continue" "Not resuming.*" "1, refused to resume"
>
>
Thanks,
Pedro Alves