[RFA]: Dead Thread Patch

[RFA]: Dead Thread Patch

[Jeff Johnston]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

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?

-- 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.


[-- Attachment #2: dead-thread-fix.patch --]
[-- Type: text/plain, Size: 3555 bytes --]

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

Re: [RFA]: Dead Thread Patch

[Michael Snyder]

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

Re: [RFA]: Dead Thread Patch

[Jeff Johnston]

Michael Snyder wrote:
> 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/)
> 

Thanks Michael.  Change made and patch checked in.

-- Jeff J.

>>
>> -- 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
> 
> 
> 
>