Re: [PATCH 00/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)

[Pedro Alves <palves@redhat.com>]

On 4/15/20 3:46 PM, Simon Marchi wrote:
> On 2020-04-14 1:54 p.m., Pedro Alves via Gdb-patches wrote:
>> In PR/25412, Simon noticed that after the multi-target series, the
>> tid-reuse.exp testcase manages to create a duplicate thread in the
>> thread list.  Or rather, two threads with the same PTID.
>>
>> This in turn exposes a design problem in GDB.  The inferior_thread()
>> function looks up the current thread based on inferior_ptid:
>>
>>  struct thread_info*
>>  inferior_thread (void)
>>  {
>>    struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
>>    gdb_assert (tp);
>>    return tp;
>>  }
>>
>> But if there can be more than one thread in the thread list with the
>> same ptid_t, inferior_thread() may well return the wrong thread.
> 
> I don't quite understand this part of the explanation.  Thread lists are
> per-inferior, and an inferior is specific to one target, and ptids are
> unique per-target.  So it's not possible at the moment (or at least not
> expected) to have two threads with the same ptid in one specific list.

The tid-reuse.exp testcase is about:

 # Test running a program that spawns enough threads that the tid of an
 # exited thread is reused.  GDB should not crash when this happens.

I.e., the testcase spawns enough short-lived threads that we end up
in add_thread_silent adding a new thread with the same ptid as
a preexisting thread, because the kernel tids eventually wrap around.
In this scenario, you know the preexisting thread with that ptid must
be gone, since otherwise the kernel would not be reusing the thread id.

But if that thread that is gone was the currently selected
thread, or somewhere throughout gdb something holds a reference
to it, then we can't delete that exiting thread immediately.  That's
how you end up with more than one thread with the same ptid in the
thread list.  Only one of those threads will be live, all other
"dups" will be exited threads.

So given that currently inferior_thread() does a look up by
ptid, it can happen that GDB meant to have the live thread
selected temporarily, say, but inferior_thread() returns the
exited thread. Like, this could fail:

switch_to_thread (exited_thread);
{
 scoped_restore_current_thread restore_thread;
 switch_to_thread (live_thread);
 gdb_assert (inferior_thread () == live_thread);
}

Note this happens with a single inferior.

Thanks,
Pedro Alves