[Bug gdb/25412] thread_info with duplicate ptid added to inferior thread list

[Bug gdb/25412] thread_info with duplicate ptid added to inferior thread list

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=25412

--- Comment #1 from Simon Marchi <simark at simark dot ca> ---
Proposed patch series by Pedro:

https://sourceware.org/pipermail/gdb-patches/2020-April/167578.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25412] thread_info with duplicate ptid added to inferior thread list

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=25412

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Pedro Alves <palves@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3922b302645fda04da42a5279399578ae2f6206c

commit 3922b302645fda04da42a5279399578ae2f6206c
Author: Pedro Alves <palves@redhat.com>
Date:   Thu Jun 18 21:28:37 2020 +0100

    Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR
25412)

    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.

    add_thread_silent has code in place to detect the case of a new thread
    reusing some older thread's ptid, but it doesn't work correctly
    anymore when the old thread is NOT the current thread and it has a
    refcount higher than 0.  Either condition prevents a thread from being
    deleted, but the refcount case wasn't being considered.  I think the
    reason that case wasn't considered is that that code predates
    thread_info refcounting.  Back when it was originally written,
    delete_thread always deleted the thread.

    That add_thread_silent code in question has some now-unnecessary
    warts, BTW.  For instance, this:

      /* Make switch_to_thread not read from the thread.  */
      new_thr->state = THREAD_EXITED;

    ... used to be required because switch_to_thread would update
    'stop_pc' otherwise.  I.e., it would read registers from an exited
    thread otherwise.  switch_to_thread no longer reads the stop_pc, since:

      commit f2ffa92bbce9dd5fbedc138ac2a3bc8a88327d09
      Author:     Pedro Alves <palves@redhat.com>
      AuthorDate: Thu Jun 28 20:18:24 2018 +0100

          gdb: Eliminate the 'stop_pc' global

    Also, if the ptid of the now-gone current thread is reused, we
    currently return from add_thread_silent with the current thread
    pointing at the _new_ thread.  Either pointing at the old thread, or
    at no thread selected would be reasonable.  But pointing at an
    unrelated thread (the new thread that happens to reuse the ptid) is
    just broken.  Seems like I was the one who wrote it like that but I
    have no clue why, FWIW.

    Currently, an exited thread kept in the thread list still holds its
    original ptid.  The idea was that we need the ptid to be able to
    temporarily switch to another thread and then switch back to the
    original thread, because thread switching is really inferior_ptid
    switching.  Switching back to the original thread requires a ptid
    lookup.

    Now, in order to avoid exited threads with the same ptid as a live
    thread in the same thread list, one thing I considered (and tried) was
    to change an exited thread's ptid to minus_one_ptid.  However, with
    that, there's a case that we won't handle well, which is if we end up
    with more than one exited thread in the list, since then all exited
    threads will all have the same ptid.  Since inferior_thread() relies
    on inferior_ptid, may well return the wrong thread.

    My next attempt to address this, was to switch an exited thread's ptid
    to a globally unique "exited" ptid, which is a ptid with pid == -1 and
    tid == 'the thread's global GDB thread number'.  Note that GDB assumes
    that the GDB global thread number is monotonically increasing and
    doesn't wrap around.  (We should probably make GDB thread numbers
    64-bit to prevent that happening in practice; they're currently signed
    32-bit.)  This attempt went a long way, but still ran into a number of
    issues.  It was a major hack too, obviously.

    My next attempt is the one that I'm proposing, which is to bite the
    bullet and break the connection between inferior_ptid and
    inferior_thread(), aka the current thread.  I.e., make 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.

    By making the current thread a global thread_info pointer, we can make
    switch_to_thread simply write to the global thread pointer, which
    makes scoped_restore_current_thread able to restore back to an exited
    thread without relying on unrelyable ptid look ups.  I.e., this makes
    it not a real problem to have more than one thread with the same ptid
    in the thread list.  There will always be only one live thread with a
    given ptid, so code that looks up a live thread by ptid will always be
    able to find the right one.

    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() would return a thread
    unrelated to the changed-to inferior_ptid.  That was all (hopefully)
    done in previous patches.

    After this, inferior_ptid is mainly used by target backend code.  It
    is also relied on by a number of target methods.  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.

    gdb/ChangeLog:
    2020-06-18  Pedro Alves  <palves@redhat.com>

            PR gdb/25412
            * gdbthread.h (delete_thread, delete_thread_silent)
            (find_thread_ptid): Update comments.
            * thread.c (current_thread_): New global.
            (is_current_thread): Move higher, and reimplement.
            (inferior_thread): Reimplement.
            (set_thread_exited): Use bool.  Add assertions.
            (add_thread_silent): Simplify thread-reuse handling by always
            calling delete_thread.
            (delete_thread): Remove intro comment.
            (find_thread_ptid): Skip exited threads.
            (switch_to_thread_no_regs): Write to current_thread_.
            (switch_to_no_thread): Check CURRENT_THREAD_ instead of
            INFERIOR_PTID.  Clear current_thread_.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/25412] thread_info with duplicate ptid added to inferior thread list

[palves at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=25412

Pedro Alves <palves at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |palves at redhat dot com
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #3 from Pedro Alves <palves at redhat dot com> ---
Fix merged.

-- 
You are receiving this mail because:
You are on the CC list for the bug.