Decouple inferior_ptid/inferior_thread(); dup ptids in thread list (PR/25412)users/palves/current-thread

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_.

2 files changed, 42 insertions, 72 deletions

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 717a2ad08c2..6764c8fc498 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -433,12 +433,13 @@ extern struct thread_info *add_thread_with_info (process_stratum_target *targ,
 						 ptid_t ptid,
 						 private_thread_info *);
 
-/* Delete an existing thread list entry.  */
+/* Delete thread THREAD and notify of thread exit.  If the thread is
+   currently not deletable, don't actually delete it but still tag it
+   as exited and do the notification.  */
 extern void delete_thread (struct thread_info *thread);
 
-/* Delete an existing thread list entry, and be quiet about it.  Used
-   after the process this thread having belonged to having already
-   exited, for example.  */
+/* Like delete_thread, but be quiet about it.  Used when the process
+   this thread belonged to has already exited, for example.  */
 extern void delete_thread_silent (struct thread_info *thread);
 
 /* Delete a step_resume_breakpoint from the thread database.  */
@@ -478,15 +479,15 @@ extern bool in_thread_list (process_stratum_target *targ, ptid_t ptid);
    global id, not the system's).  */
 extern int valid_global_thread_id (int global_id);
 
-/* Find thread PTID of inferior INF.  */
+/* Find (non-exited) thread PTID of inferior INF.  */
 extern thread_info *find_thread_ptid (inferior *inf, ptid_t ptid);
 
-/* Search function to lookup a thread by 'pid'.  */
+/* Search function to lookup a (non-exited) thread by 'ptid'.  */
 extern struct thread_info *find_thread_ptid (process_stratum_target *targ,
 					     ptid_t ptid);
 
-/* Search function to lookup a thread by 'ptid'.  Only searches in
-   threads of INF.  */
+/* Search function to lookup a (non-exited) thread by 'ptid'.  Only
+   searches in threads of INF.  */
 extern struct thread_info *find_thread_ptid (inferior *inf, ptid_t ptid);
 
 /* Find thread by GDB global thread ID.  */
diff --git a/gdb/thread.c b/gdb/thread.c
index 02672f01fcf..f0722d35888 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -55,6 +55,9 @@
 
 static int highest_thread_num;
 
+/* The current/selected thread.  */
+static thread_info *current_thread_;
+
 /* RAII type used to increase / decrease the refcount of each thread
    in a given list of threads.  */
 
@@ -78,13 +81,19 @@ private:
   const std::vector<thread_info *> &m_thrds;
 };
 
+/* Returns true if THR is the current thread.  */
+
+static bool
+is_current_thread (const thread_info *thr)
+{
+  return thr == current_thread_;
+}
 
 struct thread_info*
 inferior_thread (void)
 {
-  struct thread_info *tp = find_thread_ptid (current_inferior (), inferior_ptid);
-  gdb_assert (tp);
-  return tp;
+  gdb_assert (current_thread_ != nullptr);
+  return current_thread_;
 }
 
 /* Delete the breakpoint pointed at by BP_P, if there's one.  */
@@ -194,7 +203,7 @@ clear_thread_inferior_resources (struct thread_info *tp)
 /* Set the TP's state as exited.  */
 
 static void
-set_thread_exited (thread_info *tp, int silent)
+set_thread_exited (thread_info *tp, bool silent)
 {
   /* Dead threads don't need to step-over.  Remove from queue.  */
   if (tp->step_over_next != NULL)
@@ -245,7 +254,12 @@ new_thread (struct inferior *inf, ptid_t ptid)
       struct thread_info *last;
 
       for (last = inf->thread_list; last->next != NULL; last = last->next)
-	;
+	gdb_assert (ptid != last->ptid
+		    || last->state == THREAD_EXITED);
+
+      gdb_assert (ptid != last->ptid
+		  || last->state == THREAD_EXITED);
+
       last->next = tp;
     }
 
@@ -255,51 +269,15 @@ new_thread (struct inferior *inf, ptid_t ptid)
 struct thread_info *
 add_thread_silent (process_stratum_target *targ, ptid_t ptid)
 {
-  inferior *inf;
-
-  thread_info *tp = find_thread_ptid (targ, ptid);
-  if (tp)
-    /* Found an old thread with the same id.  It has to be dead,
-       otherwise we wouldn't be adding a new thread with the same id.
-       The OS is reusing this id --- delete it, and recreate a new
-       one.  */
-    {
-      /* In addition to deleting the thread, if this is the current
-	 thread, then we need to take care that delete_thread doesn't
-	 really delete the thread if it is inferior_ptid.  Create a
-	 new template thread in the list with an invalid ptid, switch
-	 to it, delete the original thread, reset the new thread's
-	 ptid, and switch to it.  */
-
-      if (inferior_ptid == ptid)
-	{
-	  thread_info *new_thr = new_thread (tp->inf, null_ptid);
-
-	  /* Make switch_to_thread not read from the thread.  */
-	  new_thr->state = THREAD_EXITED;
-	  switch_to_no_thread ();
-
-	  /* Now we can delete it.  */
-	  delete_thread (tp);
-
-	  /* Now reset its ptid, and reswitch inferior_ptid to it.  */
-	  new_thr->ptid = ptid;
-	  new_thr->state = THREAD_STOPPED;
-	  switch_to_thread (new_thr);
-
-	  gdb::observers::new_thread.notify (new_thr);
-
-	  /* All done.  */
-	  return new_thr;
-	}
+  inferior *inf = find_inferior_ptid (targ, ptid);
 
-      inf = tp->inf;
-
-      /* Just go ahead and delete it.  */
-      delete_thread (tp);
-    }
-  else
-    inf = find_inferior_ptid (targ, ptid);
+  /* We may have an old thread with the same id in the thread list.
+     If we do, it must be dead, otherwise we wouldn't be adding a new
+     thread with the same id.  The OS is reusing this id --- delete
+     the old thread, and create a new one.  */
+  thread_info *tp = find_thread_ptid (inf, ptid);
+  if (tp != nullptr)
+    delete_thread (tp);
 
   tp = new_thread (inf, ptid);
   gdb::observers::new_thread.notify (tp);
@@ -349,14 +327,6 @@ thread_info::~thread_info ()
   xfree (this->name);
 }
 
-/* Returns true if THR is the current thread.  */
-
-static bool
-is_current_thread (const thread_info *thr)
-{
-  return thr->inf == current_inferior () && thr->ptid == inferior_ptid;
-}
-
 /* See gdbthread.h.  */
 
 bool
@@ -482,10 +452,7 @@ delete_thread_1 (thread_info *thr, bool silent)
   delete tp;
 }
 
-/* Delete thread THREAD and notify of thread exit.  If this is the
-   current thread, don't actually delete it, but tag it as exited and
-   do the notification.  If this is the user selected thread, clear
-   it.  */
+/* See gdbthread.h.  */
 
 void
 delete_thread (thread_info *thread)
@@ -535,7 +502,7 @@ find_thread_ptid (process_stratum_target *targ, ptid_t ptid)
 struct thread_info *
 find_thread_ptid (inferior *inf, ptid_t ptid)
 {
-  for (thread_info *tp : inf->threads ())
+  for (thread_info *tp : inf->non_exited_threads ())
     if (tp->ptid == ptid)
       return tp;
 
@@ -1317,7 +1284,8 @@ switch_to_thread_no_regs (struct thread_info *thread)
   set_current_program_space (inf->pspace);
   set_current_inferior (inf);
 
-  inferior_ptid = thread->ptid;
+  current_thread_ = thread;
+  inferior_ptid = current_thread_->ptid;
 }
 
 /* See gdbthread.h.  */
@@ -1325,9 +1293,10 @@ switch_to_thread_no_regs (struct thread_info *thread)
 void
 switch_to_no_thread ()
 {
-  if (inferior_ptid == null_ptid)
+  if (current_thread_ == nullptr)
     return;
 
+  current_thread_ = nullptr;
   inferior_ptid = null_ptid;
   reinit_frame_cache ();
 }