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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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