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

Simon Marchi
2020-04-15T15:42:45Z

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