Re: [RFA]: Dead Thread Patch

[Michael Snyder <msnyder@redhat.com>]

Jeff Johnston wrote:
> The attached patch handles a problem I have run into while working on 
> threaded watchpoints.  What happens is that a thread may die and we have 
> other events occurring at the same time (e.g. watchpoints).  If the 
> thread_alive() function gets run by a gdb interface in the meantime, the 
> thread will be removed on the thread list.  What later happens is that 
> we get the death event to handle and we see a tid that is not found on 
> the thread list.  We think we have discovered a new thread and add it.  
> This causes all kinds of problems in the future if we attempt to get 
> registers or switch to the dead thread, etc.
> 
> My patch attempts to recognize the situation and avoid any processing of 
> the dead thread event.  I have tested this with the testsuite on i686 to 
> verify there are no regressions.
> 
> Daniel, I cc'd you in case this fixes some of the occasional random 
> thread errors you were seeing and couldn't explain.
> 
> Ok to commit?

Looks good to me (s/prior/previously/)

> 
> -- Jeff J.
> 
> 2004-06-04  Jeff Johnston  <jjohnstn@redhat.com>
> 
>         * infrun.c (handle_inferior_event): Don't treat an invalid ptid
>         as a new thread event.
>         * thread_db.c (thread_get_info_callback): If the thread is a
>         zombie, return TD_THR_ZOMBIE.
>         * (thread_from_lwp): If thread_get_info_callback returns
>         TD_THR_ZOMBIE, check if the thread is still on the thread list
>         and return a -1 ptid if not found.
>         (thread_db_wait): If thread_from_lwp returns a -1 ptid, then
>         change the status to TARGET_WAITKIND_SPURIOUS.
> 
> 
> ------------------------------------------------------------------------
> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.163
> diff -u -p -r1.163 infrun.c
> --- infrun.c	14 May 2004 18:45:42 -0000	1.163
> +++ infrun.c	4 Jun 2004 18:13:53 -0000
> @@ -1332,6 +1332,7 @@ handle_inferior_event (struct execution_
>    /* If it's a new process, add it to the thread database */
>  
>    ecs->new_thread_event = (!ptid_equal (ecs->ptid, inferior_ptid)
> +			   && !ptid_equal (ecs->ptid, minus_one_ptid)
>  			   && !in_thread_list (ecs->ptid));
>  
>    if (ecs->ws.kind != TARGET_WAITKIND_EXITED
> Index: thread-db.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/thread-db.c,v
> retrieving revision 1.40
> diff -u -p -r1.40 thread-db.c
> --- thread-db.c	25 May 2004 14:58:31 -0000	1.40
> +++ thread-db.c	4 Jun 2004 18:13:53 -0000
> @@ -252,7 +252,10 @@ thread_db_state_str (td_thr_state_e stat
>  
>     THP is a handle to the current thread; if INFOP is not NULL, the
>     struct thread_info associated with this thread is returned in
> -   *INFOP.  */
> +   *INFOP.
> +
> +   If the thread is a zombie, TD_THR_ZOMBIE is returned.  Otherwise,
> +   zero is returned to indicate success.  */
>  
>  static int
>  thread_get_info_callback (const td_thrhandle_t *thp, void *infop)
> @@ -271,6 +274,16 @@ thread_get_info_callback (const td_thrha
>    thread_ptid = BUILD_THREAD (ti.ti_tid, GET_PID (inferior_ptid));
>    thread_info = find_thread_pid (thread_ptid);
>  
> +  /* In the case of a zombie thread, don't continue.  We don't want to
> +     attach to it thinking it is a new thread and we don't want to mark
> +     it as valid.  */
> +  if (ti.ti_state == TD_THR_UNKNOWN || ti.ti_state == TD_THR_ZOMBIE)
> +    {
> +      if (infop != NULL)
> +        *(struct thread_info **) infop = thread_info;
> +      return TD_THR_ZOMBIE;
> +    }
> +
>    if (thread_info == NULL)
>      {
>        /* New thread.  Attach to it now (why wait?).  */
> @@ -355,7 +368,19 @@ thread_from_lwp (ptid_t ptid)
>  	   GET_LWP (ptid), thread_db_err_str (err));
>  
>    thread_info = NULL;
> -  thread_get_info_callback (&th, &thread_info);
> +
> +  /* Fetch the thread info.  If we get back TD_THR_ZOMBIE, then the
> +     event thread has already died.  If another gdb interface has called
> +     thread_alive() prior, the thread won't be found on the thread list
> +     anymore.  In that case, we don't want to process this ptid anymore
> +     to avoid the possibility of later treating it as a newly
> +     discovered thread id that we should add to the list.  Thus,
> +     we return a -1 ptid which is also how the thread list marks a
> +     dead thread.  */
> +  if (thread_get_info_callback (&th, &thread_info) == TD_THR_ZOMBIE
> +      && thread_info == NULL)
> +    return pid_to_ptid (-1);
> +
>    gdb_assert (thread_info && thread_info->private->ti_valid);
>  
>    return BUILD_THREAD (thread_info->private->ti.ti_tid, GET_PID (ptid));
> @@ -950,7 +975,16 @@ thread_db_wait (ptid_t ptid, struct targ
>    if (!ptid_equal (trap_ptid, null_ptid))
>      trap_ptid = thread_from_lwp (trap_ptid);
>  
> -  return thread_from_lwp (ptid);
> +  /* Change the ptid back into the higher level PID + TID format.
> +     If the thread is dead and no longer on the thread list, we will 
> +     get back a dead ptid.  This can occur if the thread death event
> +     gets postponed by other simultaneous events.  In such a case, 
> +     we want to just ignore the event and continue on.  */
> +  ptid = thread_from_lwp (ptid);
> +  if (GET_PID (ptid) == -1)
> +    ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
> +  
> +  return ptid;
>  }
>  
>  static int