[PATCH 0/3] Make lwp_info use intrusive_list

[PATCH 0/3] Make lwp_info use intrusive_list

[Simon Marchi]

The goal of this series is to replace the manually maintained linked
list of lwp_info objects with an intrusive_list.  No user-visible
changes expected, no regressions seen on Linux / x86-64.

Simon Marchi (3):
  gdb: make lwp_info non-POD
  gdb: add destructor to lwp_info
  gdb: use intrusive_list for linux-nat lwp_list

 gdb/ia64-linux-nat.c  |  8 ++---
 gdb/linux-nat.c       | 84 ++++++++++++++++++-------------------------
 gdb/linux-nat.h       | 73 ++++++++++++++++++++-----------------
 gdb/linux-thread-db.c |  3 +-
 gdb/mips-linux-nat.c  |  7 ++--
 5 files changed, 80 insertions(+), 95 deletions(-)

-- 
2.33.0

[PATCH 1/3] gdb: make lwp_info non-POD

[Simon Marchi]

Initialize all fields in the class declaration directly.  This opens the
door to using intrusive_list, done in the following patch.

Change-Id: I38bb27410cd9ebf511d310bb86fe2ea1872c3b05
---
 gdb/linux-nat.c | 27 ++++++---------------------
 gdb/linux-nat.h | 38 ++++++++++++++++++++++----------------
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index e9433b2206bc..26324af3cfa2 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -845,19 +845,10 @@ purge_lwp_list (int pid)
 static struct lwp_info *
 add_initial_lwp (ptid_t ptid)
 {
-  struct lwp_info *lp;
-
   gdb_assert (ptid.lwp_p ());
 
-  lp = XNEW (struct lwp_info);
-
-  memset (lp, 0, sizeof (struct lwp_info));
-
-  lp->last_resume_kind = resume_continue;
-  lp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
+  lwp_info *lp = new lwp_info (ptid);
 
-  lp->ptid = ptid;
-  lp->core = -1;
 
   /* Add to sorted-by-reverse-creation-order list.  */
   lwp_list_add (lp);
@@ -893,16 +884,13 @@ add_lwp (ptid_t ptid)
 static void
 delete_lwp (ptid_t ptid)
 {
-  struct lwp_info *lp;
-  void **slot;
-  struct lwp_info dummy;
+  lwp_info dummy (ptid);
 
-  dummy.ptid = ptid;
-  slot = htab_find_slot (lwp_lwpid_htab, &dummy, NO_INSERT);
+  void **slot = htab_find_slot (lwp_lwpid_htab, &dummy, NO_INSERT);
   if (slot == NULL)
     return;
 
-  lp = *(struct lwp_info **) slot;
+  lwp_info *lp = *(struct lwp_info **) slot;
   gdb_assert (lp != NULL);
 
   htab_clear_slot (lwp_lwpid_htab, slot);
@@ -920,18 +908,15 @@ delete_lwp (ptid_t ptid)
 static struct lwp_info *
 find_lwp_pid (ptid_t ptid)
 {
-  struct lwp_info *lp;
   int lwp;
-  struct lwp_info dummy;
 
   if (ptid.lwp_p ())
     lwp = ptid.lwp ();
   else
     lwp = ptid.pid ();
 
-  dummy.ptid = ptid_t (0, lwp, 0);
-  lp = (struct lwp_info *) htab_find (lwp_lwpid_htab, &dummy);
-  return lp;
+  lwp_info dummy (ptid_t (0, lwp, 0));
+  return (struct lwp_info *) htab_find (lwp_lwpid_htab, &dummy);
 }
 
 /* See nat/linux-nat.h.  */
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 83bd6d4a6786..050b8046da48 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -202,20 +202,26 @@ struct arch_lwp_info;
 
 struct lwp_info
 {
+  lwp_info (ptid_t ptid)
+    : ptid (ptid)
+  {
+    waitstatus.kind = TARGET_WAITKIND_IGNORE;
+  }
+
   /* The process id of the LWP.  This is a combination of the LWP id
      and overall process id.  */
   ptid_t ptid;
 
   /* If this flag is set, we need to set the event request flags the
      next time we see this LWP stop.  */
-  int must_set_ptrace_flags;
+  int must_set_ptrace_flags = 0;
 
   /* Non-zero if we sent this LWP a SIGSTOP (but the LWP didn't report
      it back yet).  */
-  int signalled;
+  int signalled = 0;
 
   /* Non-zero if this LWP is stopped.  */
-  int stopped;
+  int stopped = 0;
 
   /* Non-zero if this LWP will be/has been resumed.  Note that an LWP
      can be marked both as stopped and resumed at the same time.  This
@@ -223,38 +229,38 @@ struct lwp_info
      pending.  We shouldn't let the LWP run until that wait status has
      been processed, but we should not report that wait status if GDB
      didn't try to let the LWP run.  */
-  int resumed;
+  int resumed = 0;
 
   /* The last resume GDB requested on this thread.  */
-  enum resume_kind last_resume_kind;
+  resume_kind last_resume_kind = resume_continue;
 
   /* If non-zero, a pending wait status.  */
-  int status;
+  int status = 0;
 
   /* When 'stopped' is set, this is where the lwp last stopped, with
      decr_pc_after_break already accounted for.  If the LWP is
      running and stepping, this is the address at which the lwp was
      resumed (that is, it's the previous stop PC).  If the LWP is
      running and not stepping, this is 0.  */
-  CORE_ADDR stop_pc;
+  CORE_ADDR stop_pc = 0;
 
   /* Non-zero if we were stepping this LWP.  */
-  int step;
+  int step = 0;
 
   /* The reason the LWP last stopped, if we need to track it
      (breakpoint, watchpoint, etc.).  */
-  enum target_stop_reason stop_reason;
+  target_stop_reason stop_reason = TARGET_STOPPED_BY_NO_REASON;
 
   /* On architectures where it is possible to know the data address of
      a triggered watchpoint, STOPPED_DATA_ADDRESS_P is non-zero, and
      STOPPED_DATA_ADDRESS contains such data address.  Otherwise,
      STOPPED_DATA_ADDRESS_P is false, and STOPPED_DATA_ADDRESS is
      undefined.  Only valid if STOPPED_BY_WATCHPOINT is true.  */
-  int stopped_data_address_p;
-  CORE_ADDR stopped_data_address;
+  int stopped_data_address_p = 0;
+  CORE_ADDR stopped_data_address = 0;
 
   /* Non-zero if we expect a duplicated SIGINT.  */
-  int ignore_sigint;
+  int ignore_sigint = 0;
 
   /* If WAITSTATUS->KIND != TARGET_WAITKIND_SPURIOUS, the waitstatus
      for this LWP's last event.  This may correspond to STATUS above,
@@ -269,15 +275,15 @@ struct lwp_info
   enum target_waitkind syscall_state;
 
   /* The processor core this LWP was last seen on.  */
-  int core;
+  int core = -1;
 
   /* Arch-specific additions.  */
-  struct arch_lwp_info *arch_private;
+  struct arch_lwp_info *arch_private = nullptr;
 
   /* Previous and next pointers in doubly-linked list of known LWPs,
      sorted by reverse creation order.  */
-  struct lwp_info *prev;
-  struct lwp_info *next;
+  struct lwp_info *prev = nullptr;
+  struct lwp_info *next = nullptr;
 };
 
 /* The global list of LWPs, for ALL_LWPS.  Unlike the threads list,
-- 
2.33.0

[PATCH 2/3] gdb: add destructor to lwp_info

[Simon Marchi]

Replace the lwp_free function with a destructor.  Make lwp_info
non-copyable, since there is now a destructor (we wouldn't want an
lwp_info object getting copied and this->arch_private getting deleted
twice).

Change-Id: I09fcbe967e362566d3a06fed2abca2a9955570fa
---
 gdb/linux-nat.c | 11 ++++-------
 gdb/linux-nat.h |  4 ++++
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 26324af3cfa2..930209db6e89 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -793,13 +793,10 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp);
 
 /* Destroy and free LP.  */
 
-static void
-lwp_free (struct lwp_info *lp)
+lwp_info::~lwp_info ()
 {
   /* Let the arch specific bits release arch_lwp_info.  */
-  linux_target->low_delete_thread (lp->arch_private);
-
-  xfree (lp);
+  linux_target->low_delete_thread (this->arch_private);
 }
 
 /* Traversal function for purge_lwp_list.  */
@@ -814,7 +811,7 @@ lwp_lwpid_htab_remove_pid (void **slot, void *info)
     {
       htab_clear_slot (lwp_lwpid_htab, slot);
       lwp_list_remove (lp);
-      lwp_free (lp);
+      delete lp;
     }
 
   return 1;
@@ -899,7 +896,7 @@ delete_lwp (ptid_t ptid)
   lwp_list_remove (lp);
 
   /* Release.  */
-  lwp_free (lp);
+  delete lp;
 }
 
 /* Return a pointer to the structure describing the LWP corresponding
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 050b8046da48..b1b168d2bfee 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -208,6 +208,10 @@ struct lwp_info
     waitstatus.kind = TARGET_WAITKIND_IGNORE;
   }
 
+  ~lwp_info ();
+
+  DISABLE_COPY_AND_ASSIGN (lwp_info);
+
   /* The process id of the LWP.  This is a combination of the LWP id
      and overall process id.  */
   ptid_t ptid;
-- 
2.33.0

[PATCH 3/3] gdb: use intrusive_list for linux-nat lwp_list

[Simon Marchi]

Replace the manually maintained linked list of lwp_info objects with
intrusive_list.  Replace the ALL_LWPS macro with all_lwps, which returns
a range.  Add all_lwps_safe as well, for use in iterate_over_lwps, which
currently iterates in a safe manner.

Change-Id: I355313502510acc0103f5eaf2fbde80897d6376c
---
 gdb/ia64-linux-nat.c  |  8 +++-----
 gdb/linux-nat.c       | 46 ++++++++++++++++++++++---------------------
 gdb/linux-nat.h       | 35 +++++++++++++++-----------------
 gdb/linux-thread-db.c |  3 +--
 gdb/mips-linux-nat.c  |  7 ++-----
 5 files changed, 46 insertions(+), 53 deletions(-)

diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c
index 6381e5318e68..371d51f00bba 100644
--- a/gdb/ia64-linux-nat.c
+++ b/gdb/ia64-linux-nat.c
@@ -589,7 +589,6 @@ ia64_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
 					  enum target_hw_bp_type type,
 					  struct expression *cond)
 {
-  struct lwp_info *lp;
   int idx;
   long dbr_addr, dbr_mask;
   int max_watchpoints = 4;
@@ -630,7 +629,8 @@ ia64_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
 
   debug_registers[2 * idx] = dbr_addr;
   debug_registers[2 * idx + 1] = dbr_mask;
-  ALL_LWPS (lp)
+
+  for (const lwp_info *lp : all_lwps ())
     {
       store_debug_register_pair (lp->ptid, idx, &dbr_addr, &dbr_mask);
       enable_watchpoints_in_psr (lp->ptid);
@@ -657,14 +657,12 @@ ia64_linux_nat_target::remove_watchpoint (CORE_ADDR addr, int len,
       dbr_mask = debug_registers[2 * idx + 1];
       if ((dbr_mask & (0x3UL << 62)) && addr == (CORE_ADDR) dbr_addr)
 	{
-	  struct lwp_info *lp;
-
 	  debug_registers[2 * idx] = 0;
 	  debug_registers[2 * idx + 1] = 0;
 	  dbr_addr = 0;
 	  dbr_mask = 0;
 
-	  ALL_LWPS (lp)
+	  for (const lwp_info *lp : all_lwps ())
 	    store_debug_register_pair (lp->ptid, idx, &dbr_addr, &dbr_mask);
 
 	  return 0;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 930209db6e89..fd01bd9301c0 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -421,9 +421,8 @@ static int
 num_lwps (int pid)
 {
   int count = 0;
-  struct lwp_info *lp;
 
-  for (lp = lwp_list; lp; lp = lp->next)
+  for (const lwp_info *lp ATTRIBUTE_UNUSED : all_lwps ())
     if (lp->ptid.pid () == pid)
       count++;
 
@@ -700,17 +699,31 @@ lwp_lwpid_htab_add_lwp (struct lwp_info *lp)
    creation order.  This order is assumed in some cases.  E.g.,
    reaping status after killing alls lwps of a process: the leader LWP
    must be reaped last.  */
-struct lwp_info *lwp_list;
+
+static intrusive_list<lwp_info> lwp_list;
+
+/* See linux-nat.h.  */
+
+lwp_info_range
+all_lwps ()
+{
+  return lwp_info_range (lwp_list.begin ());
+}
+
+/* See linux-nat.h.  */
+
+lwp_info_safe_range
+all_lwps_safe ()
+{
+  return lwp_info_safe_range (lwp_list.begin ());
+}
 
 /* Add LP to sorted-by-reverse-creation-order doubly-linked list.  */
 
 static void
 lwp_list_add (struct lwp_info *lp)
 {
-  lp->next = lwp_list;
-  if (lwp_list != NULL)
-    lwp_list->prev = lp;
-  lwp_list = lp;
+  lwp_list.push_front (*lp);
 }
 
 /* Remove LP from sorted-by-reverse-creation-order doubly-linked
@@ -720,12 +733,7 @@ static void
 lwp_list_remove (struct lwp_info *lp)
 {
   /* Remove from sorted-by-creation-order list.  */
-  if (lp->next != NULL)
-    lp->next->prev = lp->prev;
-  if (lp->prev != NULL)
-    lp->prev->next = lp->next;
-  if (lp == lwp_list)
-    lwp_list = lp->next;
+  lwp_list.erase (lwp_list.iterator_to (*lp));
 }
 
 \f
@@ -922,12 +930,8 @@ struct lwp_info *
 iterate_over_lwps (ptid_t filter,
 		   gdb::function_view<iterate_over_lwps_ftype> callback)
 {
-  struct lwp_info *lp, *lpnext;
-
-  for (lp = lwp_list; lp; lp = lpnext)
+  for (lwp_info *lp : all_lwps_safe ())
     {
-      lpnext = lp->next;
-
       if (lp->ptid.matches (filter))
 	{
 	  if (callback (lp) != 0)
@@ -3716,8 +3720,6 @@ linux_nat_target::thread_alive (ptid_t ptid)
 void
 linux_nat_target::update_thread_list ()
 {
-  struct lwp_info *lwp;
-
   /* We add/delete threads from the list as clone/exit events are
      processed, so just try deleting exited threads still in the
      thread list.  */
@@ -3725,7 +3727,7 @@ linux_nat_target::update_thread_list ()
 
   /* Update the processor core that each lwp/thread was last seen
      running on.  */
-  ALL_LWPS (lwp)
+  for (lwp_info *lwp : all_lwps ())
     {
       /* Avoid accessing /proc if the thread hasn't run since we last
 	 time we fetched the thread's core.  Accessing /proc becomes
@@ -3949,7 +3951,7 @@ linux_proc_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
 
   /* Iterate over LWPs of the current inferior, trying to access
      memory through one of them.  */
-  for (lwp_info *lp = lwp_list; lp != nullptr; lp = lp->next)
+  for (lwp_info *lp : all_lwps ())
     {
       if (lp->ptid.pid () != cur_pid)
 	continue;
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index b1b168d2bfee..74b5eddc1365 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -196,11 +196,9 @@ extern linux_nat_target *linux_target;
 
 struct arch_lwp_info;
 
-/* Structure describing an LWP.  This is public only for the purposes
-   of ALL_LWPS; target-specific code should generally not access it
-   directly.  */
+/* Structure describing an LWP.  */
 
-struct lwp_info
+struct lwp_info : intrusive_list_node<lwp_info>
 {
   lwp_info (ptid_t ptid)
     : ptid (ptid)
@@ -283,27 +281,26 @@ struct lwp_info
 
   /* Arch-specific additions.  */
   struct arch_lwp_info *arch_private = nullptr;
-
-  /* Previous and next pointers in doubly-linked list of known LWPs,
-     sorted by reverse creation order.  */
-  struct lwp_info *prev = nullptr;
-  struct lwp_info *next = nullptr;
 };
 
-/* The global list of LWPs, for ALL_LWPS.  Unlike the threads list,
-   there is always at least one LWP on the list while the GNU/Linux
-   native target is active.  */
-extern struct lwp_info *lwp_list;
+/* lwp_info iterator and range types.  */
+
+using lwp_info_iterator
+  = reference_to_pointer_iterator<intrusive_list<lwp_info>::iterator>;
+using lwp_info_range = iterator_range<lwp_info_iterator>;
+using lwp_info_safe_range = basic_safe_range<lwp_info_range>;
+
+/* Get an iterable range over all lwps.  */
+
+lwp_info_range all_lwps ();
+
+/* Same as the above, but safe against deletion while iterating.  */
+
+lwp_info_safe_range all_lwps_safe ();
 
 /* Does the current host support PTRACE_GETREGSET?  */
 extern enum tribool have_ptrace_getregset;
 
-/* Iterate over each active thread (light-weight process).  */
-#define ALL_LWPS(LP)							\
-  for ((LP) = lwp_list;							\
-       (LP) != NULL;							\
-       (LP) = (LP)->next)
-
 /* Called from the LWP layer to inform the thread_db layer that PARENT
    spawned CHILD.  Both LWPs are currently stopped.  This function
    does whatever is required to have the child LWP under the
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index eb2aa5ac409a..aff00dc8d40e 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -920,13 +920,12 @@ try_thread_db_load_1 (struct thread_db_info *info)
 
   if (info->td_ta_thr_iter_p == NULL)
     {
-      struct lwp_info *lp;
       int pid = inferior_ptid.pid ();
       thread_info *curr_thread = inferior_thread ();
 
       linux_stop_and_wait_all_lwps ();
 
-      ALL_LWPS (lp)
+      for (const lwp_info *lp : all_lwps ())
 	if (lp->ptid.pid () == pid)
 	  thread_from_lwp (curr_thread, lp->ptid);
 
diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c
index b21c7cb2ea68..1088a7dc1b03 100644
--- a/gdb/mips-linux-nat.c
+++ b/gdb/mips-linux-nat.c
@@ -632,12 +632,9 @@ mips_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
 static int
 write_watchpoint_regs (void)
 {
-  struct lwp_info *lp;
-  int tid;
-
-  ALL_LWPS (lp)
+  for (const lwp_info *lp : all_lwps ())
     {
-      tid = lp->ptid.lwp ();
+      int tid = lp->ptid.lwp ();
       if (ptrace (PTRACE_SET_WATCH_REGS, tid, &watch_mirror, NULL) == -1)
 	perror_with_name (_("Couldn't write debug register"));
     }
-- 
2.33.0

Re: [PATCH 0/3] Make lwp_info use intrusive_list

[Simon Marchi]

On 2021-08-28 10:58, Simon Marchi wrote:
> The goal of this series is to replace the manually maintained linked
> list of lwp_info objects with an intrusive_list.  No user-visible
> changes expected, no regressions seen on Linux / x86-64.
> 
> Simon Marchi (3):
>   gdb: make lwp_info non-POD
>   gdb: add destructor to lwp_info
>   gdb: use intrusive_list for linux-nat lwp_list
> 
>  gdb/ia64-linux-nat.c  |  8 ++---
>  gdb/linux-nat.c       | 84 ++++++++++++++++++-------------------------
>  gdb/linux-nat.h       | 73 ++++++++++++++++++++-----------------
>  gdb/linux-thread-db.c |  3 +-
>  gdb/mips-linux-nat.c  |  7 ++--
>  5 files changed, 80 insertions(+), 95 deletions(-)
> 

I pushed this.

Simon