[PATCH 28/28] Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)
Andrew Burgess
andrew.burgess@embecosm.com
Tue Jun 23 13:37:50 GMT 2020
* Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> [2020-04-14 18:54:34 +0100]:
> 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:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * 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_.
> ---
> gdb/gdbthread.h | 17 +++++-----
> gdb/thread.c | 97 ++++++++++++++++++++-------------------------------------
> 2 files changed, 42 insertions(+), 72 deletions(-)
This commit causes a regression for gdb.gdb/unittest.exp when GDB is
configured with --enable-targets=all.
The failure is:
gdb/thread.c:95: internal-error: thread_info* inferior_thread(): Assertion `current_thread_ != nullptr' failed.
Here's a partial backtrace:
#8 0x00000000013e4d0f in internal_error (file=0x16c55c7 "../../src.dev-1/gdb/thread.c", line=95,
fmt=0x16c54d9 "%s: Assertion `%s' failed.") at ../../src.dev-1/gdbsupport/errors.cc:55
#9 0x0000000000c34e63 in inferior_thread () at ../../src.dev-1/gdb/thread.c:95
#10 0x0000000000a73d62 in get_current_regcache () at ../../src.dev-1/gdb/regcache.c:391
#11 0x00000000009051c0 in current_options () at ../../src.dev-1/gdb/mep-tdep.c:873
#12 0x00000000009052eb in mep_register_name (gdbarch=0x33862a0, regnr=152) at ../../src.dev-1/gdb/mep-tdep.c:958
#13 0x00000000009054aa in mep_register_reggroup_p (gdbarch=0x33862a0, regnum=152, group=0x26334c0 <save_group>)
at ../../src.dev-1/gdb/mep-tdep.c:1029
#14 0x00000000007975e8 in gdbarch_register_reggroup_p (gdbarch=0x33862a0, regnum=152, reggroup=0x26334c0 <save_group>)
at ../../src.dev-1/gdb/gdbarch.c:3595
#15 0x0000000000a7355a in reg_buffer::save(gdb::function_view<register_status (int, unsigned char*)>) (
this=0x7fffffffad00, cooked_read=...) at ../../src.dev-1/gdb/regcache.c:248
#16 0x0000000000780b36 in readonly_detached_regcache::readonly_detached_regcache(gdbarch*, gdb::function_view<register_status (int, unsigned char*)>) (this=0x7fffffffad00, gdbarch=0x33862a0, cooked_read=...)
at ../../src.dev-1/gdb/regcache.h:455
#17 0x0000000000a73409 in readonly_detached_regcache::readonly_detached_regcache (this=0x7fffffffad00, src=...)
at ../../src.dev-1/gdb/regcache.c:213
#18 0x0000000000a773b5 in selftests::cooked_read_test (gdbarch=0x33862a0) at ../../src.dev-1/gdb/regcache.c:1690
#19 0x0000000000b48c29 in selftests::gdbarch_selftest::operator() (this=0x320fef0)
at ../../src.dev-1/gdb/selftest-arch.c:73
#20 0x00000000013fe5ac in selftests::run_tests (filter=0x0) at ../../src.dev-1/gdbsupport/selftest.cc:88
#21 0x00000000008e78ba in maintenance_selftest (args=0x0, from_tty=1) at ../../src.dev-1/gdb/maint.c:1044
#22 0x00000000005b0557 in do_const_cfunc (c=0x32004f0, args=0x0, from_tty=1)
at ../../src.dev-1/gdb/cli/cli-decode.c:95
#23 0x00000000005b3839 in cmd_func (cmd=0x32004f0, args=0x0, from_tty=1) at ../../src.dev-1/gdb/cli/cli-decode.c:2113
#24 0x0000000000c4953b in execute_command (p=0x2924895 "", from_tty=1) at ../../src.dev-1/gdb/top.c:655
#25 0x0000000000759c26 in command_handler (command=0x2924880 "maintenance selftest ")
at ../../src.dev-1/gdb/event-top.c:588
#26 0x000000000075a052 in command_line_handler (rl=...) at ../../src.dev-1/gdb/event-top.c:773
I suspect that the problem might be this line in regcache.c:cooked_read_test:
/* Switch to the mock thread. */
scoped_restore restore_inferior_ptid
= make_scoped_restore (&inferior_ptid, mock_ptid);
My suspicion from a quick read of your patch above is that we need to
do more than just set inferior_ptid here now.
Thanks,
Andrew
More information about the Gdb-patches
mailing list