Skip to content

Commit

Permalink
[SymbolFile] Add the module lock where necessary and assert that we o…
Browse files Browse the repository at this point in the history 
…wn 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
  • Loading branch information
@JDevlieghere
JDevlieghere committed Oct 22, 2018
1 parent 7e4edbd commit 4f78c4f
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 4 deletions.
16 changes: 16 additions & 0 deletions lldb/include/lldb/Symbol/SymbolFile.h
View file
Expand Up @@ -20,6 +20,14 @@

#include "llvm/ADT/DenseSet.h"

#include <mutex>

#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 {
Expand Down Expand Up @@ -93,6 +101,12 @@ class SymbolFile : public PluginInterface {

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.
///
Expand Down Expand Up @@ -208,6 +222,8 @@ class SymbolFile : public PluginInterface {
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;
Expand Down
47 changes: 43 additions & 4 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
View file
Expand Up @@ -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<std::recursive_mutex> guard(GetModuleMutex());
SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
if (debug_map_symfile)
return debug_map_symfile->GetTypeList();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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());
Expand All @@ -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)
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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();
Expand All @@ -960,6 +970,7 @@ bool SymbolFileDWARF::ParseCompileUnitIsOptimized(
bool SymbolFileDWARF::ParseImportedModules(
const lldb_private::SymbolContext &sc,
std::vector<lldb_private::ConstString> &imported_modules) {
ASSERT_MODULE_LOCK(this);
assert(sc.comp_unit);
DWARFUnit *dwarf_cu = GetDWARFCompileUnit(sc.comp_unit);
if (dwarf_cu) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<std::recursive_mutex> 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
Expand All @@ -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<std::recursive_mutex> 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
Expand All @@ -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<std::recursive_mutex> 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.
Expand All @@ -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<std::recursive_mutex> 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.
Expand All @@ -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<std::recursive_mutex> 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.
Expand All @@ -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<std::recursive_mutex> 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.
Expand Down Expand Up @@ -1438,8 +1469,7 @@ bool SymbolFileDWARF::HasForwardDeclForClangType(
}

bool SymbolFileDWARF::CompleteType(CompilerType &compiler_type) {
std::lock_guard<std::recursive_mutex> guard(
GetObjectFile()->GetModule()->GetMutex());
std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());

ClangASTContext *clang_type_system =
llvm::dyn_cast_or_null<ClangASTContext>(compiler_type.GetTypeSystem());
Expand Down Expand Up @@ -1984,11 +2014,17 @@ uint32_t SymbolFileDWARF::ResolveSymbolContext(const FileSpec &file_spec,
}

void SymbolFileDWARF::PreloadSymbols() {
std::lock_guard<std::recursive_mutex> guard(
GetObjectFile()->GetModule()->GetMutex());
std::lock_guard<std::recursive_mutex> 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()) {
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
View file
Expand Up @@ -228,6 +228,8 @@ class SymbolFileDWARF : public lldb_private::SymbolFile,

void PreloadSymbols() override;

std::recursive_mutex &GetModuleMutex() const override;

//------------------------------------------------------------------
// PluginInterface protocol
//------------------------------------------------------------------
Expand Down
20 changes: 20 additions & 0 deletions lldb/source/Symbol/SymbolFile.cpp
View file
Expand Up @@ -19,12 +19,18 @@
#include "lldb/Utility/StreamString.h"
#include "lldb/lldb-private.h"

#include <future>

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<SymbolFile> best_symfile_ap;
if (obj_file != nullptr) {
Expand Down Expand Up @@ -150,3 +156,17 @@ size_t SymbolFile::FindTypes(const std::vector<CompilerContext> &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
}

0 comments on commit 4f78c4f

Please sign in to comment.