From fbf19b6cc611f08b41cd7ef2e524b8f64d8ffbb8 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 12 May 2025 15:09:45 -0400 Subject: [PATCH] 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 --- gdb/dwarf2/read.c | 92 ++++++++++++++++++++++++++++++----------------- gdb/dwarf2/read.h | 5 +++ 2 files changed, 64 insertions(+), 33 deletions(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index b1849b9b071..c02b9cffbc0 100644 --- 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 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 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 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 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 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 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 aaac5e7cbbf..e676d64f2e9 100644 --- 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; -- 2.39.5