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

[Simon Marchi <simark@simark.ca>]

On 2020-04-15 11:33 a.m., Pedro Alves wrote:
> 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
> 

Ok thanks, that clarifies it.

Simon