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

Simon Marchi
2017-07-23T21:27:55Z

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