[PATCH 0/2] Clean up handling of DWARF queue

[PATCH 0/2] Clean up handling of DWARF queue

[Tom Tromey]

This series changes the DWARF CU expansion queue to simplify its
memory management and also to avoid destruction order dependencies.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/2] Change allocation of m_dwarf2_cus

[Tom Tromey]

m_dwarf2_cus manually manages the 'dwarf2_cu' pointers it owns.  This
patch simplifies the code by changing it to use unique_ptr.
---
 gdb/dwarf2/read.c | 23 +++++++++--------------
 gdb/dwarf2/read.h |  6 ++++--
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 848fd5627b8..d279e729db9 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1476,9 +1476,6 @@ dwarf2_per_objfile::remove_all_cus ()
 {
   gdb_assert (!this->per_bfd->queue.has_value ());
 
-  for (auto pair : m_dwarf2_cus)
-    delete pair.second;
-
   m_dwarf2_cus.clear ();
 }
 
@@ -6350,7 +6347,7 @@ cutu_reader::keep ()
       /* Save this dwarf2_cu in the per_objfile.  The per_objfile owns it
 	 now.  */
       dwarf2_per_objfile *per_objfile = m_new_cu->per_objfile;
-      per_objfile->set_cu (m_this_cu, m_new_cu.release ());
+      per_objfile->set_cu (m_this_cu, std::move (m_new_cu));
     }
 }
 
@@ -23535,17 +23532,18 @@ dwarf2_per_objfile::get_cu (dwarf2_per_cu_data *per_cu)
   if (it == m_dwarf2_cus.end ())
     return nullptr;
 
-  return it->second;
+  return it->second.get ();
 }
 
 /* See read.h.  */
 
 void
-dwarf2_per_objfile::set_cu (dwarf2_per_cu_data *per_cu, dwarf2_cu *cu)
+dwarf2_per_objfile::set_cu (dwarf2_per_cu_data *per_cu,
+			    std::unique_ptr<dwarf2_cu> cu)
 {
   gdb_assert (this->get_cu (per_cu) == nullptr);
 
-  m_dwarf2_cus[per_cu] = cu;
+  m_dwarf2_cus[per_cu] = std::move (cu);
 }
 
 /* See read.h.  */
@@ -23563,14 +23561,14 @@ dwarf2_per_objfile::age_comp_units ()
   gdb_assert (!this->per_bfd->queue.has_value ());
 
   /* Start by clearing all marks.  */
-  for (auto pair : m_dwarf2_cus)
+  for (const auto &pair : m_dwarf2_cus)
     pair.second->clear_mark ();
 
   /* Traverse all CUs, mark them and their dependencies if used recently
      enough.  */
-  for (auto pair : m_dwarf2_cus)
+  for (const auto &pair : m_dwarf2_cus)
     {
-      dwarf2_cu *cu = pair.second;
+      dwarf2_cu *cu = pair.second.get ();
 
       cu->last_used++;
       if (cu->last_used <= dwarf_max_cache_age)
@@ -23580,13 +23578,12 @@ dwarf2_per_objfile::age_comp_units ()
   /* Delete all CUs still not marked.  */
   for (auto it = m_dwarf2_cus.begin (); it != m_dwarf2_cus.end ();)
     {
-      dwarf2_cu *cu = it->second;
+      dwarf2_cu *cu = it->second.get ();
 
       if (!cu->is_marked ())
 	{
 	  dwarf_read_debug_printf_v ("deleting old CU %s",
 				     sect_offset_str (cu->per_cu->sect_off));
-	  delete cu;
 	  it = m_dwarf2_cus.erase (it);
 	}
       else
@@ -23603,8 +23600,6 @@ dwarf2_per_objfile::remove_cu (dwarf2_per_cu_data *per_cu)
   if (it == m_dwarf2_cus.end ())
     return;
 
-  delete it->second;
-
   m_dwarf2_cus.erase (it);
 }
 
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index b58c574c2be..75dd38c0b73 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -28,6 +28,7 @@
 #include "dwarf2/index-cache.h"
 #include "dwarf2/mapped-index.h"
 #include "dwarf2/section.h"
+#include "dwarf2/cu.h"
 #include "filename-seen-cache.h"
 #include "gdbsupport/gdb_obstack.h"
 #include "gdbsupport/hash_enum.h"
@@ -547,7 +548,7 @@ struct dwarf2_per_objfile
   dwarf2_cu *get_cu (dwarf2_per_cu_data *per_cu);
 
   /* Set the dwarf2_cu matching PER_CU for this objfile.  */
-  void set_cu (dwarf2_per_cu_data *per_cu, dwarf2_cu *cu);
+  void set_cu (dwarf2_per_cu_data *per_cu, std::unique_ptr<dwarf2_cu> cu);
 
   /* Remove/free the dwarf2_cu matching PER_CU for this objfile.  */
   void remove_cu (dwarf2_per_cu_data *per_cu);
@@ -596,7 +597,8 @@ struct dwarf2_per_objfile
 
   /* Map from the objfile-independent dwarf2_per_cu_data instances to the
      corresponding objfile-dependent dwarf2_cu instances.  */
-  std::unordered_map<dwarf2_per_cu_data *, dwarf2_cu *> m_dwarf2_cus;
+  std::unordered_map<dwarf2_per_cu_data *,
+		     std::unique_ptr<dwarf2_cu>> m_dwarf2_cus;
 };
 
 /* Get the dwarf2_per_objfile associated to OBJFILE.  */
-- 
2.34.1

[PATCH 2/2] Move CU queue to dwarf2_per_objfile

[Tom Tromey]

The CU queue is a member of dwarf2_per_bfd, but it is only used when
expanding CUs.  Also, the dwarf2_per_objfile destructor checks the
queue -- however, if the per-BFD object is destroyed first, this will
not work.  This was pointed out Lancelot as fallout from the patch to
rewrite the registry system.

This patch avoids this problem by moving the queue to the per-objfile
object.
---
 gdb/dwarf2/read.c | 22 +++++++++++-----------
 gdb/dwarf2/read.h |  6 +++---
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d279e729db9..696277d931a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1231,18 +1231,18 @@ class dwarf2_queue_guard
   explicit dwarf2_queue_guard (dwarf2_per_objfile *per_objfile)
     : m_per_objfile (per_objfile)
   {
-    gdb_assert (!m_per_objfile->per_bfd->queue.has_value ());
+    gdb_assert (!m_per_objfile->queue.has_value ());
 
-    m_per_objfile->per_bfd->queue.emplace ();
+    m_per_objfile->queue.emplace ();
   }
 
   /* Free any entries remaining on the queue.  There should only be
      entries left if we hit an error while processing the dwarf.  */
   ~dwarf2_queue_guard ()
   {
-    gdb_assert (m_per_objfile->per_bfd->queue.has_value ());
+    gdb_assert (m_per_objfile->queue.has_value ());
 
-    m_per_objfile->per_bfd->queue.reset ();
+    m_per_objfile->queue.reset ();
   }
 
   DISABLE_COPY_AND_ASSIGN (dwarf2_queue_guard);
@@ -1474,7 +1474,7 @@ dwarf2_per_bfd::~dwarf2_per_bfd ()
 void
 dwarf2_per_objfile::remove_all_cus ()
 {
-  gdb_assert (!this->per_bfd->queue.has_value ());
+  gdb_assert (!queue.has_value ());
 
   m_dwarf2_cus.clear ();
 }
@@ -7491,8 +7491,8 @@ queue_comp_unit (dwarf2_per_cu_data *per_cu,
 {
   per_cu->queued = 1;
 
-  gdb_assert (per_objfile->per_bfd->queue.has_value ());
-  per_cu->per_bfd->queue->emplace (per_cu, per_objfile, pretend_language);
+  gdb_assert (per_objfile->queue.has_value ());
+  per_objfile->queue->emplace (per_cu, per_objfile, pretend_language);
 }
 
 /* If PER_CU is not yet expanded of queued for expansion, add it to the queue.
@@ -7575,9 +7575,9 @@ process_queue (dwarf2_per_objfile *per_objfile)
 
   /* The queue starts out with one item, but following a DIE reference
      may load a new CU, adding it to the end of the queue.  */
-  while (!per_objfile->per_bfd->queue->empty ())
+  while (!per_objfile->queue->empty ())
     {
-      dwarf2_queue_item &item = per_objfile->per_bfd->queue->front ();
+      dwarf2_queue_item &item = per_objfile->queue->front ();
       dwarf2_per_cu_data *per_cu = item.per_cu;
 
       if (!per_objfile->symtab_set_p (per_cu))
@@ -7623,7 +7623,7 @@ process_queue (dwarf2_per_objfile *per_objfile)
 	}
 
       per_cu->queued = 0;
-      per_objfile->per_bfd->queue->pop ();
+      per_objfile->queue->pop ();
     }
 
   dwarf_read_debug_printf ("Done expanding symtabs of %s.",
@@ -23558,7 +23558,7 @@ dwarf2_per_objfile::age_comp_units ()
      loaded in memory.  Calling age_comp_units while the queue is in use could
      make us free the DIEs for a CU that is in the queue and therefore break
      that invariant.  */
-  gdb_assert (!this->per_bfd->queue.has_value ());
+  gdb_assert (!queue.has_value ());
 
   /* Start by clearing all marks.  */
   for (const auto &pair : m_dwarf2_cus)
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 75dd38c0b73..51e02dfc457 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -467,9 +467,6 @@ struct dwarf2_per_bfd
 		     gdb::hash_enum<sect_offset>>
     abstract_to_concrete;
 
-  /* CUs that are queued to be read.  */
-  gdb::optional<std::queue<dwarf2_queue_item>> queue;
-
   /* The address map that is used by the DWARF index code.  */
   struct addrmap *index_addrmap = nullptr;
 };
@@ -578,6 +575,9 @@ struct dwarf2_per_objfile
   /* The CU containing the m_builder in scope.  */
   dwarf2_cu *sym_cu = nullptr;
 
+  /* CUs that are queued to be read.  */
+  gdb::optional<std::queue<dwarf2_queue_item>> queue;
+
 private:
   /* Hold the corresponding compunit_symtab for each CU or TU.  This
      is indexed by dwarf2_per_cu_data::index.  A NULL value means
-- 
2.34.1

Re: [PATCH 0/2] Clean up handling of DWARF queue

[Simon Marchi]

On 2022-06-08 09:58, Tom Tromey wrote:
> This series changes the DWARF CU expansion queue to simplify its
> memory management and also to avoid destruction order dependencies.
> 
> Regression tested on x86-64 Fedora 34.
> 
> Tom
> 
> 

This looks good, thanks.

Simon