From 4f78c4f67b1e0901a97a828634b9cd3ddc70ca40 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Mon, 22 Oct 2018 20:14:36 +0000 Subject: [PATCH] [SymbolFile] Add the module lock where necessary and assert that we own it. As discussed with Greg at the dev meeting, we need to ensure we have the module lock in the SymbolFile. Usually the symbol file is accessed through the symbol vendor which ensures that the necessary locks are taken. However, there are a few methods that are accessed by the expression parser and were lacking the lock. This patch adds the locking where necessary and everywhere else asserts that we actually already own the lock. Differential revision: https://reviews.llvm.org/D52543 llvm-svn: 344945 --- lldb/include/lldb/Symbol/SymbolFile.h | 16 +++++++ .../SymbolFile/DWARF/SymbolFileDWARF.cpp | 47 +++++++++++++++++-- .../SymbolFile/DWARF/SymbolFileDWARF.h | 2 + lldb/source/Symbol/SymbolFile.cpp | 20 ++++++++ 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/lldb/include/lldb/Symbol/SymbolFile.h b/lldb/include/lldb/Symbol/SymbolFile.h index 61dffbfd2014..c6f16c19bd02 100644 --- a/lldb/include/lldb/Symbol/SymbolFile.h +++ b/lldb/include/lldb/Symbol/SymbolFile.h @@ -20,6 +20,14 @@ #include "llvm/ADT/DenseSet.h" +#include + +#if defined(LLDB_CONFIGURATION_DEBUG) +#define ASSERT_MODULE_LOCK(expr) (expr->AssertModuleLock();) +#else +#define ASSERT_MODULE_LOCK(expr) ((void)0) +#endif + namespace lldb_private { class SymbolFile : public PluginInterface { @@ -93,6 +101,12 @@ public: virtual uint32_t CalculateAbilities() = 0; + //------------------------------------------------------------------ + /// Symbols file subclasses should override this to return the Module that + /// owns the TypeSystem that this symbol file modifies type information in. + //------------------------------------------------------------------ + virtual std::recursive_mutex &GetModuleMutex() const; + //------------------------------------------------------------------ /// Initialize the SymbolFile object. /// @@ -208,6 +222,8 @@ public: virtual void Dump(Stream &s) {} protected: + void AssertModuleLock(); + ObjectFile *m_obj_file; // The object file that symbols can be extracted from. uint32_t m_abilities; bool m_calculated_abilities; diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp index 7ef21b6e4c20..563305028e23 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp @@ -260,6 +260,9 @@ SymbolFile *SymbolFileDWARF::CreateInstance(ObjectFile *obj_file) { } TypeList *SymbolFileDWARF::GetTypeList() { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile(); if (debug_map_symfile) return debug_map_symfile->GetTypeList(); @@ -341,6 +344,7 @@ size_t SymbolFileDWARF::GetTypes(SymbolContextScope *sc_scope, uint32_t type_mask, TypeList &type_list) { + ASSERT_MODULE_LOCK(this); TypeSet type_set; CompileUnit *comp_unit = NULL; @@ -860,6 +864,7 @@ uint32_t SymbolFileDWARF::GetNumCompileUnits() { } CompUnitSP SymbolFileDWARF::ParseCompileUnitAtIndex(uint32_t cu_idx) { + ASSERT_MODULE_LOCK(this); CompUnitSP cu_sp; DWARFDebugInfo *info = DebugInfo(); if (info) { @@ -872,6 +877,7 @@ CompUnitSP SymbolFileDWARF::ParseCompileUnitAtIndex(uint32_t cu_idx) { Function *SymbolFileDWARF::ParseCompileUnitFunction(const SymbolContext &sc, const DWARFDIE &die) { + ASSERT_MODULE_LOCK(this); if (die.IsValid()) { TypeSystem *type_system = GetTypeSystemForLanguage(die.GetCU()->GetLanguageType()); @@ -895,6 +901,7 @@ bool SymbolFileDWARF::FixupAddress(Address &addr) { } lldb::LanguageType SymbolFileDWARF::ParseCompileUnitLanguage(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); if (dwarf_cu) @@ -904,6 +911,7 @@ SymbolFileDWARF::ParseCompileUnitLanguage(const SymbolContext &sc) { } size_t SymbolFileDWARF::ParseCompileUnitFunctions(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); size_t functions_added = 0; DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); @@ -926,6 +934,7 @@ size_t SymbolFileDWARF::ParseCompileUnitFunctions(const SymbolContext &sc) { bool SymbolFileDWARF::ParseCompileUnitSupportFiles( const SymbolContext &sc, FileSpecList &support_files) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); if (dwarf_cu) { @@ -951,6 +960,7 @@ bool SymbolFileDWARF::ParseCompileUnitSupportFiles( bool SymbolFileDWARF::ParseCompileUnitIsOptimized( const lldb_private::SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); if (dwarf_cu) return dwarf_cu->GetIsOptimized(); @@ -960,6 +970,7 @@ bool SymbolFileDWARF::ParseCompileUnitIsOptimized( bool SymbolFileDWARF::ParseImportedModules( const lldb_private::SymbolContext &sc, std::vector &imported_modules) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); if (dwarf_cu) { @@ -1037,6 +1048,7 @@ static void ParseDWARFLineTableCallback(dw_offset_t offset, } bool SymbolFileDWARF::ParseCompileUnitLineTable(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); if (sc.comp_unit->GetLineTable() != NULL) return true; @@ -1122,6 +1134,7 @@ SymbolFileDWARF::ParseDebugMacros(lldb::offset_t *offset) { } bool SymbolFileDWARF::ParseCompileUnitDebugMacros(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit); DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); @@ -1301,6 +1314,9 @@ void SymbolFileDWARF::ParseDeclsForContext(CompilerDeclContext decl_ctx) { } SymbolFileDWARF *SymbolFileDWARF::GetDWARFForUID(lldb::user_id_t uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we // must make sure we use the correct DWARF file when resolving things. On // MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple @@ -1317,6 +1333,9 @@ SymbolFileDWARF *SymbolFileDWARF::GetDWARFForUID(lldb::user_id_t uid) { DWARFDIE SymbolFileDWARF::GetDIEFromUID(lldb::user_id_t uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we get a "lldb::user_id_t" from an lldb_private::SymbolFile API we // must make sure we use the correct DWARF file when resolving things. On // MacOSX, when using SymbolFileDWARFDebugMap, we will use multiple @@ -1331,6 +1350,9 @@ SymbolFileDWARF::GetDIEFromUID(lldb::user_id_t uid) { } CompilerDecl SymbolFileDWARF::GetDeclForUID(lldb::user_id_t type_uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we have a lldb::user_id_t, we must get the DIE by calling // SymbolFileDWARF::GetDIEFromUID(). See comments inside the // SymbolFileDWARF::GetDIEFromUID() for details. @@ -1342,6 +1364,9 @@ CompilerDecl SymbolFileDWARF::GetDeclForUID(lldb::user_id_t type_uid) { CompilerDeclContext SymbolFileDWARF::GetDeclContextForUID(lldb::user_id_t type_uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we have a lldb::user_id_t, we must get the DIE by calling // SymbolFileDWARF::GetDIEFromUID(). See comments inside the // SymbolFileDWARF::GetDIEFromUID() for details. @@ -1353,6 +1378,9 @@ SymbolFileDWARF::GetDeclContextForUID(lldb::user_id_t type_uid) { CompilerDeclContext SymbolFileDWARF::GetDeclContextContainingUID(lldb::user_id_t type_uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we have a lldb::user_id_t, we must get the DIE by calling // SymbolFileDWARF::GetDIEFromUID(). See comments inside the // SymbolFileDWARF::GetDIEFromUID() for details. @@ -1363,6 +1391,9 @@ SymbolFileDWARF::GetDeclContextContainingUID(lldb::user_id_t type_uid) { } Type *SymbolFileDWARF::ResolveTypeUID(lldb::user_id_t type_uid) { + // This method can be called without going through the symbol vendor so we + // need to lock the module. + std::lock_guard guard(GetModuleMutex()); // Anytime we have a lldb::user_id_t, we must get the DIE by calling // SymbolFileDWARF::GetDIEFromUID(). See comments inside the // SymbolFileDWARF::GetDIEFromUID() for details. @@ -1438,8 +1469,7 @@ bool SymbolFileDWARF::HasForwardDeclForClangType( } bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) { - std::lock_guard guard( - GetObjectFile()->GetModule()->GetMutex()); + std::lock_guard guard(GetModuleMutex()); ClangASTContext *clang_type_system = llvm::dyn_cast_or_null(compiler_type.GetTypeSystem()); @@ -1984,11 +2014,17 @@ uint32_t SymbolFileDWARF::ResolveSymbolContext(const FileSpec &file_spec, } void SymbolFileDWARF::PreloadSymbols() { - std::lock_guard guard( - GetObjectFile()->GetModule()->GetMutex()); + std::lock_guard guard(GetModuleMutex()); m_index->Preload(); } +std::recursive_mutex &SymbolFileDWARF::GetModuleMutex() const { + lldb::ModuleSP module_sp(m_debug_map_module_wp.lock()); + if (module_sp) + return module_sp->GetMutex(); + return GetObjectFile()->GetModule()->GetMutex(); +} + bool SymbolFileDWARF::DeclContextMatchesThisSymbolFile( const lldb_private::CompilerDeclContext *decl_ctx) { if (decl_ctx == nullptr || !decl_ctx->IsValid()) { @@ -3075,6 +3111,7 @@ size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc, } size_t SymbolFileDWARF::ParseFunctionBlocks(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); assert(sc.comp_unit && sc.function); size_t functions_added = 0; DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit); @@ -3091,6 +3128,7 @@ size_t SymbolFileDWARF::ParseFunctionBlocks(const SymbolContext &sc) { } size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); // At least a compile unit must be valid assert(sc.comp_unit); size_t types_added = 0; @@ -3114,6 +3152,7 @@ size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc) { } size_t SymbolFileDWARF::ParseVariablesForContext(const SymbolContext &sc) { + ASSERT_MODULE_LOCK(this); if (sc.comp_unit != NULL) { DWARFDebugInfo *info = DebugInfo(); if (info == NULL) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h index b3d087f08127..d2ec3eeba2e6 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h @@ -228,6 +228,8 @@ public: void PreloadSymbols() override; + std::recursive_mutex &GetModuleMutex() const override; + //------------------------------------------------------------------ // PluginInterface protocol //------------------------------------------------------------------ diff --git a/lldb/source/Symbol/SymbolFile.cpp b/lldb/source/Symbol/SymbolFile.cpp index 6b4da9c53009..4bf7e5bacb91 100644 --- a/lldb/source/Symbol/SymbolFile.cpp +++ b/lldb/source/Symbol/SymbolFile.cpp @@ -19,12 +19,18 @@ #include "lldb/Utility/StreamString.h" #include "lldb/lldb-private.h" +#include + using namespace lldb_private; void SymbolFile::PreloadSymbols() { // No-op for most implementations. } +std::recursive_mutex &SymbolFile::GetModuleMutex() const { + return GetObjectFile()->GetModule()->GetMutex(); +} + SymbolFile *SymbolFile::FindPlugin(ObjectFile *obj_file) { std::unique_ptr best_symfile_ap; if (obj_file != nullptr) { @@ -150,3 +156,17 @@ size_t SymbolFile::FindTypes(const std::vector &context, types.Clear(); return 0; } + +void SymbolFile::AssertModuleLock() { + // The code below is too expensive to leave enabled in release builds. It's + // enabled in debug builds or when the correct macro is set. +#if defined(LLDB_CONFIGURATION_DEBUG) + // We assert that we have to module lock by trying to acquire the lock from a + // different thread. Note that we must abort if the result is true to + // guarantee correctness. + assert(std::async(std::launch::async, + [this] { return this->GetModuleMutex().try_lock(); }) + .get() == false && + "Module is not locked"); +#endif +}