[PATCH v3 03/11] Windows GDB: make windows_thread_info be private thread_info data

[Pedro Alves <pedro@palves.net>]

With Windows non-stop support, we'll add support for
target_thread_events to the Windows target.

When that is supported, and the core wants to be notified of thread
exit events, the target backend does not delete the thread for which
the event is being reported.  Instead, infrun takes care of that.

That causes one problem on Windows, which is that Windows maintains
its own separate Windows threads list, in parallel with the struct
thread_info thread list maintained by the core.  In the
target_thread_events events scenario, when infrun deletes the thread,
the corresponding object in the Windows backend thread list is left
dangling, causing problems.

Fix this by eliminating the parallel thread list from the Windows
backend, instead making the windows_thread_info data by registered as
the private data associated with thread_info, like other targets do.

It also adds a all_windows_threads walker function, and associated
range and iterator classes, so that most of the Windows target code
can iterate over Windows threads without having to worry about
fetching the Windows thread data out of thread_info's private data.

Change-Id: I5058969b46e0dd238c89b6c28403c1848f083683
---
 gdb/target/waitstatus.h |  4 +-
 gdb/windows-nat.c       | 75 +++++++++++--------------------
 gdb/windows-nat.h       | 99 ++++++++++++++++++++++++++++++++++++++++-
 gdb/x86-windows-nat.c   |  4 +-
 4 files changed, 129 insertions(+), 53 deletions(-)

diff --git a/gdb/target/waitstatus.h b/gdb/target/waitstatus.h
index cbdaa026218..3a1adcc6256 100644
--- a/gdb/target/waitstatus.h
+++ b/gdb/target/waitstatus.h
@@ -105,7 +105,9 @@ enum target_waitkind
   /* The thread was created.  */
   TARGET_WAITKIND_THREAD_CREATED,
 
-  /* The thread has exited.  The exit status is in value.integer.  */
+  /* The thread has exited.  The exit status is in value.integer.  The
+     thread should still exist in the thread list when infrun receives
+     this event.  */
   TARGET_WAITKIND_THREAD_EXITED,
 };
 
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 1dfe2df389d..4029e7dad00 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -272,15 +272,23 @@ windows_nat_target::continue_last_debug_event_main_thread
   m_continued = !last_call;
 }
 
+/* Return a pointer to the windows-nat target instance.  */
+
+static windows_nat_target *
+get_windows_nat_target ()
+{
+  return gdb::checked_static_cast<windows_nat_target *> (get_native_target ());
+}
+
 /* See nat/windows-nat.h.  */
 
 windows_thread_info *
 windows_per_inferior::find_thread (ptid_t ptid)
 {
-  for (auto &th : thread_list)
-    if (th->tid == ptid.lwp ())
-      return th.get ();
-  return nullptr;
+  thread_info *thr = get_windows_nat_target ()->find_thread (ptid);
+  if (thr == nullptr)
+    return nullptr;
+  return as_windows_thread_info (thr);
 }
 
 /* See nat/windows-nat.h.  */
@@ -297,12 +305,11 @@ windows_thread_info *
 windows_nat_target::add_thread (ptid_t ptid, HANDLE h, void *tlb,
 				bool main_thread_p)
 {
-  windows_thread_info *th;
-
   gdb_assert (ptid.lwp () != 0);
 
-  if ((th = windows_process->find_thread (ptid)))
-    return th;
+  windows_thread_info *existing = windows_process->find_thread (ptid);
+  if (existing != nullptr)
+    return existing;
 
   CORE_ADDR base = (CORE_ADDR) (uintptr_t) tlb;
 #ifdef __x86_64__
@@ -311,18 +318,18 @@ windows_nat_target::add_thread (ptid_t ptid, HANDLE h, void *tlb,
   if (windows_process->wow64_process)
     base += 0x2000;
 #endif
-  th = new windows_thread_info (windows_process, ptid.lwp (), h, base);
-  windows_process->thread_list.emplace_back (th);
+  windows_private_thread_info *th
+    = new windows_private_thread_info (windows_process, ptid.lwp (), h, base);
 
   /* Add this new thread to the list of threads.
 
      To be consistent with what's done on other platforms, we add
      the main thread silently (in reality, this thread is really
      more of a process to the user than a thread).  */
-  if (main_thread_p)
-    add_thread_silent (this, ptid);
-  else
-    ::add_thread (this, ptid);
+  thread_info *gth = (main_thread_p
+		      ? ::add_thread_silent (this, ptid)
+		      : ::add_thread (this, ptid));
+  gth->priv.reset (th);
 
   /* It's simplest to always set this and update the debug
      registers.  */
@@ -331,15 +338,6 @@ windows_nat_target::add_thread (ptid_t ptid, HANDLE h, void *tlb,
   return th;
 }
 
-/* Clear out any old thread list and reinitialize it to a
-   pristine state.  */
-static void
-windows_init_thread_list (void)
-{
-  DEBUG_EVENTS ("called");
-  windows_process->thread_list.clear ();
-}
-
 /* Delete a thread from the list of threads.
 
    PTID is the ptid of the thread to be deleted.
@@ -351,12 +349,6 @@ void
 windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code,
 				   bool main_thread_p)
 {
-  DWORD id;
-
-  gdb_assert (ptid.lwp () != 0);
-
-  id = ptid.lwp ();
-
   /* Note that no notification was printed when the main thread was
      created, and thus, unless in verbose mode, we should be symmetrical,
      and avoid an exit notification for the main thread here as well.  */
@@ -364,16 +356,6 @@ windows_nat_target::delete_thread (ptid_t ptid, DWORD exit_code,
   bool silent = (main_thread_p && !info_verbose);
   thread_info *to_del = this->find_thread (ptid);
   delete_thread_with_exit_code (to_del, exit_code, silent);
-
-  auto iter = std::find_if (windows_process->thread_list.begin (),
-			    windows_process->thread_list.end (),
-			    [=] (std::unique_ptr<windows_thread_info> &th)
-			    {
-			      return th->tid == id;
-			    });
-
-  if (iter != windows_process->thread_list.end ())
-    windows_process->thread_list.erase (iter);
 }
 
 void
@@ -712,7 +694,7 @@ BOOL
 windows_nat_target::windows_continue (DWORD continue_status, int id,
 				      windows_continue_flags cont_flags)
 {
-  for (auto &th : windows_process->thread_list)
+  for (auto *th : all_windows_threads ())
     {
       if ((id == -1 || id == (int) th->tid)
 	  && !th->suspended
@@ -730,9 +712,9 @@ windows_nat_target::windows_continue (DWORD continue_status, int id,
 	}
     }
 
-  for (auto &th : windows_process->thread_list)
+  for (auto *th : all_windows_threads ())
     if (id == -1 || id == (int) th->tid)
-      continue_one_thread (th.get (), cont_flags);
+      continue_one_thread (th, cont_flags);
 
   continue_last_debug_event_main_thread
     (_("Failed to resume program execution"), continue_status,
@@ -914,7 +896,7 @@ windows_nat_target::get_windows_debug_event
   /* If there is a relevant pending stop, report it now.  See the
      comment by the definition of "windows_thread_info::pending_status"
      for details on why this is needed.  */
-  for (auto &th : windows_process->thread_list)
+  for (auto *th : all_windows_threads ())
     {
       if (!th->suspended
 	  && th->pending_status.kind () != TARGET_WAITKIND_IGNORE)
@@ -927,7 +909,7 @@ windows_nat_target::get_windows_debug_event
 	  *current_event = th->last_event;
 
 	  ptid_t ptid (windows_process->process_id, thread_id);
-	  windows_process->invalidate_thread_context (th.get ());
+	  windows_process->invalidate_thread_context (th);
 	  return ptid;
 	}
     }
@@ -1226,7 +1208,7 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 
 	      /* All-stop, suspend all threads until they are
 		 explicitly resumed.  */
-	      for (auto &thr : windows_process->thread_list)
+	      for (auto *thr : all_windows_threads ())
 		thr->suspend ();
 	    }
 
@@ -1382,7 +1364,6 @@ windows_nat_target::attach (const char *args, int from_tty)
     warning ("Failed to get SE_DEBUG_NAME privilege\n"
 	     "This can cause attach to fail on Windows NT/2K/XP");
 
-  windows_init_thread_list ();
   windows_process->saw_create = 0;
 
   std::optional<unsigned> err;
@@ -2142,7 +2123,6 @@ windows_nat_target::create_inferior (const char *exec_file,
 	}
     }
 
-  windows_init_thread_list ();
   do_synchronously ([&] ()
     {
       BOOL ok = create_process (nullptr, args, flags, w32_env,
@@ -2271,7 +2251,6 @@ windows_nat_target::create_inferior (const char *exec_file,
   /* Final nil string to terminate new env.  */
   *temp = 0;
 
-  windows_init_thread_list ();
   do_synchronously ([&] ()
     {
       BOOL ok = create_process (nullptr, /* image */
diff --git a/gdb/windows-nat.h b/gdb/windows-nat.h
index 235844a7c95..d63ef6e7b33 100644
--- a/gdb/windows-nat.h
+++ b/gdb/windows-nat.h
@@ -77,6 +77,38 @@ enum windows_continue_flag
 
 DEF_ENUM_FLAGS_TYPE (windows_continue_flag, windows_continue_flags);
 
+/* We want to register windows_thread_info as struct thread_info
+   private data.  thread_info::priv must point to a class that
+   inherits from private_thread_info.  But we can't make
+   windows_thread_info inherit private_thread_info, because
+   windows_thread_info is shared with GDBserver.  So we make a new
+   class that inherits from both private_thread_info,
+   windows_thread_info, and register that one as thread_info::private.
+   This multiple inheritance is benign, because private_thread_info is
+   a java-style interface class with no data.  */
+struct windows_private_thread_info : private_thread_info, windows_thread_info
+{
+  windows_private_thread_info (windows_nat::windows_process_info *proc,
+			       DWORD tid, HANDLE h, CORE_ADDR tlb)
+    : windows_thread_info (proc, tid, h, tlb)
+  {}
+
+  ~windows_private_thread_info () override
+  {}
+};
+
+/* Get the windows_thread_info object associated with THR.  */
+
+static inline windows_thread_info *
+as_windows_thread_info (thread_info *thr)
+{
+  /* Cast to windows_private_thread_info, which inherits from
+     private_thread_info, and is implicitly convertible to
+     windows_thread_info, the return type.  */
+  private_thread_info *priv = thr->priv.get ();
+  return gdb::checked_static_cast<windows_private_thread_info *> (priv);
+}
+
 struct windows_per_inferior : public windows_nat::windows_process_info
 {
   windows_thread_info *find_thread (ptid_t ptid) override;
@@ -91,8 +123,6 @@ struct windows_per_inferior : public windows_nat::windows_process_info
 
   int windows_initialization_done = 0;
 
-  std::vector<std::unique_ptr<windows_thread_info>> thread_list;
-
   /* Counts of things.  */
   int saw_create = 0;
   int open_process_used = 0;
@@ -352,4 +382,69 @@ int amd64_windows_segment_register_p (int regnum);
 extern const int amd64_mappings[];
 #endif
 
+/* Creates an iterator that works like all_matching_threads_iterator,
+   but that returns windows_thread_info pointers instead of
+   thread_info.  This could be replaced with a std::range::transform
+   when we require C++20.  */
+class all_windows_threads_iterator
+{
+public:
+  typedef all_windows_threads_iterator self_type;
+  typedef windows_thread_info value_type;
+  typedef windows_thread_info *&reference;
+  typedef windows_thread_info **pointer;
+  typedef std::forward_iterator_tag iterator_category;
+  typedef int difference_type;
+
+  explicit all_windows_threads_iterator (all_non_exited_threads_iterator base_iter)
+    : m_base_iter (base_iter)
+  {}
+
+  windows_thread_info * operator* () const { return as_windows_thread_info (&*m_base_iter); }
+
+  all_windows_threads_iterator &operator++ ()
+  {
+    ++m_base_iter;
+    return *this;
+  }
+
+  bool operator== (const all_windows_threads_iterator &other) const
+  { return m_base_iter == other.m_base_iter; }
+
+  bool operator!= (const all_windows_threads_iterator &other) const
+  { return !(*this == other); }
+
+private:
+  all_non_exited_threads_iterator m_base_iter;
+};
+
+/* The range for all_windows_threads, below.  */
+
+class all_windows_threads_range : public all_non_exited_threads_range
+{
+public:
+  all_windows_threads_range (all_non_exited_threads_range base_range)
+    : m_base_range (base_range)
+  {}
+
+  all_windows_threads_iterator begin () const
+  { return all_windows_threads_iterator (m_base_range.begin ()); }
+  all_windows_threads_iterator end () const
+  { return all_windows_threads_iterator (m_base_range.end ()); }
+
+private:
+  all_non_exited_threads_range m_base_range;
+};
+
+/* Return a range that can be used to walk over all non-exited Windows
+   threads of all inferiors, with range-for.  */
+
+static inline all_windows_threads_range
+all_windows_threads ()
+{
+  auto *win_tgt = static_cast<windows_nat_target *> (get_native_target ());
+  return (all_windows_threads_range
+	  (all_non_exited_threads_range (win_tgt, minus_one_ptid)));
+}
+
 #endif /* GDB_WINDOWS_NAT_H */
diff --git a/gdb/x86-windows-nat.c b/gdb/x86-windows-nat.c
index 5a4876b7fbe..ccabe80f9b4 100644
--- a/gdb/x86-windows-nat.c
+++ b/gdb/x86-windows-nat.c
@@ -346,7 +346,7 @@ windows_set_dr (int i, CORE_ADDR addr)
   if (i < 0 || i > 3)
     internal_error (_("Invalid register %d in windows_set_dr.\n"), i);
 
-  for (auto &th : x86_windows_process.thread_list)
+  for (auto *th : all_windows_threads ())
     th->debug_registers_changed = true;
 }
 
@@ -356,7 +356,7 @@ windows_set_dr (int i, CORE_ADDR addr)
 static void
 windows_set_dr7 (unsigned long val)
 {
-  for (auto &th : x86_windows_process.thread_list)
+  for (auto *th : all_windows_threads ())
     th->debug_registers_changed = true;
 }
 
-- 
2.53.0