gdb/dwarf: split dwo_lock in more granular locks

The dwo_lock mutex is used to synchronize access to some dwo/dwp-related
data structures, such as dwarf2_per_bfd::dwo_files and
dwp_file::loaded_{cus,tus}.  Right now the scope of the lock is kind of
coarse.  It is taken in the top-level lookup_dwo_unit function, and held
while the thread:

 - looks up an existing dwo_file in the per-bfd hash table for the given
   id/signature
 - if there's no existing dwo_file, attempt to find a .dwo file, open
   it, build the list of units it contains
 - if a new dwo_file was created, insert it in the per-bfd hash table
 - look up the desired unit in the dwo_file

And something similar for the dwp code path.  This means that two
indexing thread can't read in two dwo files simultaneously.  This isn't
ideal in terms of parallelism.

This patch breaks this lock into 3 more fine grained locks:

 - one lock to access dwarf2_per_bfd::dwo_files
 - one lock to access dwp_file::loaded_{cus,tus}
 - one lock in try_open_dwop_file, where we do two operations that
   aren't thread safe (bfd_check_format and gdb_bfd_record_inclusion)

Unfortunately I don't see a clear speedup on my computer with 8 threads.
But the change shouldn't hurt, in theory, and hopefully this can be a
piece that helps in making GDB scale better on machines with many cores
(if we ever bump the max number of worker threads).

This patch uses "double-checked locking" to avoid holding the lock(s)
for the whole duration of reading in dwo files.  The idea is, when
looking up a dwo with a given name:

 - with the lock held, check for an existing dwo_file with that name in
   dwarf2_per_bfd::dwo_files, if found return it
 - if not found, drop the lock, load the dwo file and create a dwo_file
   describing it
 - with the lock held, attempt to insert the new dwo_file in
   dwarf2_per_bfd::dwo_files.  If an entry exists, it means another
   thread simultaneously created an equivalent dwo_file, but won the
   race.  Drop the new dwo_file and use the existing one.  The new
   dwo_file is automatically deleted, because it is help by a unique_ptr
   and the insertion into the unordered_set fails.

Note that it shouldn't normally happen for two threads to look up a dwo
file with the same name, since different units will point to different
dwo files.  But it were to happen, we handle it.  This way of doing
things allows two threads to read in two different dwo files
simulatenously, which in theory should help get better parallelism.  The
same technique is used for dwp_file::loaded_{cus,tus}.

I have some local CI jobs that run the fission and fission-dwp boards,
and I haven't seen regressions.  In addition to the regular testing, I
ran a few tests using those boards on a ThreadSanitizer build of GDB.

Change-Id: I625c98b0aa97b47d5ee59fe22a137ad0eafc8c25
Reviewed-By: Andrew Burgess <aburgess@redhat.com>

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b1849b9b071d34645cbcc2b9a74c0f0487e51bab..c02b9cffbc06ed8015adf0a791ea0793b96059a7 100644 (file)
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -604,6 +604,11 @@ struct dwp_file
   dwo_unit_set loaded_cus;
   dwo_unit_set loaded_tus;
 
+#if CXX_STD_THREAD
+  /* Mutex to synchronize access to LOADED_CUS and LOADED_TUS.  */
+  std::mutex loaded_cutus_lock;
+#endif
+
   /* Table to map ELF section numbers to their sections.
      This is only needed for the DWP V1 file format.  */
   unsigned int num_sections = 0;
@@ -2782,13 +2787,6 @@ dwo_unit *
 cutu_reader::lookup_dwo_unit (dwarf2_cu *cu, die_info *comp_unit_die,
                              const char *dwo_name)
 {
-#if CXX_STD_THREAD
-  /* We need a lock here to handle the DWO hash table.  */
-  static std::mutex dwo_lock;
-
-  std::lock_guard<std::mutex> guard (dwo_lock);
-#endif
-
   dwarf2_per_cu *per_cu = cu->per_cu;
   struct dwo_unit *dwo_unit;
   const char *comp_dir;
@@ -6205,11 +6203,33 @@ static dwo_file *
 lookup_dwo_file (dwarf2_per_bfd *per_bfd, const char *dwo_name,
                 const char *comp_dir)
 {
+#if CXX_STD_THREAD
+  std::lock_guard<std::mutex> guard (per_bfd->dwo_files_lock);
+#endif
+
   auto it = per_bfd->dwo_files.find (dwo_file_search {dwo_name, comp_dir});
 
   return it != per_bfd->dwo_files.end () ? it->get() : nullptr;
 }
 
+/* Add DWO_FILE to the per-BFD DWO file hash table.
+
+   Return the dwo_file actually kept in the hash table.
+
+   If another thread raced with this one, opening the exact same DWO file and
+   inserting it first in the hash table, then keep that other thread's copy
+   and DWO_FILE gets freed.  */
+
+static dwo_file *
+add_dwo_file (dwarf2_per_bfd *per_bfd, dwo_file_up dwo_file)
+{
+#if CXX_STD_THREAD
+  std::lock_guard<std::mutex> lock (per_bfd->dwo_files_lock);
+#endif
+
+  return per_bfd->dwo_files.emplace (std::move (dwo_file)).first->get ();
+}
+
 void
 cutu_reader::create_dwo_unit_hash_tables (dwo_file &dwo_file,
                                          dwarf2_cu &skeleton_cu,
@@ -6929,10 +6949,7 @@ create_dwo_unit_in_dwp_v1 (dwarf2_per_bfd *per_bfd,
         types we'll grow the vector and eventually have to reallocate space
         for it, invalidating all copies of pointers into the previous
         contents.  */
-      auto [it, inserted]
-       = per_bfd->dwo_files.emplace (std::move (new_dwo_file));
-      gdb_assert (inserted);
-      dwo_file = it->get ();
+      dwo_file = add_dwo_file (per_bfd, std::move (new_dwo_file));
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -7135,10 +7152,7 @@ create_dwo_unit_in_dwp_v2 (dwarf2_per_bfd *per_bfd,
         types we'll grow the vector and eventually have to reallocate space
         for it, invalidating all copies of pointers into the previous
         contents.  */
-      auto [it, inserted]
-       = per_bfd->dwo_files.emplace (std::move (new_dwo_file));
-      gdb_assert (inserted);
-      dwo_file = it->get ();
+      dwo_file = add_dwo_file (per_bfd, std::move (new_dwo_file));
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -7306,10 +7320,7 @@ create_dwo_unit_in_dwp_v5 (dwarf2_per_bfd *per_bfd,
         types we'll grow the vector and eventually have to reallocate space
         for it, invalidating all copies of pointers into the previous
         contents.  */
-      auto [it, inserted]
-       = per_bfd->dwo_files.emplace (std::move (new_dwo_file));
-      gdb_assert (inserted);
-      dwo_file = it->get ();
+      dwo_file = add_dwo_file (per_bfd, std::move (new_dwo_file));
     }
   else
     dwarf_read_debug_printf ("Using existing virtual DWO: %s",
@@ -7346,9 +7357,15 @@ lookup_dwo_unit_in_dwp (dwarf2_per_bfd *per_bfd,
   auto &dwo_unit_set
     = is_debug_types ? dwp_file->loaded_tus : dwp_file->loaded_cus;
 
-  if (auto it = dwo_unit_set.find (signature);
-      it != dwo_unit_set.end ())
-    return it->get ();
+  {
+#if CXX_STD_THREAD
+    std::lock_guard<std::mutex> guard (dwp_file->loaded_cutus_lock);
+#endif
+
+    if (auto it = dwo_unit_set.find (signature);
+       it != dwo_unit_set.end ())
+      return it->get ();
+  }
 
   /* Use a for loop so that we don't loop forever on bad debug info.  */
   for (unsigned int i = 0; i < dwp_htab->nr_slots; ++i)
@@ -7377,8 +7394,13 @@ lookup_dwo_unit_in_dwp (dwarf2_per_bfd *per_bfd,
              = create_dwo_unit_in_dwp_v5 (per_bfd, dwp_file, unit_index,
                                           comp_dir, signature, is_debug_types);
 
+         /* If another thread raced with this one, opening the exact same
+            DWO unit, then we'll keep that other thread's copy.  */
+#if CXX_STD_THREAD
+         std::lock_guard<std::mutex> guard (dwp_file->loaded_cutus_lock);
+#endif
+
          auto [it, inserted] = dwo_unit_set.emplace (std::move (dwo_unit));
-         gdb_assert (inserted);
          return it->get ();
        }
 
@@ -7456,14 +7478,23 @@ try_open_dwop_file (dwarf2_per_bfd *per_bfd, const char *file_name, int is_dwp,
   if (sym_bfd == NULL)
     return NULL;
 
-  if (!bfd_check_format (sym_bfd.get (), bfd_object))
-    return NULL;
+  {
+#if CXX_STD_THREAD
+    /* The operations below are not thread-safe, use a lock to synchronize
+       concurrent accesses.  */
+    static std::mutex mutex;
+    std::lock_guard<std::mutex> lock (mutex);
+#endif
 
-  /* Success.  Record the bfd as having been included by the objfile's bfd.
+    if (!bfd_check_format (sym_bfd.get (), bfd_object))
+      return NULL;
+
+    /* Success.  Record the bfd as having been included by the objfile's bfd.
      This is important because things like demangled_names_hash lives in the
      objfile's per_bfd space and may have references to things like symbol
      names that live in the DWO/DWP file's per_bfd space.  PR 16426.  */
-  gdb_bfd_record_inclusion (per_bfd->obfd, sym_bfd.get ());
+    gdb_bfd_record_inclusion (per_bfd->obfd, sym_bfd.get ());
+  }
 
   return sym_bfd;
 }
@@ -7958,12 +7989,7 @@ cutu_reader::lookup_dwo_cutu (dwarf2_cu *cu, const char *dwo_name,
            = open_and_init_dwo_file (cu, dwo_name, comp_dir);
 
          if (new_dwo_file != nullptr)
-           {
-             auto [it, inserted]
-               = per_bfd->dwo_files.emplace (std::move (new_dwo_file));
-             gdb_assert (inserted);
-             dwo_file = (*it).get ();
-           }
+           dwo_file = add_dwo_file (per_bfd, std::move (new_dwo_file));
        }
 
       if (dwo_file != NULL)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index aaac5e7cbbfc6ae1dab321c89fa2918c8e45b66b..e676d64f2e981bcbaec2bdc933fa84fdd0d5b7ef 100644 (file)
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -634,6 +634,11 @@ public:
   /* Set of dwo_file objects.  */
   dwo_file_up_set dwo_files;
 
+#if CXX_STD_THREAD
+  /* Mutex to synchronize access to DWO_FILES.  */
+  std::mutex dwo_files_lock;
+#endif
+
   /* The DWP file if there is one, or NULL.  */
   dwp_file_up dwp_file;