Re: [PATCH 2/2] Don't delete thread_info if refcount isn't zero

[Pedro Alves <palves at redhat dot com>]

On 04/07/2017 10:22 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>>> -  tp = find_thread_ptid (old->inferior_ptid);
>>> -
>>> -  /* If the previously selected thread belonged to a process that has
>>> -     in the mean time been deleted (due to normal exit, detach, etc.),
>>> -     then don't revert back to it, but instead simply drop back to no
>>> -     thread selected.  */
>>
>> This comment still makes sense, with a small tweak -- saying "deleted"
>> has always been a bit misleading here:
>>
>>   /* If the previously selected thread belonged to a process that has
>>      in the mean time exited (or killed, detached, etc.), then don't revert
>>      back to it, but instead simply drop back to no thread selected.  */
>>
>> I'll be happy with restoring this comment alongside your new comment.
> 
> The comments describe something different from what the code does.  

It doesn't.  See below.

> With
> my patch, in make_cleanup_restore_current_thread, if we can find
> thread_info by inferior_ptid, saved it in old->thread and increase the
> refcount, so that it won't be deleted, but it can be marked as exited.
> Then, in do_restore_current_thread_cleanup, 
> 
> +  if (old->thread != NULL
> +      && find_inferior_ptid (old->thread->ptid) != NULL)
> +    restore_current_thread (old->thread->ptid);
>    else
>      {
>        restore_current_thread (null_ptid);
> 
> the first line means we previously selected one existing thread, we
> still switch to that thread, which may be already marked as exited.  It
> is nothing wrong as far as I can see.

I'm not sure what you mean by "wrong".  I'm saying that the old comment
still makes sense.  That comment is talking about the 
"find_inferior_ptid (old->thread->ptid) != NULL" line, which is a 
lookup for an _inferior_ not a thread.  Both the inferior and thread_info
object are still around, but the process may have exited/been detached
meanwhile, and consequently the inferior's "pid" field is now
zero.  And in that case, we don't restore back the selected thread.

E.g., with:

 start
 thread apply all detach

#1 - the current thread (thread 1), has its refcount incremented.
#2 - detach gets rid of all threads of the process.  The "thread 1"
     object is left around, marked exited, because it was not
     deletable.
#3 - when we try to restore the previous thread, its
     _inferior_ has meanwhile gotten its "pid" field zeroed,
     so a look up for an inferior with the pid of the previous
     thread's "ptid" (which just looks at the pid) won't
     find any inferior.  (Unless it finds the wrong inferior
     that happened to reuse the pid meanwhile.)

Hopefully that makes the rest of my comments in my previous
message clearer.

Thanks,
Pedro Alves