Re: [RFC]: fix for recycled thread ids

[Jeff Johnston <jjohnstn@redhat.com>]

Daniel Jacobowitz wrote:
> On Wed, Mar 24, 2004 at 10:51:30AM -0500, Daniel Jacobowitz wrote:
> 
>>Fortunately, in <gnu/libc-version.h> there is a function to return the
>>runtime version of glibc.  We should be able to use that - and the not
>>100% valid, but generally valid and already assumed by thread_db,
>>assumption that a native GDB, when used to debug native programs, is
>>debugging the same version of the C library - to enable TD_DEATH when
>>it is safe to do so.  This will let us detach the threads when they
>>die.
>>
>>That has its own risks, since the thread continues to run for a short
>>while after the death event is reported.  For instance, in NPTL the
>>thread reports the event and then cleans up after itself; in LT I don't
>>remember whether the manager or the thread does this, but I think it's
>>the same.  I already wrote limited code to handle this, if you search
>>for "zombie" in thread-db.c, so it should be OK.  The gist is that we
>>remove it from the thread list right away, but do not detach the
>>thread.  We resist attaching to zombies.
>>
>>At least, all that is how it looks to me.  I'll experiment with
>>TD_DEATH before I speculate further.
> 
> 
> Here's a proof-of-concept patch.  It is not quite right, in that now I
> can hit C-c about four times before I get the error().  It does
> illustrate what I was suggesting, though, minus the version checking.
> 
> The reason we still get the segfaults is not that surprising.  I spent
> a little while coaxing it into producing a core dump, and got this:
> 
> (gdb) i threads
>   7 process 10303  0x40034531 in __nptl_create_event () from /lib/tls/i686/cmov/libpthread.so.0
>   6 process 12467  0xffffe410 in __kernel_vsyscall ()
>   5 process 12468  0xffffe410 in __kernel_vsyscall ()
>   4 process 12469  0x0804884e in threadfunc (arg=0x25) at lphello.c:64
>   3 process 12470  0x40034540 in __nptl_death_event () from /lib/tls/i686/cmov/libpthread.so.0
>   2 process 12471  0xffffe410 in __kernel_vsyscall ()
> * 1 process 12472  0xffffe410 in __kernel_vsyscall ()
> 
> So one thread is running, four are probably sleeping, one is dying...
> and one is creating another thread.  I'd bet money that we aren't
> attached to the one being created yet.  I can produce all sorts of
> other interesting GDB internal errors this way, too.
> 

I also tried the TD_DEATH method but gave up after running into segfaults.  I 
have to go back and recheck whether those segfaults could occur without the 
Crtl-C being issued.  I can't remember off-hand.

> Does this patch fix the problems you were seeing with less pathological
> test cases?  Eclipse, in this case, is a less pathological test case,
> since it creates threads that actually do a bit of work.  If it does,
> I'll add the necessary glibc version checking.  The only way to fix the
> race on the create side is to add PTRACE_EVENT_CLONE support (this _is_
> what it was for), but that will require some major surgery.
> 

As you know, I am also looking at the PTRACE_EVENT_CLONE stuff.  I'll try this 
patch out with the Eclipse test.  I also will include your lin-lwp linux event 
handler fix to see if that gets past the fork problem they were seeing.

> Index: thread-db.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/thread-db.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 thread-db.c
> --- thread-db.c	29 Feb 2004 02:39:47 -0000	1.37
> +++ thread-db.c	24 Mar 2004 16:47:23 -0000
> @@ -1,6 +1,6 @@
>  /* libthread_db assisted debugging support, generic parts.
>  
> -   Copyright 1999, 2000, 2001, 2003 Free Software Foundation, Inc.
> +   Copyright 1999, 2000, 2001, 2003, 2004 Free Software Foundation, Inc.
>  
>     This file is part of GDB.
>  
> @@ -130,6 +130,7 @@ static CORE_ADDR td_death_bp_addr;
>  static void thread_db_find_new_threads (void);
>  static void attach_thread (ptid_t ptid, const td_thrhandle_t *th_p,
>  			   const td_thrinfo_t *ti_p, int verbose);
> +static void detach_thread (ptid_t ptid, int verbose);
>  \f
>  
>  /* Building process ids.  */
> @@ -154,6 +155,8 @@ struct private_thread_info
>    unsigned int th_valid:1;
>    unsigned int ti_valid:1;
>  
> +  unsigned int dying:1;
> +
>    td_thrhandle_t th;
>    td_thrinfo_t ti;
>  };
> @@ -501,7 +504,7 @@ enable_thread_event_reporting (void)
>    /* Set the process wide mask saying which events we're interested in.  */
>    td_event_emptyset (&events);
>    td_event_addset (&events, TD_CREATE);
> -#if 0
> +#if 1
>    /* FIXME: kettenis/2000-04-23: The event reporting facility is
>       broken for TD_DEATH events in glibc 2.1.3, so don't enable it for
>       now.  */
> @@ -696,6 +699,20 @@ attach_thread (ptid_t ptid, const td_thr
>    struct thread_info *tp;
>    td_err_e err;
>  
> +	  /* We may already know about this thread, for instance when the
> +	     user has issued the `info threads' command before the SIGTRAP
> +	     for hitting the thread creation breakpoint was reported.  */
> +  if (in_thread_list (ptid))
> +    {
> +      tp = find_thread_pid (ptid);
> +      gdb_assert (tp != NULL);
> +
> +      if (!tp->private->dying)
> +        return;
> +
> +      delete_thread (ptid);
> +    }
> +
>    check_thread_signals ();
>  
>    /* Add the thread to GDB's thread list.  */
> @@ -741,8 +758,16 @@ thread_db_attach (char *args, int from_t
>  static void
>  detach_thread (ptid_t ptid, int verbose)
>  {
> +  struct thread_info *thread_info;
> +
>    if (verbose)
>      printf_unfiltered ("[%s exited]\n", target_pid_to_str (ptid));
> +
> +  /* Don't delete the thread now, because it still reports as active until
> +     it has executed a few instructions after the event breakpoint.  */
> +  thread_info = find_thread_pid (ptid);
> +  gdb_assert (thread_info != NULL);
> +  thread_info->private->dying = 1;
>  }
>  
>  static void
> @@ -848,11 +873,7 @@ check_event (ptid_t ptid)
>  	{
>  	case TD_CREATE:
>  
> -	  /* We may already know about this thread, for instance when the
> -	     user has issued the `info threads' command before the SIGTRAP
> -	     for hitting the thread creation breakpoint was reported.  */
> -	  if (!in_thread_list (ptid))
> -	    attach_thread (ptid, msg.th_p, &ti, 1);
> +	  attach_thread (ptid, msg.th_p, &ti, 1);
>  
>  	  break;
>  
> @@ -1121,8 +1142,7 @@ find_new_threads_callback (const td_thrh
>  
>    ptid = BUILD_THREAD (ti.ti_tid, GET_PID (inferior_ptid));
>  
> -  if (!in_thread_list (ptid))
> -    attach_thread (ptid, th_p, &ti, 1);
> +  attach_thread (ptid, th_p, &ti, 1);
>  
>    return 0;
>  }
> 
>