[PATCH v3 0/2] Write DWARF index cache files in background

[PATCH v3 0/2] Write DWARF index cache files in background

[Tom Tromey]

Here's v3 of the patches to write the DWARF index in the background.

v2 was here:

https://sourceware.org/pipermail/gdb-patches/2022-June/189889.html

This hides a user-noticeable pause when the feature is enabled.  (Note
that, when the feature is used in batch mode, the pause is still
seen.)

This version of the series fixes a possible race that I found in v2.
Essentially, the index-writing background job must be started after
all the finalization tasks have started.  There's a comment in the
code explaining this.

Let me know what you think.

Tom

[PATCH v3 1/2] Only use the per-BFD object to write a DWARF index

[Tom Tromey]

The DWARF index does not need access to the objfile or per-objfile
objects when writing -- it's entirely based on the objfile-independent
per-BFD data.

This patch implements this idea by changing the entire API to only be
passed the per-BFD object.  This simplifies some lifetime reasoning
for the next patch.

This patch removes some code that ensures that the BFD came from a
file.  It seems to me that checking for the existence of a build-id is
good enough for the index cache.
---
 gdb/dwarf2/index-cache.c | 20 ++++------
 gdb/dwarf2/index-cache.h |  4 +-
 gdb/dwarf2/index-write.c | 81 ++++++++++++++++++----------------------
 gdb/dwarf2/index-write.h |  2 +-
 gdb/dwarf2/read.c        |  2 +-
 5 files changed, 48 insertions(+), 61 deletions(-)

diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 6de58592050..b2902de5dcd 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -89,23 +89,17 @@ index_cache::disable ()
 /* See dwarf-index-cache.h.  */
 
 void
-index_cache::store (dwarf2_per_objfile *per_objfile)
+index_cache::store (dwarf2_per_bfd *per_bfd)
 {
-  objfile *obj = per_objfile->objfile;
-
   if (!enabled ())
     return;
 
-  /* If the objfile does not correspond to an actual file, skip it.  */
-  if ((obj->flags & OBJF_NOT_FILENAME) != 0)
-    return;
-
   /* Get build id of objfile.  */
-  const bfd_build_id *build_id = build_id_bfd_get (obj->obfd.get ());
+  const bfd_build_id *build_id = build_id_bfd_get (per_bfd->obfd);
   if (build_id == nullptr)
     {
       index_cache_debug ("objfile %s has no build id",
-			 objfile_name (obj));
+			 bfd_get_filename (per_bfd->obfd));
       return;
     }
 
@@ -113,7 +107,7 @@ index_cache::store (dwarf2_per_objfile *per_objfile)
 
   /* Get build id of dwz file, if present.  */
   gdb::optional<std::string> dwz_build_id_str;
-  const dwz_file *dwz = dwarf2_get_dwz_file (per_objfile->per_bfd);
+  const dwz_file *dwz = dwarf2_get_dwz_file (per_bfd);
   const char *dwz_build_id_ptr = NULL;
 
   if (dwz != nullptr)
@@ -148,18 +142,18 @@ index_cache::store (dwarf2_per_objfile *per_objfile)
 	}
 
       index_cache_debug ("writing index cache for objfile %s",
-			 objfile_name (obj));
+			 bfd_get_filename (per_bfd->obfd));
 
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
-      write_dwarf_index (per_objfile, m_dir.c_str (),
+      write_dwarf_index (per_bfd, m_dir.c_str (),
 			 build_id_str.c_str (), dwz_build_id_ptr,
 			 dw_index_kind::GDB_INDEX);
     }
   catch (const gdb_exception_error &except)
     {
       index_cache_debug ("couldn't store index cache for objfile %s: %s",
-			 objfile_name (obj), except.what ());
+			 bfd_get_filename (per_bfd->obfd), except.what ());
     }
 }
 
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 6366a9a9360..a6ac0112e41 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -24,6 +24,8 @@
 #include "gdbsupport/array-view.h"
 #include "symfile.h"
 
+class dwarf2_per_bfd;
+
 /* Base of the classes used to hold the resources of the indices loaded from
    the cache (e.g. mmapped files).  */
 
@@ -53,7 +55,7 @@ class index_cache
   void disable ();
 
   /* Store an index for the specified object file in the cache.  */
-  void store (dwarf2_per_objfile *per_objfile);
+  void store (dwarf2_per_bfd *per_bfd);
 
   /* Look for an index file matching BUILD_ID.  If found, return the contents
      as an array_view and store the underlying resources (allocated memory,
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 3d215a6307b..a63c461da0b 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -522,7 +522,7 @@ write_address_map (struct addrmap *addrmap, data_buf &addr_vec,
 class debug_names
 {
 public:
-  debug_names (dwarf2_per_objfile *per_objfile, bool is_dwarf64,
+  debug_names (dwarf2_per_bfd *per_bfd, bool is_dwarf64,
 	       bfd_endian dwarf5_byte_order)
     : m_dwarf5_byte_order (dwarf5_byte_order),
       m_dwarf32 (dwarf5_byte_order),
@@ -532,7 +532,7 @@ class debug_names
 	       : static_cast<dwarf &> (m_dwarf32)),
       m_name_table_string_offs (m_dwarf.name_table_string_offs),
       m_name_table_entry_offs (m_dwarf.name_table_entry_offs),
-      m_debugstrlookup (per_objfile)
+      m_debugstrlookup (per_bfd)
   {}
 
   int dwarf5_offset_size () const
@@ -785,23 +785,23 @@ class debug_names
   {
   public:
 
-    /* Object constructor to be called for current DWARF2_PER_OBJFILE.
+    /* Object constructor to be called for current DWARF2_PER_BFD.
        All .debug_str section strings are automatically stored.  */
-    debug_str_lookup (dwarf2_per_objfile *per_objfile)
-      : m_abfd (per_objfile->objfile->obfd.get ()),
-	m_per_objfile (per_objfile)
+    debug_str_lookup (dwarf2_per_bfd *per_bfd)
+      : m_abfd (per_bfd->obfd),
+	m_per_bfd (per_bfd)
     {
-      per_objfile->per_bfd->str.read (per_objfile->objfile);
-      if (per_objfile->per_bfd->str.buffer == NULL)
+      gdb_assert (per_bfd->str.readin);
+      if (per_bfd->str.buffer == NULL)
 	return;
-      for (const gdb_byte *data = per_objfile->per_bfd->str.buffer;
-	   data < (per_objfile->per_bfd->str.buffer
-		   + per_objfile->per_bfd->str.size);)
+      for (const gdb_byte *data = per_bfd->str.buffer;
+	   data < (per_bfd->str.buffer
+		   + per_bfd->str.size);)
 	{
 	  const char *const s = reinterpret_cast<const char *> (data);
 	  const auto insertpair
 	    = m_str_table.emplace (c_str_view (s),
-				   data - per_objfile->per_bfd->str.buffer);
+				   data - per_bfd->str.buffer);
 	  if (!insertpair.second)
 	    complaint (_("Duplicate string \"%s\" in "
 			 ".debug_str section [in module %s]"),
@@ -818,7 +818,7 @@ class debug_names
       const auto it = m_str_table.find (c_str_view (s));
       if (it != m_str_table.end ())
 	return it->second;
-      const size_t offset = (m_per_objfile->per_bfd->str.size
+      const size_t offset = (m_per_bfd->str.size
 			     + m_str_add_buf.size ());
       m_str_table.emplace (c_str_view (s), offset);
       m_str_add_buf.append_cstr0 (s);
@@ -834,7 +834,7 @@ class debug_names
   private:
     std::unordered_map<c_str_view, size_t, c_str_view_hasher> m_str_table;
     bfd *const m_abfd;
-    dwarf2_per_objfile *m_per_objfile;
+    dwarf2_per_bfd *m_per_bfd;
 
     /* Data to add at the end of .debug_str for new needed symbol names.  */
     data_buf m_str_add_buf;
@@ -1046,9 +1046,9 @@ class debug_names
    .debug_names section.  */
 
 static bool
-check_dwarf64_offsets (dwarf2_per_objfile *per_objfile)
+check_dwarf64_offsets (dwarf2_per_bfd *per_bfd)
 {
-  for (const auto &per_cu : per_objfile->per_bfd->all_units)
+  for (const auto &per_cu : per_bfd->all_units)
     {
       if (to_underlying (per_cu->sect_off)
 	  >= (static_cast<uint64_t> (1) << 32))
@@ -1197,7 +1197,7 @@ write_cooked_index (cooked_index_vector *table,
    associated dwz file, DWZ_OUT_FILE must be NULL.  */
 
 static void
-write_gdbindex (dwarf2_per_objfile *per_objfile,
+write_gdbindex (dwarf2_per_bfd *per_bfd,
 		cooked_index_vector *table,
 		FILE *out_file, FILE *dwz_out_file)
 {
@@ -1210,7 +1210,7 @@ write_gdbindex (dwarf2_per_objfile *per_objfile,
      in the index file).  This will later be needed to write the address
      table.  */
   cu_index_map cu_index_htab;
-  cu_index_htab.reserve (per_objfile->per_bfd->all_units.size ());
+  cu_index_htab.reserve (per_bfd->all_units.size ());
 
   /* Store out the .debug_type CUs, if any.  */
   data_buf types_cu_list;
@@ -1221,10 +1221,9 @@ write_gdbindex (dwarf2_per_objfile *per_objfile,
 
   int counter = 0;
   int types_counter = 0;
-  for (int i = 0; i < per_objfile->per_bfd->all_units.size (); ++i)
+  for (int i = 0; i < per_bfd->all_units.size (); ++i)
     {
-      dwarf2_per_cu_data *per_cu
-	= per_objfile->per_bfd->all_units[i].get ();
+      dwarf2_per_cu_data *per_cu = per_bfd->all_units[i].get ();
 
       int &this_counter = per_cu->is_debug_types ? types_counter : counter;
 
@@ -1287,27 +1286,25 @@ static const gdb_byte dwarf5_gdb_augmentation[] = { 'G', 'D', 'B', 0 };
    many bytes were expected to be written into OUT_FILE.  */
 
 static void
-write_debug_names (dwarf2_per_objfile *per_objfile,
+write_debug_names (dwarf2_per_bfd *per_bfd,
 		   cooked_index_vector *table,
 		   FILE *out_file, FILE *out_file_str)
 {
-  const bool dwarf5_is_dwarf64 = check_dwarf64_offsets (per_objfile);
-  struct objfile *objfile = per_objfile->objfile;
+  const bool dwarf5_is_dwarf64 = check_dwarf64_offsets (per_bfd);
   const enum bfd_endian dwarf5_byte_order
-    = gdbarch_byte_order (objfile->arch ());
+    = bfd_big_endian (per_bfd->obfd) ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
 
   /* The CU list is already sorted, so we don't need to do additional
      work here.  Also, the debug_types entries do not appear in
      all_units, but only in their own hash table.  */
   data_buf cu_list;
   data_buf types_cu_list;
-  debug_names nametable (per_objfile, dwarf5_is_dwarf64, dwarf5_byte_order);
+  debug_names nametable (per_bfd, dwarf5_is_dwarf64, dwarf5_byte_order);
   int counter = 0;
   int types_counter = 0;
-  for (int i = 0; i < per_objfile->per_bfd->all_units.size (); ++i)
+  for (int i = 0; i < per_bfd->all_units.size (); ++i)
     {
-      dwarf2_per_cu_data *per_cu
-	= per_objfile->per_bfd->all_units[i].get ();
+      dwarf2_per_cu_data *per_cu = per_bfd->all_units[i].get ();
 
       int &this_counter = per_cu->is_debug_types ? types_counter : counter;
       data_buf &this_list = per_cu->is_debug_types ? types_cu_list : cu_list;
@@ -1320,8 +1317,8 @@ write_debug_names (dwarf2_per_objfile *per_objfile,
     }
 
    /* Verify that all units are represented.  */
-  gdb_assert (counter == per_objfile->per_bfd->all_comp_units.size ());
-  gdb_assert (types_counter == per_objfile->per_bfd->all_type_units.size ());
+  gdb_assert (counter == per_bfd->all_units.size ());
+  gdb_assert (types_counter == per_bfd->all_type_units.size ());
 
   for (const cooked_index_entry *entry : table->all_entries ())
     nametable.insert (entry);
@@ -1454,23 +1451,17 @@ struct index_wip_file
 /* See dwarf-index-write.h.  */
 
 void
-write_dwarf_index (dwarf2_per_objfile *per_objfile, const char *dir,
+write_dwarf_index (dwarf2_per_bfd *per_bfd, const char *dir,
 		   const char *basename, const char *dwz_basename,
 		   dw_index_kind index_kind)
 {
-  struct objfile *objfile = per_objfile->objfile;
-
-  if (per_objfile->per_bfd->index_table == nullptr)
+  if (per_bfd->index_table == nullptr)
     error (_("No debugging symbols"));
-  cooked_index_vector *table
-    = per_objfile->per_bfd->index_table->index_for_writing ();
+  cooked_index_vector *table = per_bfd->index_table->index_for_writing ();
 
-  if (per_objfile->per_bfd->types.size () > 1)
+  if (per_bfd->types.size () > 1)
     error (_("Cannot make an index when the file has multiple .debug_types sections"));
 
-
-  gdb_assert ((objfile->flags & OBJF_NOT_FILENAME) == 0);
-
   const char *index_suffix = (index_kind == dw_index_kind::DEBUG_NAMES
 			      ? INDEX5_SUFFIX : INDEX4_SUFFIX);
 
@@ -1484,13 +1475,13 @@ write_dwarf_index (dwarf2_per_objfile *per_objfile, const char *dir,
     {
       index_wip_file str_wip_file (dir, basename, DEBUG_STR_SUFFIX);
 
-      write_debug_names (per_objfile, table, objfile_index_wip.out_file.get (),
+      write_debug_names (per_bfd, table, objfile_index_wip.out_file.get (),
 			 str_wip_file.out_file.get ());
 
       str_wip_file.finalize ();
     }
   else
-    write_gdbindex (per_objfile, table, objfile_index_wip.out_file.get (),
+    write_gdbindex (per_bfd, table, objfile_index_wip.out_file.get (),
 		    (dwz_index_wip.has_value ()
 		     ? dwz_index_wip->out_file.get () : NULL));
 
@@ -1545,8 +1536,8 @@ save_gdb_index_command (const char *arg, int from_tty)
 	      if (dwz != NULL)
 		dwz_basename = lbasename (dwz->filename ());
 
-	      write_dwarf_index (per_objfile, arg, basename, dwz_basename,
-				 index_kind);
+	      write_dwarf_index (per_objfile->per_bfd, arg, basename,
+				 dwz_basename, index_kind);
 	    }
 	  catch (const gdb_exception_error &except)
 	    {
diff --git a/gdb/dwarf2/index-write.h b/gdb/dwarf2/index-write.h
index b8855dcae7c..3a9c1432c7f 100644
--- a/gdb/dwarf2/index-write.h
+++ b/gdb/dwarf2/index-write.h
@@ -33,7 +33,7 @@
    same, but for the dwz file's index.  */
 
 extern void write_dwarf_index
-  (dwarf2_per_objfile *per_objfile, const char *dir, const char *basename,
+  (dwarf2_per_bfd *per_bfd, const char *dir, const char *basename,
    const char *dwz_basename, dw_index_kind index_kind);
 
 #endif /* DWARF_INDEX_WRITE_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ddea38cf596..fc8239b4261 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -5434,7 +5434,7 @@ dwarf2_build_psymtabs (struct objfile *objfile)
       dwarf2_build_psymtabs_hard (per_objfile);
 
       /* (maybe) store an index in the cache.  */
-      global_index_cache.store (per_objfile);
+      global_index_cache.store (per_objfile->per_bfd);
     }
   catch (const gdb_exception_error &except)
     {
-- 
2.38.1

[PATCH v3 2/2] Write the DWARF index in the background

[Tom Tromey]

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.
---
 gdb/dwarf2/cooked-index.c | 73 ++++++++++++++++++++++++++++++++++++++-
 gdb/dwarf2/cooked-index.h | 31 ++++++++++-------
 gdb/dwarf2/mapped-index.h |  9 +++++
 gdb/dwarf2/read.c         | 11 +++---
 4 files changed, 107 insertions(+), 17 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 0aa026c7779..172a4dd96d2 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -20,11 +20,20 @@
 #include "defs.h"
 #include "dwarf2/cooked-index.h"
 #include "dwarf2/read.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 <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_vector *> active_vectors;
 
 /* Hash function for cooked_index_entry.  */
 
@@ -282,11 +291,46 @@ cooked_index::find (gdb::string_view name, bool completing)
   return range (lower, upper);
 }
 
-cooked_index_vector::cooked_index_vector (vec_type &&vec)
+cooked_index_vector::cooked_index_vector (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_vector::~cooked_index_vector ()
+{
+  /* 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.  */
+  wait ();
+
+  /* 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.  */
@@ -345,3 +389,30 @@ cooked_index_vector::get_main () const
 
   return result;
 }
+
+void
+cooked_index_vector::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_vector *item : active_vectors)
+    item->wait_completely ();
+}
+
+void _initialize_cooked_index ();
+void
+_initialize_cooked_index ()
+{
+  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 2ea32781be5..fb901c6f87f 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
@@ -307,7 +308,8 @@ class cooked_index_vector : public dwarf_scanner_base
      object.  */
   typedef std::vector<std::unique_ptr<cooked_index>> vec_type;
 
-  explicit cooked_index_vector (vec_type &&vec);
+  cooked_index_vector (vec_type &&vec, dwarf2_per_bfd *per_bfd);
+  ~cooked_index_vector () override;
   DISABLE_COPY_AND_ASSIGN (cooked_index_vector);
 
   /* Wait until the finalization of the entire cooked_index_vector is
@@ -318,17 +320,6 @@ class cooked_index_vector : public dwarf_scanner_base
       item->wait ();
   }
 
-  ~cooked_index_vector ()
-  {
-    /* 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.  */
-    wait ();
-  }
-
   /* A range over a vector of subranges.  */
   typedef range_chain<cooked_index::range> range;
 
@@ -367,11 +358,27 @@ class cooked_index_vector : public dwarf_scanner_base
 
   quick_symbol_functions_up make_quick_functions () const override;
 
+  /* 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 7d71347f9f4..e0c3a447991 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_vector *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 fc8239b4261..6d27fbf1d75 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1467,6 +1467,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 ();
@@ -5432,9 +5437,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)
     {
@@ -7182,7 +7184,8 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
      indexes.end ());
   indexes.shrink_to_fit ();
 
-  cooked_index_vector *vec = new cooked_index_vector (std::move (indexes));
+  cooked_index_vector *vec = new cooked_index_vector (std::move (indexes),
+						      per_bfd);
   per_bfd->index_table.reset (vec);
 
   const cooked_index_entry *main_entry = vec->get_main ();
-- 
2.38.1

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Here's v3 of the patches to write the DWARF index in the background.
Tom> v2 was here:

Tom> https://sourceware.org/pipermail/gdb-patches/2022-June/189889.html

Tom> This hides a user-noticeable pause when the feature is enabled.  (Note
Tom> that, when the feature is used in batch mode, the pause is still
Tom> seen.)

Tom> This version of the series fixes a possible race that I found in v2.
Tom> Essentially, the index-writing background job must be started after
Tom> all the finalization tasks have started.  There's a comment in the
Tom> code explaining this.

I'm checking this in now.

Tom

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Simon Marchi]

On 2/24/23 13:46, Tom Tromey via Gdb-patches wrote:
>>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Here's v3 of the patches to write the DWARF index in the background.
> Tom> v2 was here:
> 
> Tom> https://sourceware.org/pipermail/gdb-patches/2022-June/189889.html
> 
> Tom> This hides a user-noticeable pause when the feature is enabled.  (Note
> Tom> that, when the feature is used in batch mode, the pause is still
> Tom> seen.)
> 
> Tom> This version of the series fixes a possible race that I found in v2.
> Tom> Essentially, the index-writing background job must be started after
> Tom> all the finalization tasks have started.  There's a comment in the
> Tom> code explaining this.
> 
> I'm checking this in now.
> 
> Tom

I started seeing this, I guess it's related:

FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file was created
FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

Simon

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I started seeing this, I guess it's related:

Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file was created
Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

Yeah, I will look into it soon.

Tom

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Tom Tromey]

Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file was created
Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

I can't reliably reproduce this.

However, I believe the problem is that because the cache file is written
in the background, the test is racy.

I think the appended should fix it.  This just mirrors something done in
test_cache_enabled_hit, and the idea is that waiting for gdb to exit
ensures that the cache file will have been written.

Tom

diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
index 0614d4ee2db..d9a6145b5df 100644
--- a/gdb/testsuite/gdb.base/index-cache.exp
+++ b/gdb/testsuite/gdb.base/index-cache.exp
@@ -157,6 +157,9 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
     lassign [ls_host $cache_dir] ret files_before
 
+    # Just to populate the cache.
+    run_test_with_flags $cache_dir on {}
+
     run_test_with_flags $cache_dir on {
 
 	lassign [ls_host $cache_dir] ret files_after

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Simon Marchi]

On 3/3/23 11:41, Tom Tromey wrote:
> Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file was created
> Simon> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there
> 
> I can't reliably reproduce this.
> 
> However, I believe the problem is that because the cache file is written
> in the background, the test is racy.

Ok, thanks for the tip.  It should be possible to reliably reproduce it
with that change then, which makes GDB take more time to write the
index:


diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 62c2cc6ac7de..d3bad4aaa0c9 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1485,6 +1485,7 @@ write_dwarf_index (dwarf2_per_bfd *per_bfd, const char *dir,
 		    (dwz_index_wip.has_value ()
 		     ? dwz_index_wip->out_file.get () : NULL));

+  sleep(3);
   objfile_index_wip.finalize ();

   if (dwz_index_wip.has_value ())

> I think the appended should fix it.  This just mirrors something done in
> test_cache_enabled_hit, and the idea is that waiting for gdb to exit
> ensures that the cache file will have been written.
> 
> Tom
> 
> diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
> index 0614d4ee2db..d9a6145b5df 100644
> --- a/gdb/testsuite/gdb.base/index-cache.exp
> +++ b/gdb/testsuite/gdb.base/index-cache.exp
> @@ -157,6 +157,9 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
>  
>      lassign [ls_host $cache_dir] ret files_before
>  
> +    # Just to populate the cache.
> +    run_test_with_flags $cache_dir on {}
> +
>      run_test_with_flags $cache_dir on {
>  
>  	lassign [ls_host $cache_dir] ret files_after

I don't understand this.  The first run_test_with_flags will populate
the cache, so the second run will use the cache and be a cache hit.  The
tests that checks that the expected file is there will work.  But the
check_cache_stats test will operate the GDB that will have had a cache
hit, so we won't get the expected result (a cache miss).

I think this could be solved with a maintenance command to explicitly
wait for index cache creation (calls wait_for_index_cache).  I don't see
another reliable way.  What do you think?  I can implement it if you
think it would work.

Simon

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Tom Tromey]

Simon> I don't understand this.

Yeah, sorry, I didn't properly understand the test.

Simon> I think this could be solved with a maintenance command to explicitly
Simon> wait for index cache creation (calls wait_for_index_cache).  I don't see
Simon> another reliable way.  What do you think?  I can implement it if you
Simon> think it would work.

I think you're right.  I can handle it.

Tom

Re: [PATCH v3 0/2] Write DWARF index cache files in background

[Simon Marchi]

On 3/3/23 12:29, Tom Tromey via Gdb-patches wrote:
> Simon> I don't understand this.
> 
> Yeah, sorry, I didn't properly understand the test.
> 
> Simon> I think this could be solved with a maintenance command to explicitly
> Simon> wait for index cache creation (calls wait_for_index_cache).  I don't see
> Simon> another reliable way.  What do you think?  I can implement it if you
> Simon> think it would work.
> 
> I think you're right.  I can handle it.

I spotted another failure, when running with native-extended-gdbserver:


254 (gdb) file /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/index-cache/index-cache^M
255 Reading symbols from /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/index-cache/index-cache...^M
256 /home/smarchi/src/binutils-gdb/gdb/gdbtypes.h:967: internal-error: field: Assertion `idx >= 0 && idx < num_fields ()' failed.^M
257 A problem internal to GDB has been detected,^M
258 further debugging may prove unreliable.^M
259 ERROR: Couldn't load index-cache into GDB (GDB internal error).

I don't know why it would be specific to native-extended-gdbserver,
maybe it changes the timing in just the right (wrong) way.  I haven't
dug into it yet.

Simon