[binutils-gdb] Write the DWARF index in the background

[binutils-gdb] Write the DWARF index in the background

[Tom Tromey]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=52e5e48e533dd91686d04a200a11516e90b4cab7

commit 52e5e48e533dd91686d04a200a11516e90b4cab7
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Apr 13 11:25:53 2022 -0600

    Write the DWARF index in the background
    
    The new DWARF cooked indexer interacts poorly with the DWARF index
    cache.  In particular, the cache will require gdb to wait for the
    cooked index to be finalized.  As this happens in the foreground, it
    means that users with this setting enabled will see a slowdown.
    
    This patch changes gdb to write the cache entry a worker thread.  (As
    usual, in the absence of threads, this work is simply done immediately
    in the main thread.)
    
    Some care is taken to ensure that this can't crash, and that gdb will
    not exit before the task is complete.
    
    To avoid use-after-free problems, the DWARF per-BFD object explicitly
    waits for the index cache task to complete.
    
    To avoid gdb exiting early, an exit observer is used to wait for all
    such pending tasks.
    
    In normal use, neither of these waits will be very visible.  For users
    using "-batch" to pre-generate the index, though, it would be.
    However I don't think there is much to be done about this, as it was
    the status quo ante.

Diff:
---
 gdb/dwarf2/cooked-index.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/dwarf2/cooked-index.h | 33 +++++++++++++----------
 gdb/dwarf2/mapped-index.h |  9 +++++++
 gdb/dwarf2/read.c         | 10 ++++---
 4 files changed, 101 insertions(+), 19 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index d465028add4..47746aa9905 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -21,14 +21,23 @@
 #include "dwarf2/cooked-index.h"
 #include "dwarf2/read.h"
 #include "dwarf2/stringify.h"
+#include "dwarf2/index-cache.h"
 #include "cp-support.h"
 #include "c-lang.h"
 #include "ada-lang.h"
 #include "split-name.h"
+#include "observable.h"
+#include "run-on-main-thread.h"
 #include <algorithm>
 #include "safe-ctype.h"
 #include "gdbsupport/selftest.h"
 #include <chrono>
+#include <unordered_set>
+
+/* We don't want gdb to exit while it is in the process of writing to
+   the index cache.  So, all live cooked index vectors are stored
+   here, and then these are all waited for before exit proceeds.  */
+static std::unordered_set<cooked_index *> active_vectors;
 
 /* See cooked-index.h.  */
 
@@ -432,11 +441,46 @@ cooked_index_shard::wait (bool allow_quit) const
     m_future.wait ();
 }
 
-cooked_index::cooked_index (vec_type &&vec)
+cooked_index::cooked_index (vec_type &&vec, dwarf2_per_bfd *per_bfd)
   : m_vector (std::move (vec))
 {
   for (auto &idx : m_vector)
     idx->finalize ();
+
+  /* This must be set after all the finalization tasks have been
+     started, because it may call 'wait'.  */
+  m_write_future
+    = gdb::thread_pool::g_thread_pool->post_task ([this, per_bfd] ()
+      {
+	maybe_write_index (per_bfd);
+      });
+
+  /* ACTIVE_VECTORS is not locked, and this assert ensures that this
+     will be caught if ever moved to the background.  */
+  gdb_assert (is_main_thread ());
+  active_vectors.insert (this);
+}
+
+cooked_index::~cooked_index ()
+{
+  /* The 'finalize' method may be run in a different thread.  If
+     this object is destroyed before this completes, then the method
+     will end up writing to freed memory.  Waiting for this to
+     complete avoids this problem; and the cost seems ignorable
+     because creating and immediately destroying the debug info is a
+     relatively rare thing to do.  */
+  for (auto &item : m_vector)
+    item->wait (false);
+
+  /* Likewise for the index-creating future, though this one must also
+     waited for by the per-BFD object to ensure the required data
+     remains live.  */
+  wait_completely ();
+
+  /* Remove our entry from the global list.  See the assert in the
+     constructor to understand this.  */
+  gdb_assert (is_main_thread ());
+  active_vectors.erase (this);
 }
 
 /* See cooked-index.h.  */
@@ -576,6 +620,26 @@ cooked_index::dump (gdbarch *arch) const
     }
 }
 
+void
+cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd)
+{
+  /* Wait for finalization.  */
+  wait ();
+
+  /* (maybe) store an index in the cache.  */
+  global_index_cache.store (per_bfd);
+}
+
+/* Wait for all the index cache entries to be written before gdb
+   exits.  */
+static void
+wait_for_index_cache (int)
+{
+  gdb_assert (is_main_thread ());
+  for (cooked_index *item : active_vectors)
+    item->wait_completely ();
+}
+
 void _initialize_cooked_index ();
 void
 _initialize_cooked_index ()
@@ -583,4 +647,6 @@ _initialize_cooked_index ()
 #if GDB_SELF_TEST
   selftests::register_test ("cooked_index_entry::compare", test_compare);
 #endif
+
+  gdb::observers::gdb_exiting.attach (wait_for_index_cache, "cooked-index");
 }
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 135f982d4b4..b16b87bbb0e 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -36,6 +36,7 @@
 #include "gdbsupport/range-chain.h"
 
 struct dwarf2_per_cu_data;
+struct dwarf2_per_bfd;
 
 /* Flags that describe an entry in the index.  */
 enum cooked_index_flag_enum : unsigned char
@@ -365,7 +366,8 @@ public:
      object.  */
   using vec_type = std::vector<std::unique_ptr<cooked_index_shard>>;
 
-  explicit cooked_index (vec_type &&vec);
+  cooked_index (vec_type &&vec, dwarf2_per_bfd *per_bfd);
+  ~cooked_index () override;
   DISABLE_COPY_AND_ASSIGN (cooked_index);
 
   /* Wait until the finalization of the entire cooked_index is
@@ -376,19 +378,6 @@ public:
       item->wait ();
   }
 
-  ~cooked_index ()
-  {
-    /* The 'finalize' methods may be run in a different thread.  If
-       this object is destroyed before these complete, then one will
-       end up writing to freed memory.  Waiting for finalization to
-       complete avoids this problem; and the cost seems ignorable
-       because creating and immediately destroying the debug info is a
-       relatively rare thing to do.  Do not allow quitting from this
-       wait.  */
-    for (auto &item : m_vector)
-      item->wait (false);
-  }
-
   /* A range over a vector of subranges.  */
   using range = range_chain<cooked_index_shard::range>;
 
@@ -430,11 +419,27 @@ public:
   /* Dump a human-readable form of the contents of the index.  */
   void dump (gdbarch *arch) const;
 
+  /* Wait for the index to be completely finished.  For ordinary uses,
+     the index code ensures this itself -- e.g., 'all_entries' will
+     wait on the 'finalize' future.  However, on destruction, if an
+     index is being written, it's also necessary to wait for that to
+     complete.  */
+  void wait_completely () override
+  {
+    m_write_future.wait ();
+  }
+
 private:
 
+  /* Maybe write the index to the index cache.  */
+  void maybe_write_index (dwarf2_per_bfd *per_bfd);
+
   /* The vector of cooked_index objects.  This is stored because the
      entries are stored on the obstacks in those objects.  */
   vec_type m_vector;
+
+  /* A future that tracks when the 'index_write' method is done.  */
+  std::future<void> m_write_future;
 };
 
 #endif /* GDB_DWARF2_COOKED_INDEX_H */
diff --git a/gdb/dwarf2/mapped-index.h b/gdb/dwarf2/mapped-index.h
index 29c7291ef96..fb15da9f8d5 100644
--- a/gdb/dwarf2/mapped-index.h
+++ b/gdb/dwarf2/mapped-index.h
@@ -73,6 +73,15 @@ struct dwarf_scanner_base
      will return 'this' as a cooked index.  For other forms, it will
      throw an exception with an appropriate error message.  */
   virtual cooked_index *index_for_writing () = 0;
+
+  /* Wait for reading of the debuginfo to be completely finished.
+     This normally has a trivial implementation, but if a subclass
+     does any background reading, it's needed to ensure that the
+     reading is completed before destroying the containing per-BFD
+     object.  */
+  virtual void wait_completely ()
+  {
+  }
 };
 
 /* Base class containing bits shared by both .gdb_index and
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 712e8b286ba..69b2310be1f 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1234,6 +1234,11 @@ dwarf2_per_bfd::dwarf2_per_bfd (bfd *obfd, const dwarf2_debug_sections *names,
 
 dwarf2_per_bfd::~dwarf2_per_bfd ()
 {
+  /* Data from the per-BFD may be needed when finalizing the cooked
+     index table, so wait here while this happens.  */
+  if (index_table != nullptr)
+    index_table->wait_completely ();
+
   for (auto &per_cu : all_units)
     {
       per_cu->imported_symtabs_free ();
@@ -3422,9 +3427,6 @@ dwarf2_build_psymtabs (struct objfile *objfile)
   try
     {
       dwarf2_build_psymtabs_hard (per_objfile);
-
-      /* (maybe) store an index in the cache.  */
-      global_index_cache.store (per_objfile->per_bfd);
     }
   catch (const gdb_exception_error &except)
     {
@@ -5162,7 +5164,7 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
   indexes.push_back (index_storage.release ());
   indexes.shrink_to_fit ();
 
-  cooked_index *vec = new cooked_index (std::move (indexes));
+  cooked_index *vec = new cooked_index (std::move (indexes), per_bfd);
   per_bfd->index_table.reset (vec);
 
   const cooked_index_entry *main_entry = vec->get_main ();