[PATCH 0/5] Fix race when writing to index cache

[PATCH 0/5] Fix race when writing to index cache

[Tom Tromey]

This series is mostly cleanup, in preparation for the final patch that
fixes a race when writing to the index cache.

---
Tom Tromey (5):
      Rename members of index_cache_store_context
      Capture directory in index_cache_store_context
      Capture the per-BFD object in index_cache_store_context
      Move the 'store' method to index_cache_store_context
      Avoid race when writing to index cache

 gdb/dwarf2/cooked-index.c | 28 ++++++++++++----------------
 gdb/dwarf2/cooked-index.h | 16 ++++++++++------
 gdb/dwarf2/index-cache.c  | 33 +++++++++++++++++++--------------
 gdb/dwarf2/index-cache.h  | 19 +++++++++++--------
 4 files changed, 52 insertions(+), 44 deletions(-)
---
base-commit: 3dfc29463ae08a45adb6f1f7bb2a5232c1a33151
change-id: 20240128-pr-31262-index-cache-race-0488a29d2d10

Best regards,
-- 
Tom Tromey <tom@tromey.com>

[PATCH 1/5] Rename members of index_cache_store_context

[Tom Tromey]

This renames the private members of index_cache_store_context to start
with "m_".
---
 gdb/dwarf2/index-cache.c | 10 +++++-----
 gdb/dwarf2/index-cache.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 7ddd7b3f974..645e36c66f7 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -104,7 +104,7 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
       m_enabled = false;
       return;
     }
-  build_id_str = build_id_to_string (build_id);
+  m_build_id_str = build_id_to_string (build_id);
 
   /* Get build id of dwz file, if present.  */
   const dwz_file *dwz = dwarf2_get_dwz_file (per_bfd);
@@ -121,7 +121,7 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
 	  return;
 	}
 
-      dwz_build_id_str = build_id_to_string (dwz_build_id);
+      m_dwz_build_id_str = build_id_to_string (dwz_build_id);
     }
 
   if (ic.m_dir.empty ())
@@ -159,8 +159,8 @@ index_cache::store (dwarf2_per_bfd *per_bfd,
   if (!ctx.m_enabled)
     return;
 
-  const char *dwz_build_id_ptr = (ctx.dwz_build_id_str.has_value ()
-				  ? ctx.dwz_build_id_str->c_str ()
+  const char *dwz_build_id_ptr = (ctx.m_dwz_build_id_str.has_value ()
+				  ? ctx.m_dwz_build_id_str->c_str ()
 				  : nullptr);
 
   try
@@ -171,7 +171,7 @@ index_cache::store (dwarf2_per_bfd *per_bfd,
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
       write_dwarf_index (per_bfd, m_dir.c_str (),
-			 ctx.build_id_str.c_str (), dwz_build_id_ptr,
+			 ctx.m_build_id_str.c_str (), dwz_build_id_ptr,
 			 dw_index_kind::GDB_INDEX);
     }
   catch (const gdb_exception_error &except)
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index db2ec82c958..079ed412d2e 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -49,10 +49,10 @@ struct index_cache_store_context
   bool m_enabled;
 
   /* Captured value of build id.  */
-  std::string build_id_str;
+  std::string m_build_id_str;
 
   /* Captured value of dwz build id.  */
-  std::optional<std::string> dwz_build_id_str;
+  std::optional<std::string> m_dwz_build_id_str;
 };
 
 /* Class to manage the access to the DWARF index cache.  */

-- 
2.43.0

[PATCH 2/5] Capture directory in index_cache_store_context

[Tom Tromey]

I noticed that index_cache_store_context captures the 'enabled'
setting, but not the index cache directory.  This patch makes this
change, which avoids a possible race -- with background reading, the
user could possibly change this directory at the exact moment the
writer examines the variable.
---
 gdb/dwarf2/index-cache.c | 9 +++++----
 gdb/dwarf2/index-cache.h | 3 +++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 645e36c66f7..a269eb46a36 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -90,7 +90,8 @@ index_cache::disable ()
 
 index_cache_store_context::index_cache_store_context (const index_cache &ic,
 						      dwarf2_per_bfd *per_bfd)
-  :  m_enabled (ic.enabled ())
+  :  m_enabled (ic.enabled ()),
+     m_dir (ic.m_dir)
 {
   if (!m_enabled)
     return;
@@ -124,7 +125,7 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
       m_dwz_build_id_str = build_id_to_string (dwz_build_id);
     }
 
-  if (ic.m_dir.empty ())
+  if (m_dir.empty ())
     {
       warning (_("The index cache directory name is empty, skipping store."));
       m_enabled = false;
@@ -134,7 +135,7 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
   try
     {
       /* Try to create the containing directory.  */
-      if (!mkdir_recursive (ic.m_dir.c_str ()))
+      if (!mkdir_recursive (m_dir.c_str ()))
 	{
 	  warning (_("index cache: could not make cache directory: %s"),
 		   safe_strerror (errno));
@@ -170,7 +171,7 @@ index_cache::store (dwarf2_per_bfd *per_bfd,
 
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
-      write_dwarf_index (per_bfd, m_dir.c_str (),
+      write_dwarf_index (per_bfd, ctx.m_dir.c_str (),
 			 ctx.m_build_id_str.c_str (), dwz_build_id_ptr,
 			 dw_index_kind::GDB_INDEX);
     }
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 079ed412d2e..f66f72caea1 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -48,6 +48,9 @@ struct index_cache_store_context
   /* Captured value of enabled ().  */
   bool m_enabled;
 
+  /* Captured value of index cache directory.  */
+  std::string m_dir;
+
   /* Captured value of build id.  */
   std::string m_build_id_str;
 

-- 
2.43.0

[PATCH 3/5] Capture the per-BFD object in index_cache_store_context

[Tom Tromey]

This changes index_cache_store_context to also capture the per-BFD
object when it is constructed.  This is used when storing to the
cache, and this approach makes the code a little simpler.
---
 gdb/dwarf2/cooked-index.c |  7 +++----
 gdb/dwarf2/cooked-index.h |  3 +--
 gdb/dwarf2/index-cache.c  | 12 ++++++------
 gdb/dwarf2/index-cache.h  |  6 ++++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index d5a21ee9dec..c53b5129d9e 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -619,7 +619,7 @@ cooked_index::set_contents (vec_type &&vec)
   gdb::task_group finalizers ([this, ctx = std::move (ctx)] ()
   {
     m_state->set (cooked_state::FINALIZED);
-    maybe_write_index (m_per_bfd, ctx);
+    maybe_write_index (ctx);
   });
 
   for (auto &idx : m_vector)
@@ -825,13 +825,12 @@ cooked_index::dump (gdbarch *arch)
 }
 
 void
-cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd,
-				 const index_cache_store_context &ctx)
+cooked_index::maybe_write_index (const index_cache_store_context &ctx)
 {
   if (index_for_writing () != nullptr)
     {
       /* (maybe) store an index in the cache.  */
-      global_index_cache.store (m_per_bfd, ctx);
+      global_index_cache.store (ctx);
     }
   m_state->set (cooked_state::CACHE_DONE);
 }
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 86fbb8ffac4..fd7686205d5 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -716,8 +716,7 @@ class cooked_index : public dwarf_scanner_base
 private:
 
   /* Maybe write the index to the index cache.  */
-  void maybe_write_index (dwarf2_per_bfd *per_bfd,
-			  const index_cache_store_context &);
+  void maybe_write_index (const index_cache_store_context &);
 
   /* The vector of cooked_index objects.  This is stored because the
      entries are stored on the obstacks in those objects.  */
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index a269eb46a36..91f2af3e275 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -91,7 +91,8 @@ index_cache::disable ()
 index_cache_store_context::index_cache_store_context (const index_cache &ic,
 						      dwarf2_per_bfd *per_bfd)
   :  m_enabled (ic.enabled ()),
-     m_dir (ic.m_dir)
+     m_dir (ic.m_dir),
+     m_per_bfd (per_bfd)
 {
   if (!m_enabled)
     return;
@@ -154,8 +155,7 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
 /* See dwarf-index-cache.h.  */
 
 void
-index_cache::store (dwarf2_per_bfd *per_bfd,
-		    const index_cache_store_context &ctx)
+index_cache::store (const index_cache_store_context &ctx)
 {
   if (!ctx.m_enabled)
     return;
@@ -167,18 +167,18 @@ index_cache::store (dwarf2_per_bfd *per_bfd,
   try
     {
       index_cache_debug ("writing index cache for objfile %s",
-			 bfd_get_filename (per_bfd->obfd));
+			 bfd_get_filename (ctx.m_per_bfd->obfd));
 
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
-      write_dwarf_index (per_bfd, ctx.m_dir.c_str (),
+      write_dwarf_index (ctx.m_per_bfd, ctx.m_dir.c_str (),
 			 ctx.m_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",
-			 bfd_get_filename (per_bfd->obfd), except.what ());
+			 bfd_get_filename (ctx.m_per_bfd->obfd), except.what ());
     }
 }
 
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index f66f72caea1..09577aca257 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -51,6 +51,9 @@ struct index_cache_store_context
   /* Captured value of index cache directory.  */
   std::string m_dir;
 
+  /* The per-bfd object that we're caching.  */
+  dwarf2_per_bfd *m_per_bfd;
+
   /* Captured value of build id.  */
   std::string m_build_id_str;
 
@@ -80,8 +83,7 @@ class index_cache
   void disable ();
 
   /* Store an index for the specified object file in the cache.  */
-  void store (dwarf2_per_bfd *per_bfd,
-	      const index_cache_store_context &);
+  void store (const index_cache_store_context &);
 
   /* Look for an index file matching BUILD_ID.  If found, return the contents
      as an array_view and store the underlying resources (allocated memory,

-- 
2.43.0

[PATCH 4/5] Move the 'store' method to index_cache_store_context

[Tom Tromey]

I think it is cleaner for 'store' to be a method on
index_cache_store_context rather than on the global index cache
itself.  This patch makes this change.
---
 gdb/dwarf2/cooked-index.c |  2 +-
 gdb/dwarf2/index-cache.c  | 16 ++++++++--------
 gdb/dwarf2/index-cache.h  |  8 +++-----
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index c53b5129d9e..ad9fe871552 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -830,7 +830,7 @@ cooked_index::maybe_write_index (const index_cache_store_context &ctx)
   if (index_for_writing () != nullptr)
     {
       /* (maybe) store an index in the cache.  */
-      global_index_cache.store (ctx);
+      ctx.store ();
     }
   m_state->set (cooked_state::CACHE_DONE);
 }
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 91f2af3e275..780d2c4f200 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -155,30 +155,30 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
 /* See dwarf-index-cache.h.  */
 
 void
-index_cache::store (const index_cache_store_context &ctx)
+index_cache_store_context::store () const
 {
-  if (!ctx.m_enabled)
+  if (!m_enabled)
     return;
 
-  const char *dwz_build_id_ptr = (ctx.m_dwz_build_id_str.has_value ()
-				  ? ctx.m_dwz_build_id_str->c_str ()
+  const char *dwz_build_id_ptr = (m_dwz_build_id_str.has_value ()
+				  ? m_dwz_build_id_str->c_str ()
 				  : nullptr);
 
   try
     {
       index_cache_debug ("writing index cache for objfile %s",
-			 bfd_get_filename (ctx.m_per_bfd->obfd));
+			 bfd_get_filename (m_per_bfd->obfd));
 
       /* Write the index itself to the directory, using the build id as the
 	 filename.  */
-      write_dwarf_index (ctx.m_per_bfd, ctx.m_dir.c_str (),
-			 ctx.m_build_id_str.c_str (), dwz_build_id_ptr,
+      write_dwarf_index (m_per_bfd, m_dir.c_str (),
+			 m_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",
-			 bfd_get_filename (ctx.m_per_bfd->obfd), except.what ());
+			 bfd_get_filename (m_per_bfd->obfd), except.what ());
     }
 }
 
diff --git a/gdb/dwarf2/index-cache.h b/gdb/dwarf2/index-cache.h
index 09577aca257..95f217ed961 100644
--- a/gdb/dwarf2/index-cache.h
+++ b/gdb/dwarf2/index-cache.h
@@ -40,10 +40,11 @@ struct index_cache_resource
 
 struct index_cache_store_context
 {
-  friend class index_cache;
-
   index_cache_store_context (const index_cache &ic, dwarf2_per_bfd *per_bfd);
 
+  /* Store the index in the cache.  */
+  void store () const;
+
 private:
   /* Captured value of enabled ().  */
   bool m_enabled;
@@ -82,9 +83,6 @@ class index_cache
   /* Disable the cache.  */
   void disable ();
 
-  /* Store an index for the specified object file in the cache.  */
-  void store (const index_cache_store_context &);
-
   /* Look for an index file matching BUILD_ID.  If found, return the contents
      as an array_view and store the underlying resources (allocated memory,
      mapped file, etc) in RESOURCE.  The returned array_view is valid as long

-- 
2.43.0

[PATCH 5/5] Avoid race when writing to index cache

[Tom Tromey]

The background DWARF reader changes introduced a race when writing to
the index cache.  The problem here is that constructing the
index_cache_store_context object should only happen on the main
thread, to ensure that the various value captures do not race.

This patch adds an assert to the construct to that effect, and then
arranges for this object to be constructed by the cooked_index_worker
constructor -- which is only invoked on the main thread.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31262
---
 gdb/dwarf2/cooked-index.c | 27 ++++++++++++---------------
 gdb/dwarf2/cooked-index.h | 15 ++++++++++-----
 gdb/dwarf2/index-cache.c  |  4 ++++
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index ad9fe871552..25f3f132989 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -566,6 +566,15 @@ cooked_index_worker::set (cooked_state desired_state)
 #endif /* CXX_STD_THREAD */
 }
 
+/* See cooked-index.h.  */
+
+void
+cooked_index_worker::write_to_cache (const cooked_index *idx) const
+{
+  if (idx != nullptr)
+    m_cache_store.store ();
+}
+
 cooked_index::cooked_index (dwarf2_per_objfile *per_objfile,
 			    std::unique_ptr<cooked_index_worker> &&worker)
   : m_state (std::move (worker)),
@@ -609,17 +618,16 @@ cooked_index::set_contents (vec_type &&vec)
 
   m_state->set (cooked_state::MAIN_AVAILABLE);
 
-  index_cache_store_context ctx (global_index_cache, m_per_bfd);
-
   /* This is run after finalization is done -- but not before.  If
      this task were submitted earlier, it would have to wait for
      finalization.  However, that would take a slot in the global
      thread pool, and if enough such tasks were submitted at once, it
      would cause a livelock.  */
-  gdb::task_group finalizers ([this, ctx = std::move (ctx)] ()
+  gdb::task_group finalizers ([this] ()
   {
     m_state->set (cooked_state::FINALIZED);
-    maybe_write_index (ctx);
+    m_state->write_to_cache (index_for_writing ());
+    m_state->set (cooked_state::CACHE_DONE);
   });
 
   for (auto &idx : m_vector)
@@ -824,17 +832,6 @@ cooked_index::dump (gdbarch *arch)
     }
 }
 
-void
-cooked_index::maybe_write_index (const index_cache_store_context &ctx)
-{
-  if (index_for_writing () != nullptr)
-    {
-      /* (maybe) store an index in the cache.  */
-      ctx.store ();
-    }
-  m_state->set (cooked_state::CACHE_DONE);
-}
-
 /* Wait for all the index cache entries to be written before gdb
    exits.  */
 static void
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index fd7686205d5..a5fe479df14 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -511,7 +511,8 @@ class cooked_index_worker
 public:
 
   explicit cooked_index_worker (dwarf2_per_objfile *per_objfile)
-    : m_per_objfile (per_objfile)
+    : m_per_objfile (per_objfile),
+      m_cache_store (global_index_cache, per_objfile->per_bfd)
   { }
   virtual ~cooked_index_worker ()
   { }
@@ -530,10 +531,15 @@ class cooked_index_worker
 
 protected:
 
-  /* Let cooked_index call the 'set' method.  */
+  /* Let cooked_index call the 'set' and 'write_to_cache' methods.  */
   friend class cooked_index;
+
+  /* Set the current state.  */
   void set (cooked_state desired_state);
 
+  /* Write to the index cache.  */
+  void write_to_cache (const cooked_index *idx) const;
+
   /* Helper function that does the work of reading.  This must be able
      to be run in a worker thread without problems.  */
   virtual void do_reading () = 0;
@@ -578,6 +584,8 @@ class cooked_index_worker
      scanning is stopped and this exception will later be reported by
      the 'wait' method.  */
   std::optional<gdb_exception> m_failed;
+  /* An object used to write to the index cache.  */
+  index_cache_store_context m_cache_store;
 };
 
 /* The main index of DIEs.
@@ -715,9 +723,6 @@ class cooked_index : public dwarf_scanner_base
 
 private:
 
-  /* Maybe write the index to the index cache.  */
-  void maybe_write_index (const index_cache_store_context &);
-
   /* The vector of cooked_index objects.  This is stored because the
      entries are stored on the obstacks in those objects.  */
   vec_type m_vector;
diff --git a/gdb/dwarf2/index-cache.c b/gdb/dwarf2/index-cache.c
index 780d2c4f200..d9047ef46bb 100644
--- a/gdb/dwarf2/index-cache.c
+++ b/gdb/dwarf2/index-cache.c
@@ -33,6 +33,7 @@
 #include "gdbsupport/selftest.h"
 #include <string>
 #include <stdlib.h>
+#include "run-on-main-thread.h"
 
 /* When set to true, show debug messages about the index cache.  */
 static bool debug_index_cache = false;
@@ -94,6 +95,9 @@ index_cache_store_context::index_cache_store_context (const index_cache &ic,
      m_dir (ic.m_dir),
      m_per_bfd (per_bfd)
 {
+  /* Capturing globals may only be done on the main thread.  */
+  gdb_assert (is_main_thread ());
+
   if (!m_enabled)
     return;
 

-- 
2.43.0

Re: [PATCH 0/5] Fix race when writing to index cache

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This series is mostly cleanup, in preparation for the final patch that
Tom> fixes a race when writing to the index cache.

I'm checking these in.

Tom