[4/5] Linux: Skip thread_db thread event reporting if PTRACE_EVENT_CLONE is supported

  [A test I wrote stumbled on a libthread_db issue related to thread
event breakpoints.  See glibc PR17705:
 [nptl_db: stale thread create/death events if debugger detaches]
 https://sourceware.org/bugzilla/show_bug.cgi?id=17705

This patch avoids that whole issue by making GDB stop using thread
event breakpoints in the first place, which is good for other reasons
as well, anyway.]

Before PTRACE_EVENT_CLONE (Linux 2.6), the only way to learn about new
threads in the inferior (to attach to them) or to learn about thread
exit was to coordinate with the inferior's glibc/runtime, using
libthread_db.  That works by putting a breakpoint at a magic address
which is called when a new thread is spawned, or when a thread is
about to exit.  When that breakpoint is hit, all threads are stopped,
and then GDB coordinates with libthread_db to read data structures out
of the inferior to learn about what happened.  Then the breakpoint is
single-stepped, and then all threads are re-resumed.  This isn't very
efficient (stops all threads) and is more fragile (inferior's thread
list in memory may be corrupt; libthread_db bugs, etc.) than ideal.

When the kernel supports PTRACE_EVENT_CLONE (which we already make use
of), there's really no need to use libthread_db's event reporting
mechanism to learn about new LWPs.  And if the kernel supports that,
then we learn about LWP exits through regular WIFEXITED wait statuses,
so no need for the death event breakpoint either.

GDBserver has been likewise skipping the thread_db events for a long
while:
  https://sourceware.org/ml/gdb-patches/2007-10/msg00547.html

There's one user-visible difference: we'll no longer print about
threads being created and exiting while the program is running, like:

 [Thread 0x7ffff7dbb700 (LWP 30670) exited]
 [New Thread 0x7ffff7db3700 (LWP 30671)]
 [Thread 0x7ffff7dd3700 (LWP 30667) exited]
 [New Thread 0x7ffff7dab700 (LWP 30672)]
 [Thread 0x7ffff7db3700 (LWP 30671) exited]
 [Thread 0x7ffff7dcb700 (LWP 30668) exited]

This is exactly the same behavior as when debugging against remote
targets / gdbserver.  I actually think that's a good thing (and as
such have listed this in the local/remote parity wiki page a while
ago), as the printing slows down the inferior.  It's also a
distraction to keep bothering the user about short-lived threads that
she won't be able to interact with anyway.  Instead, the user (and
frontend) will be informed about new threads that currently exist in
the program when the program next stops:

 (gdb) c
 ...
 * ctrl-c *
 [New Thread 0x7ffff7963700 (LWP 7797)]
 [New Thread 0x7ffff796b700 (LWP 7796)]

 Program received signal SIGINT, Interrupt.
 [Switching to Thread 0x7ffff796b700 (LWP 7796)]
 clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:81
 81              testq   %rax,%rax
 (gdb) info threads

A couple of tests had assumptions on GDB thread numbers that no longer
hold.

Tested on x86_64 Fedora 20.

gdb/
2014-12-16  Pedro Alves  <palves@redhat.com>

	Skip enabling event reporting if the kernel supports
	PTRACE_EVENT_CLONE.
	* linux-thread-db.c: Include "nat/linux-ptrace.h".
	(thread_db_use_events): New function.
	(try_thread_db_load_1): Check thread_db_use_events before enabling
	event reporting.
	(update_thread_state): New function.
	(attach_thread): Use it.  Check thread_db_use_events before
	enabling event reporting.
	(thread_db_detach): Check thread_db_use_events before disabling
	event reporting.
	(find_new_threads_callback): Check thread_db_use_events before
	enabling event reporting.  Update the thread's state if not using
	libthread_db events.

gdb/testsuite/
2014-12-16  Pedro Alves  <palves@redhat.com>

	* gdb.threads/fork-thread-pending.exp: Switch to the main thread
	instead of to thread 2.
	* gdb.threads/signal-command-multiple-signals-pending.c (main):
	Add barrier around each pthread_create call instead of around all
	calls.
	* gdb.threads/signal-command-multiple-signals-pending.exp (test):
	Set a break on thread_function and have the child threads hit it
	one at at a time.
---
 gdb/linux-thread-db.c                              | 39 ++++++++++++++++++----
 gdb/testsuite/gdb.threads/fork-thread-pending.exp  |  2 +-
 .../signal-command-multiple-signals-pending.c      | 11 +++---
 .../signal-command-multiple-signals-pending.exp    |  7 ++++
 4 files changed, 47 insertions(+), 12 deletions(-)
  

On 2014-12-16 11:53 AM, Pedro Alves wrote:
> [A test I wrote stumbled on a libthread_db issue related to thread
> event breakpoints.  See glibc PR17705:
>  [nptl_db: stale thread create/death events if debugger detaches]
>  https://sourceware.org/bugzilla/show_bug.cgi?id=17705
> 
> This patch avoids that whole issue by making GDB stop using thread
> event breakpoints in the first place, which is good for other reasons
> as well, anyway.]
> 
> Before PTRACE_EVENT_CLONE (Linux 2.6), the only way to learn about new
> threads in the inferior (to attach to them) or to learn about thread
> exit was to coordinate with the inferior's glibc/runtime, using
> libthread_db.  That works by putting a breakpoint at a magic address
> which is called when a new thread is spawned, or when a thread is
> about to exit.  When that breakpoint is hit, all threads are stopped,
> and then GDB coordinates with libthread_db to read data structures out
> of the inferior to learn about what happened.

That is libthread_db's TD_CREATE event? Could you point out where that is
done (stopping all the threads)? From the previous discussion with you, I
was thinking that those breakpoints did not affect execution. I don't find
any code in linux-thread-db.c that would do such a thing.

> Then the breakpoint is
> single-stepped, and then all threads are re-resumed.  This isn't very
> efficient (stops all threads) and is more fragile (inferior's thread
> list in memory may be corrupt; libthread_db bugs, etc.) than ideal.
> 
> When the kernel supports PTRACE_EVENT_CLONE (which we already make use
> of), there's really no need to use libthread_db's event reporting
> mechanism to learn about new LWPs.  And if the kernel supports that,
> then we learn about LWP exits through regular WIFEXITED wait statuses,
> so no need for the death event breakpoint either.
> 
> GDBserver has been likewise skipping the thread_db events for a long
> while:
>   https://sourceware.org/ml/gdb-patches/2007-10/msg00547.html
> 
> There's one user-visible difference: we'll no longer print about
> threads being created and exiting while the program is running, like:
> 
>  [Thread 0x7ffff7dbb700 (LWP 30670) exited]
>  [New Thread 0x7ffff7db3700 (LWP 30671)]
>  [Thread 0x7ffff7dd3700 (LWP 30667) exited]
>  [New Thread 0x7ffff7dab700 (LWP 30672)]
>  [Thread 0x7ffff7db3700 (LWP 30671) exited]
>  [Thread 0x7ffff7dcb700 (LWP 30668) exited]
> 
> This is exactly the same behavior as when debugging against remote
> targets / gdbserver.  I actually think that's a good thing (and as
> such have listed this in the local/remote parity wiki page a while
> ago), as the printing slows down the inferior.  It's also a
> distraction to keep bothering the user about short-lived threads that
> she won't be able to interact with anyway.  Instead, the user (and
> frontend) will be informed about new threads that currently exist in
> the program when the program next stops:

Is this a consequence of the change of algorithm, or did you actively changed
the behavior?

From what I understand, gdb still attaches to the new thread as soon as it spawns
(when it receives the PTRACE_EVENT_CLONE event), so it could print the notice when
the event happens. Not that I mind, but I just want to understand.

>  (gdb) c
>  ...
>  * ctrl-c *
>  [New Thread 0x7ffff7963700 (LWP 7797)]
>  [New Thread 0x7ffff796b700 (LWP 7796)]
> 
>  Program received signal SIGINT, Interrupt.
>  [Switching to Thread 0x7ffff796b700 (LWP 7796)]
>  clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:81
>  81              testq   %rax,%rax
>  (gdb) info threads
> 
> A couple of tests had assumptions on GDB thread numbers that no longer
> hold.
> 
> Tested on x86_64 Fedora 20.
> 
> gdb/
> 2014-12-16  Pedro Alves  <palves@redhat.com>
> 
> 	Skip enabling event reporting if the kernel supports
> 	PTRACE_EVENT_CLONE.
> 	* linux-thread-db.c: Include "nat/linux-ptrace.h".
> 	(thread_db_use_events): New function.
> 	(try_thread_db_load_1): Check thread_db_use_events before enabling
> 	event reporting.
> 	(update_thread_state): New function.
> 	(attach_thread): Use it.  Check thread_db_use_events before
> 	enabling event reporting.
> 	(thread_db_detach): Check thread_db_use_events before disabling
> 	event reporting.
> 	(find_new_threads_callback): Check thread_db_use_events before
> 	enabling event reporting.  Update the thread's state if not using
> 	libthread_db events.
> 
> gdb/testsuite/
> 2014-12-16  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.threads/fork-thread-pending.exp: Switch to the main thread
> 	instead of to thread 2.
> 	* gdb.threads/signal-command-multiple-signals-pending.c (main):
> 	Add barrier around each pthread_create call instead of around all
> 	calls.
> 	* gdb.threads/signal-command-multiple-signals-pending.exp (test):
> 	Set a break on thread_function and have the child threads hit it
> 	one at at a time.
> ---
>  gdb/linux-thread-db.c                              | 39 ++++++++++++++++++----
>  gdb/testsuite/gdb.threads/fork-thread-pending.exp  |  2 +-
>  .../signal-command-multiple-signals-pending.c      | 11 +++---
>  .../signal-command-multiple-signals-pending.exp    |  7 ++++
>  4 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
> index 4b26984..35181f0 100644
> --- a/gdb/linux-thread-db.c
> +++ b/gdb/linux-thread-db.c
> @@ -38,6 +38,7 @@
>  #include "observer.h"
>  #include "linux-nat.h"
>  #include "nat/linux-procfs.h"
> +#include "nat/linux-ptrace.h"
>  #include "nat/linux-osdata.h"
>  #include "auto-load.h"
>  #include "cli/cli-utils.h"
> @@ -77,6 +78,16 @@ static char *libthread_db_search_path;
>     by the "set auto-load libthread-db" command.  */
>  static int auto_load_thread_db = 1;
>  
> +/* Returns true if we need to use thread_db thread create/death event
> +   breakpoints to learn about threads.  */
> +
> +static int
> +thread_db_use_events (void)
> +{
> +  /* Not necessary if the kernel supports clone events.  */
> +  return !linux_supports_traceclone ();
> +}
> +
>  /* "show" command for the auto_load_thread_db configuration variable.  */
>  
>  static void
> @@ -832,7 +843,7 @@ try_thread_db_load_1 (struct thread_db_info *info)
>      push_target (&thread_db_ops);
>  
>    /* Enable event reporting, but not when debugging a core file.  */
> -  if (target_has_execution)
> +  if (target_has_execution && thread_db_use_events ())
>      enable_thread_event_reporting ();
>  
>    return 1;
> @@ -1260,6 +1271,17 @@ thread_db_inferior_created (struct target_ops *target, int from_tty)
>    check_for_thread_db ();
>  }
>  
> +/* Update the thread's state (what's displayed in "info threads"),
> +   from libthread_db thread state information.  */
> +
> +static void
> +update_thread_state (struct private_thread_info *private,
> +		     const td_thrinfo_t *ti_p)
> +{
> +  private->dying = (ti_p->ti_state == TD_THR_UNKNOWN
> +		    || ti_p->ti_state == TD_THR_ZOMBIE);
> +}
> +
>  /* Attach to a new thread.  This function is called when we receive a
>     TD_CREATE event or when we iterate over all threads and find one
>     that wasn't already in our list.  Returns true on success.  */
> @@ -1341,8 +1363,7 @@ attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
>    gdb_assert (ti_p->ti_tid != 0);
>    private->th = *th_p;
>    private->tid = ti_p->ti_tid;
> -  if (ti_p->ti_state == TD_THR_UNKNOWN || ti_p->ti_state == TD_THR_ZOMBIE)
> -    private->dying = 1;
> +  update_thread_state (private, ti_p);
>  
>    /* Add the thread to GDB's thread list.  */
>    if (tp == NULL)
> @@ -1354,7 +1375,7 @@ attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
>  
>    /* Enable thread event reporting for this thread, except when
>       debugging a core file.  */
> -  if (target_has_execution)
> +  if (target_has_execution && thread_db_use_events ())
>      {
>        err = info->td_thr_event_enable_p (th_p, 1);
>        if (err != TD_OK)
> @@ -1393,7 +1414,7 @@ thread_db_detach (struct target_ops *ops, const char *args, int from_tty)
>  
>    if (info)
>      {
> -      if (target_has_execution)
> +      if (target_has_execution && thread_db_use_events ())
>  	{
>  	  disable_thread_event_reporting (info);
>  
> @@ -1629,7 +1650,7 @@ find_new_threads_callback (const td_thrhandle_t *th_p, void *data)
>  	 need this glibc bug workaround.  */
>        info->need_stale_parent_threads_check = 0;
>  
> -      if (target_has_execution)
> +      if (target_has_execution && thread_db_use_events ())
>  	{
>  	  err = info->td_thr_event_enable_p (th_p, 1);
>  	  if (err != TD_OK)
> @@ -1666,6 +1687,12 @@ find_new_threads_callback (const td_thrhandle_t *th_p, void *data)
>  	   iteration: thread_db_find_new_threads_2 will retry.  */
>  	return 1;
>      }
> +  else if (target_has_execution && !thread_db_use_events ())
> +    {
> +      /* Need to update this if not using the libthread_db events
> +	 (particularly, the TD_DEATH event).  */
> +      update_thread_state (tp->private, &ti);
> +    }
>  
>    return 0;
>  }
> diff --git a/gdb/testsuite/gdb.threads/fork-thread-pending.exp b/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> index 57e45c9..357cb9e 100644
> --- a/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> +++ b/gdb/testsuite/gdb.threads/fork-thread-pending.exp
> @@ -46,7 +46,7 @@ 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 2" ".*" "1, switched away from event thread"
> +gdb_test "thread 1" ".*" "1, switched away from event thread"
>  
>  gdb_test "continue" "Not resuming.*" "1, refused to resume"
>  
> diff --git a/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.c b/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.c
> index 2fc5f53..761bef1 100644
> --- a/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.c
> +++ b/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.c
> @@ -76,12 +76,13 @@ main (void)
>    signal (SIGUSR1, handler_sigusr1);
>    signal (SIGUSR2, handler_sigusr2);
>  
> -  pthread_barrier_init (&barrier, NULL, 3);
> -
>    for (i = 0; i < 2; i++)
> -    pthread_create (&child_thread[i], NULL, thread_function, NULL);
> -
> -  pthread_barrier_wait (&barrier);
> +    {
> +      pthread_barrier_init (&barrier, NULL, 2);
> +      pthread_create (&child_thread[i], NULL, thread_function, NULL);
> +      pthread_barrier_wait (&barrier);
> +      pthread_barrier_destroy (&barrier);
> +    }
>  
>    all_threads_started ();
>  
> diff --git a/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.exp b/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.exp
> index b5ec00a..f574c57 100644
> --- a/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.exp
> +++ b/gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.exp
> @@ -46,6 +46,13 @@ proc test { schedlock } {
>  	gdb_test "handle SIGUSR2 stop print pass"
>  
>  	gdb_test "break all_threads_started" "Breakpoint .* at .*$srcfile.*"
> +
> +	# Create threads one at a time, to insure stable thread
> +	# numbers between runs and targets.
> +	gdb_test "break thread_function" "Breakpoint .* at .*$srcfile.*"
> +	gdb_test "continue" "thread_function.*" "thread 2 created"
> +	gdb_test "continue" "thread_function.*" "thread 3 created"
> +
>  	gdb_test "continue" "all_threads_started.*"
>  
>  	# Using schedlock, let the main thread queue a signal for each
>
  

On 12/16/2014 09:24 PM, Simon Marchi wrote:

>> Before PTRACE_EVENT_CLONE (Linux 2.6), the only way to learn about new
>> threads in the inferior (to attach to them) or to learn about thread
>> exit was to coordinate with the inferior's glibc/runtime, using
>> libthread_db.  That works by putting a breakpoint at a magic address
>> which is called when a new thread is spawned, or when a thread is
>> about to exit.  When that breakpoint is hit, all threads are stopped,
>> and then GDB coordinates with libthread_db to read data structures out
>> of the inferior to learn about what happened.
> 
> That is libthread_db's TD_CREATE event? Could you point out where that is
> done (stopping all the threads)?

When we're using libthread_db, the linux-thread-db.c target is pushed on
top of the target stack.  So a target_wait call ends up in
linux-thread-db.c:thread_db_wait:

static ptid_t
thread_db_wait (struct target_ops *ops,
		ptid_t ptid, struct target_waitstatus *ourstatus,
		int options)
{
...
  ptid = beneath->to_wait (beneath, ptid, ourstatus, options);
...
  if (ourstatus->kind == TARGET_WAITKIND_STOPPED
      && ourstatus->value.sig == GDB_SIGNAL_TRAP)
    /* Check for a thread event.  */
    check_event (ptid);
...
  return ptid;
}

and the beneath->to_wait call ends up in linux_nat_wait -- _that_ is
what stops all threads just before returning to thread_db_wait.

> From the previous discussion with you, I
> was thinking that those breakpoints did not affect execution. I don't find
> any code in linux-thread-db.c that would do such a thing.

I think you're thinking of https://sourceware.org/ml/gdb-patches/2014-12/msg00210.html

What I was saying is that although the TD_DEATH event results in all
threads stopping and then gdb core resuming the target, it's not when the
TD_DEATH event breakpoint is hit that we call delete_thread, so that's
not when mi_thread_exit ends up called.  Instead, after TD_DEATH, the
thread that is exiting actually still exists and is resumed (it still has
a few instructions to run inside glibc/pthread before actually calling
the exit syscall), and then later when the thread actually does the exit
syscall, waitpid returns an WIFEXITED status for it, and gdb _then_ calls
delete_thread, all within linux-nat.c, without stopping all threads.

>> This is exactly the same behavior as when debugging against remote
>> targets / gdbserver.  I actually think that's a good thing (and as
>> such have listed this in the local/remote parity wiki page a while
>> ago), as the printing slows down the inferior.  It's also a
>> distraction to keep bothering the user about short-lived threads that
>> she won't be able to interact with anyway.  Instead, the user (and
>> frontend) will be informed about new threads that currently exist in
>> the program when the program next stops:
> 
> Is this a consequence of the change of algorithm, or did you actively changed
> the behavior?

Both.  :-)  I made GDB do an implicit "info threads" just before
presenting a user-visible stop to the user a while ago.  See the
update_thread_list call in normal_stop, added in git b57bacecd5 -- see
also references to local/remote parity in that commit's log.

And it's a consequence in that stopping linux-thread-db.c from calling
add_thread results in that update_thread_list call finding new and dead
threads then.

> From what I understand, gdb still attaches to the new thread as soon as it spawns
> (when it receives the PTRACE_EVENT_CLONE event), 

Close, with PTRACE_EVENT_CLONE, gdb is automatically attached to the
new clone; the kernel does that for us.

> so it could print the notice when the event happens. 

Right, see the code in linux_handle_extended_wait that does that,
in non-stop mode, only.  I'd like to remove that bit soon enough
though.  I've mentioned before that I regret having added it.

> Not that I mind, but I just want to understand.

Hope I made things a little clearer.

Thanks,
Pedro Alves