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

[Pedro Alves <palves@redhat.com>]

I'm taking a look at adding the test mentioned below, and managed to
trigger an internal error:

src/gdb/inferior.c:66: internal-error: void set_current_inferior(inferior*): Assertion `inf != NULL' failed.
A problem internal to GDB has been detected,KFAIL: gdb.threads/threadapply.exp: kill_and_remove: try kill-and-remove: thread apply 1.1 kill-and-remove (GDB internal error) 

I'll poke a bit more at it.

Thanks,
Pedro Alves

On 04/06/2017 11:18 AM, Pedro Alves wrote:

> The rest of the review comments below could be addressed separately
> (by anyone, not necessarily you), I'm just putting them out as
> I thought of them.  I do think we should do it to avoid
> hard-to-debug corner cases around find_inferior_ptid finding
> an unrelated process that reused the old inferior's pid.
> 
>> -  if (tp
>> -      && find_inferior_ptid (tp->ptid) != NULL)
>> -    restore_current_thread (old->inferior_ptid);
>> +  /* If an entry of thread_info was previously selected, it won't be
>> +     deleted because we've increased its refcount.  The thread represented
>> +     by this thread_info entry may have already exited (due to normal exit,
>> +     detach, etc), so the thread_info.state is THREAD_EXITED.  */
>> +  if (old->thread != NULL
>> +      && find_inferior_ptid (old->thread->ptid) != NULL)
>> +    restore_current_thread (old->thread->ptid);
> 
> Note this was a look up by inferior ptid, not by (stable) inferior number,
> so we can genuinely find no inferior, even though the old inferior _object_
> will always be around when we get here, given that we mark it non-removable
> while the cleanup is installed [1].  Quite similar to increasing the
> thread's refcount, really.
> 
> So this predicate would probably be better as:
> 
>    if (old->thread != NULL
>       && old->thread != THREAD_EXITED
>       && find_inferior_id (old->inf_id)->pid != 0)
> 
> We could also store the inferior pointer in "old" directly,
> instead of the id, sparing the inferior look up:
> 
>    if (old->thread != NULL
>       && old->thread != THREAD_EXITED
>       && old->inf->pid != 0)
> 
> 
> [1] - We should probably have a test that does something like:
> 
> define kill-and-remove
>   kill inferiors 2
>   remove-inferiors 2
> end
> 
> # Start one inferior.
> start
> 
> # Start another inferior.
> add-inferior 2
> inferior 2
> start
> 
> # Kill and remove inferior 1 while thread 2.1 / inferior 2 is selected.
> thread apply 1.1 kill-and-remove