This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v6 6/6] Remote fork catch


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]