friss created this revision.
friss added a reviewer: clayborg.
Before a Debugger gets a Target, target settings are routed to a global set
of settings. Even without this, some part of the LLDB which exist independently
of the Debugger object (the Module cache, the Symbol vendors, ...) access
directly the global default store for those settings.
Of course, if you modify one of those global settings while they are being read,
bad things happen. We see this quite a bit with FileSpecList settings. In
particular, we see many cases where one debug session changes
target.exec-search-paths while another session starts up and it crashes when
one of those accesses invalid FileSpecs.
This patch addresses the specific FileSpecList issue by adding locking to
OptionValueFileSpecList and never returning by reference. It's not a general
solution, and I'd like to hear your ideas about how to address this in a more
principled way, but given global properties are a thing, it seems like we
need locking at this level even if we were to do a bigger refectoring.
https://reviews.llvm.org/D60468
Files:
include/lldb/Interpreter/OptionValueFileSpecList.h
include/lldb/Target/Target.h
include/lldb/Target/Thread.h
source/Commands/CommandObjectTarget.cpp
source/Interpreter/OptionValueFileSpecLIst.cpp
source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
source/Target/Target.cpp
source/Target/TargetList.cpp
source/Target/Thread.cpp
Index: source/Target/Thread.cpp
===================================================================
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -138,9 +138,9 @@
return m_collection_sp->GetPropertyAtIndexAsOptionValueRegex(nullptr, idx);
}
-FileSpecList &ThreadProperties::GetLibrariesToAvoid() const {
+FileSpecList ThreadProperties::GetLibrariesToAvoid() const {
const uint32_t idx = ePropertyStepAvoidLibraries;
- OptionValueFileSpecList *option_value =
+ const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
false, idx);
assert(option_value);
Index: source/Target/TargetList.cpp
===================================================================
--- source/Target/TargetList.cpp
+++ source/Target/TargetList.cpp
@@ -426,7 +426,7 @@
if (file.GetDirectory()) {
FileSpec file_dir;
file_dir.GetDirectory() = file.GetDirectory();
- target_sp->GetExecutableSearchPaths().Append(file_dir);
+ target_sp->AppendExecutableSearchPaths(file_dir);
}
// Don't put the dummy target in the target list, it's held separately.
Index: source/Target/Target.cpp
===================================================================
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1569,8 +1569,9 @@
arch_spec.GetArchitectureName(),
arch_spec.GetTriple().getTriple().c_str());
ModuleSpec module_spec(executable_sp->GetFileSpec(), other);
+ FileSpecList search_paths = GetExecutableSearchPaths();
Status error = ModuleList::GetSharedModule(module_spec, executable_sp,
- &GetExecutableSearchPaths(),
+ &search_paths,
nullptr, nullptr);
if (!error.Fail() && executable_sp) {
@@ -2001,7 +2002,7 @@
ModuleSP old_module_sp; // This will get filled in if we have a new version
// of the library
bool did_create_module = false;
-
+ FileSpecList search_paths = GetExecutableSearchPaths();
// If there are image search path entries, try to use them first to acquire
// a suitable image.
if (m_image_search_paths.GetSize()) {
@@ -2012,7 +2013,7 @@
transformed_spec.GetFileSpec().GetFilename() =
module_spec.GetFileSpec().GetFilename();
error = ModuleList::GetSharedModule(transformed_spec, module_sp,
- &GetExecutableSearchPaths(),
+ &search_paths,
&old_module_sp, &did_create_module);
}
}
@@ -2029,7 +2030,7 @@
if (module_spec.GetUUID().IsValid()) {
// We have a UUID, it is OK to check the global module list...
error = ModuleList::GetSharedModule(module_spec, module_sp,
- &GetExecutableSearchPaths(),
+ &search_paths,
&old_module_sp, &did_create_module);
}
@@ -2039,7 +2040,7 @@
if (m_platform_sp) {
error = m_platform_sp->GetSharedModule(
module_spec, m_process_sp.get(), module_sp,
- &GetExecutableSearchPaths(), &old_module_sp, &did_create_module);
+ &search_paths, &old_module_sp, &did_create_module);
} else {
error.SetErrorString("no platform is currently set");
}
@@ -3858,27 +3859,36 @@
return option_value->GetCurrentValue();
}
-FileSpecList &TargetProperties::GetExecutableSearchPaths() {
+void TargetProperties::AppendExecutableSearchPaths(const FileSpec& dir) {
const uint32_t idx = ePropertyExecutableSearchPaths;
OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
false, idx);
assert(option_value);
+ option_value->AppendCurrentValue(dir);
+}
+
+FileSpecList TargetProperties::GetExecutableSearchPaths() {
+ const uint32_t idx = ePropertyExecutableSearchPaths;
+ const OptionValueFileSpecList *option_value =
+ m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
+ false, idx);
+ assert(option_value);
return option_value->GetCurrentValue();
}
-FileSpecList &TargetProperties::GetDebugFileSearchPaths() {
+FileSpecList TargetProperties::GetDebugFileSearchPaths() {
const uint32_t idx = ePropertyDebugFileSearchPaths;
- OptionValueFileSpecList *option_value =
+ const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
false, idx);
assert(option_value);
return option_value->GetCurrentValue();
}
-FileSpecList &TargetProperties::GetClangModuleSearchPaths() {
+FileSpecList TargetProperties::GetClangModuleSearchPaths() {
const uint32_t idx = ePropertyClangModuleSearchPaths;
- OptionValueFileSpecList *option_value =
+ const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(nullptr,
false, idx);
assert(option_value);
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===================================================================
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -84,7 +84,7 @@
static lldb_private::SymbolFile *
CreateInstance(lldb_private::ObjectFile *obj_file);
- static const lldb_private::FileSpecList &GetSymlinkPaths();
+ static lldb_private::FileSpecList GetSymlinkPaths();
//------------------------------------------------------------------
// Constructors and Destructors
Index: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===================================================================
--- source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -136,8 +136,8 @@
m_collection_sp->Initialize(g_properties);
}
- FileSpecList &GetSymLinkPaths() {
- OptionValueFileSpecList *option_value =
+ FileSpecList GetSymLinkPaths() {
+ const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(
nullptr, true, ePropertySymLinkPaths);
assert(option_value);
@@ -159,7 +159,7 @@
} // anonymous namespace end
-const FileSpecList &SymbolFileDWARF::GetSymlinkPaths() {
+FileSpecList SymbolFileDWARF::GetSymlinkPaths() {
return GetGlobalPluginProperties()->GetSymLinkPaths();
}
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===================================================================
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -214,9 +214,9 @@
NULL, idx, g_properties[idx].default_uint_value != 0);
}
- FileSpecList &GetKextDirectories() const {
+ FileSpecList GetKextDirectories() const {
const uint32_t idx = ePropertyKextDirectories;
- OptionValueFileSpecList *option_value =
+ const OptionValueFileSpecList *option_value =
m_collection_sp->GetPropertyAtIndexAsOptionValueFileSpecList(
NULL, false, idx);
assert(option_value);
Index: source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===================================================================
--- source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -608,7 +608,7 @@
compiler_invocation_arguments.push_back(module_cache_argument);
}
- FileSpecList &module_search_paths = target.GetClangModuleSearchPaths();
+ FileSpecList module_search_paths = target.GetClangModuleSearchPaths();
for (size_t spi = 0, spe = module_search_paths.GetSize(); spi < spe; ++spi) {
const FileSpec &search_path = module_search_paths.GetFileSpecAtIndex(spi);
Index: source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===================================================================
--- source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -828,10 +828,11 @@
if (platform_name == g_platform_name) {
ModuleSpec kext_bundle_module_spec(module_spec);
FileSpec kext_filespec(m_name.c_str());
+ FileSpecList search_paths = target.GetExecutableSearchPaths();
kext_bundle_module_spec.GetFileSpec() = kext_filespec;
platform_sp->GetSharedModule(
kext_bundle_module_spec, process, m_module_sp,
- &target.GetExecutableSearchPaths(), NULL, NULL);
+ &search_paths, NULL, NULL);
}
}
Index: source/Interpreter/OptionValueFileSpecLIst.cpp
===================================================================
--- source/Interpreter/OptionValueFileSpecLIst.cpp
+++ source/Interpreter/OptionValueFileSpecLIst.cpp
@@ -163,5 +163,6 @@
}
lldb::OptionValueSP OptionValueFileSpecList::DeepCopy() const {
- return OptionValueSP(new OptionValueFileSpecList(*this));
+ std::lock_guard<std::mutex> lock(m_mutex);
+ return OptionValueSP(new OptionValueFileSpecList(m_current_value));
}
Index: source/Commands/CommandObjectTarget.cpp
===================================================================
--- source/Commands/CommandObjectTarget.cpp
+++ source/Commands/CommandObjectTarget.cpp
@@ -411,7 +411,7 @@
}
FileSpec core_file_dir;
core_file_dir.GetDirectory() = core_file.GetDirectory();
- target_sp->GetExecutableSearchPaths().Append(core_file_dir);
+ target_sp->AppendExecutableSearchPaths(core_file_dir);
ProcessSP process_sp(target_sp->CreateProcess(
m_interpreter.GetDebugger().GetListener(), llvm::StringRef(),
Index: include/lldb/Target/Thread.h
===================================================================
--- include/lldb/Target/Thread.h
+++ include/lldb/Target/Thread.h
@@ -45,7 +45,7 @@
//------------------------------------------------------------------
const RegularExpression *GetSymbolsToAvoidRegexp();
- FileSpecList &GetLibrariesToAvoid() const;
+ FileSpecList GetLibrariesToAvoid() const;
bool GetTraceEnabledState() const;
Index: include/lldb/Target/Target.h
===================================================================
--- include/lldb/Target/Target.h
+++ include/lldb/Target/Target.h
@@ -121,11 +121,13 @@
PathMappingList &GetSourcePathMap() const;
- FileSpecList &GetExecutableSearchPaths();
+ FileSpecList GetExecutableSearchPaths();
- FileSpecList &GetDebugFileSearchPaths();
+ void AppendExecutableSearchPaths(const FileSpec&);
- FileSpecList &GetClangModuleSearchPaths();
+ FileSpecList GetDebugFileSearchPaths();
+
+ FileSpecList GetClangModuleSearchPaths();
bool GetEnableAutoImportClangModules() const;
Index: include/lldb/Interpreter/OptionValueFileSpecList.h
===================================================================
--- include/lldb/Interpreter/OptionValueFileSpecList.h
+++ include/lldb/Interpreter/OptionValueFileSpecList.h
@@ -9,6 +9,8 @@
#ifndef liblldb_OptionValueFileSpecList_h_
#define liblldb_OptionValueFileSpecList_h_
+#include <mutex>
+
#include "lldb/Core/FileSpecList.h"
#include "lldb/Interpreter/OptionValue.h"
@@ -53,13 +55,23 @@
// Subclass specific functions
//---------------------------------------------------------------------
- FileSpecList &GetCurrentValue() { return m_current_value; }
+ FileSpecList GetCurrentValue() const {
+ std::lock_guard<std::mutex> lock(m_mutex);
+ return m_current_value;
+ }
- const FileSpecList &GetCurrentValue() const { return m_current_value; }
+ void SetCurrentValue(const FileSpecList &value) {
+ std::lock_guard<std::mutex> lock(m_mutex);
+ m_current_value = value;
+ }
- void SetCurrentValue(const FileSpecList &value) { m_current_value = value; }
+ void AppendCurrentValue(const FileSpec &value) {
+ std::lock_guard<std::mutex> lock(m_mutex);
+ m_current_value.Append(value);
+ }
protected:
+ mutable std::mutex m_mutex;
FileSpecList m_current_value;
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
