From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id E91113959C94; Thu, 18 Jun 2020 22:27:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E91113959C94 From: "cvs-commit at gcc dot gnu.org" To: gdb-prs@sourceware.org Subject: [Bug gdb/25412] thread_info with duplicate ptid added to inferior thread list Date: Thu, 18 Jun 2020 22:27:59 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gdb X-Bugzilla-Component: gdb X-Bugzilla-Version: HEAD X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: cvs-commit at gcc dot gnu.org X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: unassigned at sourceware dot org X-Bugzilla-Target-Milestone: 10.1 X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://sourceware.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-BeenThere: gdb-prs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-prs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Jun 2020 22:28:00 -0000 https://sourceware.org/bugzilla/show_bug.cgi?id=3D25412 --- Comment #2 from cvs-commit at gcc dot gnu.org --- The master branch has been updated by Pedro Alves : https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D3922b302645f= da04da42a5279399578ae2f6206c commit 3922b302645fda04da42a5279399578ae2f6206c Author: Pedro Alves 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 =3D 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 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 =3D=3D -1 = and tid =3D=3D 'the thread's global GDB thread number'. Note that GDB assu= mes 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 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_. --=20 You are receiving this mail because: You are on the CC list for the bug.=