Re: [PATCH v3 5/7] Add thread_db_notice_clone to gdbserver

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Kevin,

On 2017-07-19 02:55, Kevin Buettner wrote:
> While working on a patch for fetching a thread handle in gdbserver, I
> ran into a circumstance in which tests in gdb.mi/mi-nsmoribund.exp
> would occasionally fail.  Over a large enough number of runs, it would
> fail roughly 2% of the time.
> 
> That thread handle patch caused find_one_thread() to be called on
> every stop.  find_one_thread() calls td_ta_map_lwp2thr() which, in
> turn, can cause ps_get_thread_area() to be called.
> ps_get_thread_area() makes a call to ptrace() for getting the thread
> area address.  If this should happen when the thread is not stopped,
> the call to ptrace will return error which in turn propogates back to
> find_one_thread().  find_one_thread() calls error() in this instance
> which causes the program to die.
> 
> This patch causes find_one_thread() to be called upon reciept of a
> clone event.  Since the clone is stopped, the circumstances described
> above cannot occur.
> 
> gdb/gdbserver/ChangeLog:
> 
>     	* linux-low.c (handle_extended_wait): Call 
> thread_db_notice_clone().
>     	* linux-low.h (thread_db_notice_clone): Declare.
>     	* thread-db.c (thread_db_notice_clone): New function.
> ---
>  gdb/gdbserver/linux-low.c |  2 ++
>  gdb/gdbserver/linux-low.h |  7 +++++++
>  gdb/gdbserver/thread-db.c | 14 ++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 3d7cfe3..9d831e7 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -656,6 +656,8 @@ handle_extended_wait (struct lwp_info
> **orig_event_lwp, int wstat)
>  	  new_lwp->status_pending = status;
>  	}
> 
> +      thread_db_notice_clone (get_thread_process (event_thr), ptid);
> +
>        /* Don't report the event.  */
>        return 1;
>      }
> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> index 6328da0..86cfe51 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -410,4 +410,11 @@ int thread_db_get_tls_address (struct thread_info
> *thread, CORE_ADDR offset,
>  			       CORE_ADDR load_module, CORE_ADDR *address);
>  int thread_db_look_up_one_symbol (const char *name, CORE_ADDR *addrp);
> 
> +/* Called from linux-low.c when a clone event is detected.  Upon 
> entry,
> +   both the clone and the parent should be stopped.  This function 
> does
> +   whatever is required have the clone under thread_db's control.  */

New line here.

> +void thread_db_notice_clone (struct process_info *proc, ptid_t lwp);
> +
> +int thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int 
> *handle_len);

This last declaration doesn't belong in this patch I think.

>  extern int have_ptrace_getregset;
> diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
> index 1ffb79d..eff1914 100644
> --- a/gdb/gdbserver/thread-db.c
> +++ b/gdb/gdbserver/thread-db.c
> @@ -864,3 +864,17 @@ thread_db_handle_monitor_command (char *mon)
>    /* Tell server.c to perform default processing.  */
>    return 0;
>  }
> +

Just add a

   /* See linux-low.h.  */

to be fully GDB-style-compliant :)

> +void
> +thread_db_notice_clone (struct process_info *proc, ptid_t ptid)
> +{
> +  struct thread_db *thread_db = proc->priv->thread_db;
> +
> +  /* If the thread layer isn't initialized, return.  It may just
> +     be that the program uses clone, but does not use libthread_db.  
> */
> +  if (thread_db == NULL || !thread_db->all_symbols_looked_up)
> +    return;
> +
> +  if (!find_one_thread (ptid))
> +    warning ("Cannot find thread after clone.\n");
> +}

Again, this patch LGTM, but if somebody else could look at it I'd 
appreciate it.

Thanks!

Simon