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

[Simon Marchi <simark@simark.ca>]

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.

I am having trouble connecting the dots between this explanation and the
problem originally reported, which IIRC correctly involves a single
inferior.

> This series fixes this by making the current thread be a global
> thread_info pointer that is written to directly by switch_to_thread,
> etc., and making inferior_thread() return that pointer, instead of
> having inferior_thread() lookup up the inferior_ptid thread, by
> ptid_t.  You can look at this as a continuation of the effort of using
> more thread_info pointers instead of ptids when possible.

Even though I don't understand the explanation above, I think this is
a good step forward.

> This change required auditing the whole codebase for places where we
> were writing to inferior_ptid directly to change the current thread,
> and change them to use switch_to_thread instead or one of its
> siblings, because otherwise inferior_thread() and inferior_ptid would
> get out of sync and inferior_thread() would return a thread unrelated
> to the new inferior_ptid we wanted to switch to.  That was all
> (hopefully) done in all the patches leading up to the last one.
> 
> After this, inferior_ptid still exists, but it is mostly read-only and
> mainly used by target backend code.  It is also relied on by a number
> of target methods as a global input argument.  E.g., the target_resume
> interface and the memory reading routines -- we still need it there
> because we need to be able to access memory off of processes for which
> we don't have a corresponding inferior/thread object, like when
> handling forks.  Maybe we could pass down a context explicitly to
> target_read_memory, etc.

I would like to do that eventually.

> Most of the host-/native-specific code here is untested.  I did my
> best, but I won't be surprised if more tweaking is necessary.
> 
> Testing on native x86_64 GNU/Linux is regression free for me.  Testing
> against gdbserver has regressed significantly in the past months and
> is becoming difficult to run with a high number of long timeout
> sequences; really looks like people aren't paying much attention to
> that.  I think this series doesn't regress gdbserver, but it's getting
> hard to tell.  :-/

Oops... we'll have to do some bisecting.

Simon