Re: [patch] 7.3 regression for corrupted core files threads

[Pedro Alves <palves at redhat dot com>]

On 04/17/2012 07:54 PM, Jan Kratochvil wrote:

> Hi,
> 
> while this feature was great
> 	PR corefile/8210: Linux core files should use linux-thread-db.c
> 	http://sourceware.org/ml/gdb-patches/2010-08/msg00208.html
> 
> it has caused a regression - if the thread list is corrupted GDB displayed LWP
> list before but current GDB does not display even the LWP list.
> 
> This fix does:
> 
> -[Thread debugging using libthread_db enabled]^M
> -Using host libthread_db library "/lib64/libthread_db.so.1".^M
> -Cannot find new threads: debugger service failed^M
> +warning: td_ta_thr_iter failed: debugger service failed^M
> +warning: td_ta_thr_iter failed: debugger service failed^M
> +warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available.^M
> +warning: td_ta_thr_iter failed: debugger service failed^M
> +warning: td_ta_thr_iter failed: debugger service failed^M
> +warning: Unable to find libthread_db matching inferior's thread library, thread debugging will not be available.^M
>  Core was generated by `gcore-thread'.^M
>  Program terminated with signal 5, Trace/breakpoint trap.^M
>  #0  thread2 (arg=0xdeadbeef) at ./gdb.threads/pthreads.c:90^M
>  90       int k = 0;^M
>  (gdb) PASS: gdb.threads/gcore-thread.exp: core0file: re-load generated corefile
>  info threads^M
> -Cannot find new threads: debugger service failed^M
>  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> -(gdb) FAIL: gdb.threads/gcore-thread.exp: core0file: corefile contains at least two threads
> +  Id   Target Id         Frame ^M
> +  3    LWP 4644          0x00007ffff75c99bd in nanosleep () at ../sysdeps/unix/syscall-template.S:82^M
> +  2    LWP 4649          0x00007ffff75c99bd in nanosleep () at ../sysdeps/unix/syscall-template.S:82^M
> +* 1    LWP 4650          thread2 (arg=0xdeadbeef) at ./gdb.threads/pthreads.c:90^M
> +(gdb) PASS: gdb.threads/gcore-thread.exp: core0file: corefile contains at least two threads
> 
> No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2012-04-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* linux-thread-db.c (find_new_threads_once): New prototype.
> 	(try_thread_db_load_1): Try to call find_new_threads_once.
> 
> gdb/testsuite/
> 2012-04-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* gdb.threads/gcore-thread.exp: Remove variable libthread_db_seen.
> 	Wrap the test into loop for corefile and core0file.
> 
> diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
> index 4d09c6e..3da206c 100644
> --- a/gdb/linux-thread-db.c
> +++ b/gdb/linux-thread-db.c
> @@ -185,6 +185,8 @@ struct thread_db_info
>  struct thread_db_info *thread_db_list;
>  
>  static void thread_db_find_new_threads_1 (ptid_t ptid);
> +static int find_new_threads_once (struct thread_db_info *info, int iteration,
> +				  td_err_e *errp);
>  static void thread_db_find_new_threads_2 (ptid_t ptid, int until_no_new);
>  
>  /* Add the current inferior to the list of processes using libpthread.
> @@ -739,6 +741,17 @@ try_thread_db_load_1 (struct thread_db_info *info)
>    info->td_thr_event_enable_p = dlsym (info->handle, "td_thr_event_enable");
>    info->td_thr_tls_get_addr_p = dlsym (info->handle, "td_thr_tls_get_addr");
>  
> +  find_new_threads_once (info, 0, &err);
> +  if (err != TD_OK)
> +    {
> +      /* Even if libthread_db initializes in the case the thread list is
> +	 corrupted it would not list any threads.  Fall back to list at least
> +	 LWPs in such case.  */


I'd think that a thread list might end up corrupted halfway instead of
at the head, and GDB could manage to walk over and list some threads.
Anyway, I'd suggest adding a comma, and perhaps a little editing further even:

      /* Even if libthread_db initializes, if the thread list is
	 corrupted, we'd not manage to list any threads.  Better reject this
         thread_db, and fall back to at least listing LWPs.  */

> +

> +      warning (_("td_ta_thr_iter failed: %s"), thread_db_err_str (err));


How about making that a bit more user friendly?  Something like:

      warning (_("couldn't activate thread debugging using libthread_db: %s"), thread_db_err_str (err));

> +      return 0;
> +    }


Unfortunately this conflicts with this bit at the end of the function:

  /* There appears to be a bug in glibc-2.3.6: calls to td_thr_get_info fail
     with TD_ERR for statically linked executables if td_thr_get_info is
     called before glibc has initialized itself.  Silently ignore such
     errors, and let gdb enumerate threads again later.  */
  thread_db_find_new_threads_silently (inferior_ptid);

Maybe it'd be good enough to condition your new check on !target_has_execution?

-- 
Pedro Alves