llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> In #<!-- -->184273, John pointed out that the PluginManager is currently not thread safe. While we don't currently provide any guarantees, it seems desirable to be able to interact safely with the PluginManager from different threads. For example, we allow dynamically loading plugins from the command interpreter, which in theory could be coming from different threads. --- Full diff: https://github.com/llvm/llvm-project/pull/184452.diff 1 Files Affected: - (modified) lldb/source/Core/PluginManager.cpp (+143-85) ``````````diff diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index e430305999b39..6b84227a6672a 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -491,6 +491,7 @@ template <typename Instance> class PluginInstances { if (!callback) return false; assert(!name.empty()); + std::lock_guard<std::mutex> lock(m_mutex); m_instances.emplace_back(name, description, callback, std::forward<Args>(args)...); return true; @@ -499,6 +500,7 @@ template <typename Instance> class PluginInstances { bool UnregisterPlugin(typename Instance::CallbackType callback) { if (!callback) return false; + std::lock_guard<std::mutex> lock(m_mutex); auto pos = m_instances.begin(); auto end = m_instances.end(); for (; pos != end; ++pos) { @@ -511,31 +513,61 @@ template <typename Instance> class PluginInstances { } typename Instance::CallbackType GetCallbackAtIndex(uint32_t idx) { - if (const Instance *instance = GetInstanceAtIndex(idx)) - return instance->create_callback; + std::lock_guard<std::mutex> lock(m_mutex); + uint32_t count = 0; + for (const auto &instance : m_instances) { + if (!instance.enabled) + continue; + if (count++ == idx) + return instance.create_callback; + } return nullptr; } llvm::StringRef GetDescriptionAtIndex(uint32_t idx) { - if (const Instance *instance = GetInstanceAtIndex(idx)) - return instance->description; + std::lock_guard<std::mutex> lock(m_mutex); + uint32_t count = 0; + for (const auto &instance : m_instances) { + if (!instance.enabled) + continue; + if (count++ == idx) + return instance.description; + } return ""; } llvm::StringRef GetNameAtIndex(uint32_t idx) { - if (const Instance *instance = GetInstanceAtIndex(idx)) - return instance->name; + std::lock_guard<std::mutex> lock(m_mutex); + uint32_t count = 0; + for (const auto &instance : m_instances) { + if (!instance.enabled) + continue; + if (count++ == idx) + return instance.name; + } return ""; } typename Instance::CallbackType GetCallbackForName(llvm::StringRef name) { - if (const Instance *instance = GetInstanceForName(name)) - return instance->create_callback; + if (name.empty()) + return nullptr; + std::lock_guard<std::mutex> lock(m_mutex); + for (const auto &instance : m_instances) { + if (!instance.enabled) + continue; + if (instance.name == name) + return instance.create_callback; + } return nullptr; } void PerformDebuggerCallback(Debugger &debugger) { - for (const auto &instance : m_instances) { + std::vector<Instance> snapshot; + { + std::lock_guard<std::mutex> lock(m_mutex); + snapshot = m_instances; + } + for (const auto &instance : snapshot) { if (!instance.enabled) continue; if (instance.debugger_init_callback) @@ -548,38 +580,43 @@ template <typename Instance> class PluginInstances { // to the returned instances will not be reflected back to instances // stored by the PluginInstances object. std::vector<Instance> GetSnapshot() { + std::lock_guard<std::mutex> lock(m_mutex); std::vector<Instance> enabled_instances; - for (const auto &instance : m_instances) { + for (const Instance &instance : m_instances) { if (instance.enabled) enabled_instances.push_back(instance); } return enabled_instances; } - const Instance *GetInstanceAtIndex(uint32_t idx) { + // Return a field from the idx-th enabled instance, or default_val if none. + template <typename Ret, typename Callback> + Ret GetFieldAtIndex(uint32_t idx, Ret default_val, Callback callback) { + std::lock_guard<std::mutex> lock(m_mutex); uint32_t count = 0; - - return FindEnabledInstance( - [&](const Instance &instance) { return count++ == idx; }); + for (const Instance &instance : m_instances) { + if (!instance.enabled) + continue; + if (count++ == idx) + return callback(instance); + } + return default_val; } - const Instance *GetInstanceForName(llvm::StringRef name) { + // Return a field from the enabled instance with the given name, or + // default_val if none. + template <typename Ret, typename Callback> + Ret GetFieldForName(llvm::StringRef name, Ret default_val, Callback callback) { if (name.empty()) - return nullptr; - - return FindEnabledInstance( - [&](const Instance &instance) { return instance.name == name; }); - } - - const Instance * - FindEnabledInstance(std::function<bool(const Instance &)> predicate) const { - for (const auto &instance : m_instances) { + return default_val; + std::lock_guard<std::mutex> lock(m_mutex); + for (const Instance &instance : m_instances) { if (!instance.enabled) continue; - if (predicate(instance)) - return &instance; + if (instance.name == name) + return callback(instance); } - return nullptr; + return default_val; } // Return a list of all the registered plugin instances. This includes both @@ -587,6 +624,7 @@ template <typename Instance> class PluginInstances { // were registered which is the order they would be queried if they were all // enabled. std::vector<RegisteredPluginInfo> GetPluginInfoForAllInstances() { + std::lock_guard<std::mutex> lock(m_mutex); // Lookup the plugin info for each instance in the sorted order. std::vector<RegisteredPluginInfo> plugin_infos; plugin_infos.reserve(m_instances.size()); @@ -598,6 +636,7 @@ template <typename Instance> class PluginInstances { } bool SetInstanceEnabled(llvm::StringRef name, bool enable) { + std::lock_guard<std::mutex> lock(m_mutex); auto it = llvm::find_if(m_instances, [&](const Instance &instance) { return instance.name == name; }); @@ -610,7 +649,32 @@ template <typename Instance> class PluginInstances { } private: + const Instance *GetInstanceAtIndexLocked(uint32_t idx) { + uint32_t count = 0; + return FindEnabledInstanceLocked( + [&](const Instance &instance) { return count++ == idx; }); + } + + const Instance *GetInstanceForNameLocked(llvm::StringRef name) { + if (name.empty()) + return nullptr; + return FindEnabledInstanceLocked( + [&](const Instance &instance) { return instance.name == name; }); + } + + const Instance *FindEnabledInstanceLocked( + std::function<bool(const Instance &)> predicate) const { + for (const auto &instance : m_instances) { + if (!instance.enabled) + continue; + if (predicate(instance)) + return &instance; + } + return nullptr; + } + std::vector<Instance> m_instances; + mutable std::mutex m_mutex; }; #pragma mark ABI @@ -905,16 +969,16 @@ PluginManager::GetLanguageRuntimeCreateCallbackAtIndex(uint32_t idx) { LanguageRuntimeGetCommandObject PluginManager::GetLanguageRuntimeGetCommandObjectAtIndex(uint32_t idx) { - if (auto instance = GetLanguageRuntimeInstances().GetInstanceAtIndex(idx)) - return instance->command_callback; - return nullptr; + return GetLanguageRuntimeInstances().GetFieldAtIndex( + idx, (LanguageRuntimeGetCommandObject) nullptr, + [](const auto &i) { return i.command_callback; }); } LanguageRuntimeGetExceptionPrecondition PluginManager::GetLanguageRuntimeGetExceptionPreconditionAtIndex(uint32_t idx) { - if (auto instance = GetLanguageRuntimeInstances().GetInstanceAtIndex(idx)) - return instance->precondition_callback; - return nullptr; + return GetLanguageRuntimeInstances().GetFieldAtIndex( + idx, (LanguageRuntimeGetExceptionPrecondition) nullptr, + [](const auto &i) { return i.precondition_callback; }); } #pragma mark SystemRuntime @@ -975,7 +1039,8 @@ bool PluginManager::IsRegisteredObjectFilePluginName(llvm::StringRef name) { if (name.empty()) return false; - return GetObjectFileInstances().GetInstanceForName(name) != nullptr; + return GetObjectFileInstances().GetFieldForName( + name, false, [](const auto &) { return true; }); } bool PluginManager::RegisterPlugin( @@ -1001,25 +1066,25 @@ PluginManager::GetObjectFileCreateCallbackAtIndex(uint32_t idx) { ObjectFileCreateMemoryInstance PluginManager::GetObjectFileCreateMemoryCallbackAtIndex(uint32_t idx) { - if (auto instance = GetObjectFileInstances().GetInstanceAtIndex(idx)) - return instance->create_memory_callback; - return nullptr; + return GetObjectFileInstances().GetFieldAtIndex( + idx, (ObjectFileCreateMemoryInstance) nullptr, + [](const auto &i) { return i.create_memory_callback; }); } ObjectFileGetModuleSpecifications PluginManager::GetObjectFileGetModuleSpecificationsCallbackAtIndex( uint32_t idx) { - if (auto instance = GetObjectFileInstances().GetInstanceAtIndex(idx)) - return instance->get_module_specifications; - return nullptr; + return GetObjectFileInstances().GetFieldAtIndex( + idx, (ObjectFileGetModuleSpecifications) nullptr, + [](const auto &i) { return i.get_module_specifications; }); } ObjectFileCreateMemoryInstance PluginManager::GetObjectFileCreateMemoryCallbackForPluginName( llvm::StringRef name) { - if (auto instance = GetObjectFileInstances().GetInstanceForName(name)) - return instance->create_memory_callback; - return nullptr; + return GetObjectFileInstances().GetFieldForName( + name, (ObjectFileCreateMemoryInstance) nullptr, + [](const auto &i) { return i.create_memory_callback; }); } Status PluginManager::SaveCore(lldb_private::SaveCoreOptions &options) { @@ -1132,17 +1197,17 @@ PluginManager::GetObjectContainerCreateCallbackAtIndex(uint32_t idx) { ObjectContainerCreateMemoryInstance PluginManager::GetObjectContainerCreateMemoryCallbackAtIndex(uint32_t idx) { - if (auto instance = GetObjectContainerInstances().GetInstanceAtIndex(idx)) - return instance->create_memory_callback; - return nullptr; + return GetObjectContainerInstances().GetFieldAtIndex( + idx, (ObjectContainerCreateMemoryInstance) nullptr, + [](const auto &i) { return i.create_memory_callback; }); } ObjectFileGetModuleSpecifications PluginManager::GetObjectContainerGetModuleSpecificationsCallbackAtIndex( uint32_t idx) { - if (auto instance = GetObjectContainerInstances().GetInstanceAtIndex(idx)) - return instance->get_module_specifications; - return nullptr; + return GetObjectContainerInstances().GetFieldAtIndex( + idx, (ObjectFileGetModuleSpecifications) nullptr, + [](const auto &i) { return i.get_module_specifications; }); } #pragma mark Platform @@ -1309,9 +1374,9 @@ lldb::RegisterTypeBuilderSP PluginManager::GetRegisterTypeBuilder(Target &target) { // We assume that RegisterTypeBuilderClang is the only instance of this plugin // type and is always present. - auto instance = GetRegisterTypeBuilderInstances().GetInstanceAtIndex(0); - assert(instance); - return instance->create_callback(target); + auto cb = GetRegisterTypeBuilderInstances().GetCallbackAtIndex(0); + assert(cb); + return cb(target); } #pragma mark ScriptInterpreter @@ -1486,13 +1551,13 @@ PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(uint32_t idx) { StructuredDataFilterLaunchInfo PluginManager::GetStructuredDataFilterCallbackAtIndex( uint32_t idx, bool &iteration_complete) { - if (auto instance = - GetStructuredDataPluginInstances().GetInstanceAtIndex(idx)) { - iteration_complete = false; - return instance->filter_callback; - } iteration_complete = true; - return nullptr; + return GetStructuredDataPluginInstances() + .GetFieldAtIndex<StructuredDataFilterLaunchInfo>( + idx, nullptr, [&iteration_complete](const auto &i) { + iteration_complete = false; + return i.filter_callback; + }); } #pragma mark SymbolFile @@ -1724,22 +1789,19 @@ PluginManager::GetTraceCreateCallback(llvm::StringRef plugin_name) { TraceCreateInstanceForLiveProcess PluginManager::GetTraceCreateCallbackForLiveProcess( llvm::StringRef plugin_name) { - if (auto instance = GetTracePluginInstances().GetInstanceForName(plugin_name)) - return instance->create_callback_for_live_process; - - return nullptr; + return GetTracePluginInstances().GetFieldForName( + plugin_name, (TraceCreateInstanceForLiveProcess) nullptr, + [](const auto &i) { return i.create_callback_for_live_process; }); } llvm::StringRef PluginManager::GetTraceSchema(llvm::StringRef plugin_name) { - if (auto instance = GetTracePluginInstances().GetInstanceForName(plugin_name)) - return instance->schema; - return llvm::StringRef(); + return GetTracePluginInstances().GetFieldForName( + plugin_name, llvm::StringRef(), [](const auto &i) { return i.schema; }); } llvm::StringRef PluginManager::GetTraceSchema(size_t index) { - if (const TraceInstance *instance = - GetTracePluginInstances().GetInstanceAtIndex(index)) - return instance->schema; + return GetTracePluginInstances().GetFieldAtIndex( + index, llvm::StringRef(), [](const auto &i) { return i.schema; }); return llvm::StringRef(); } @@ -1786,10 +1848,9 @@ bool PluginManager::UnregisterPlugin( ThreadTraceExportCommandCreator PluginManager::GetThreadTraceExportCommandCreatorAtIndex(uint32_t index) { - if (const TraceExporterInstance *instance = - GetTraceExporterInstances().GetInstanceAtIndex(index)) - return instance->create_thread_trace_export_command; - return nullptr; + return GetTraceExporterInstances().GetFieldAtIndex( + index, (ThreadTraceExportCommandCreator) nullptr, + [](const auto &i) { return i.create_thread_trace_export_command; }); } llvm::StringRef @@ -1889,10 +1950,9 @@ bool PluginManager::UnregisterPlugin( InstrumentationRuntimeGetType PluginManager::GetInstrumentationRuntimeGetTypeCallbackAtIndex(uint32_t idx) { - if (auto instance = - GetInstrumentationRuntimeInstances().GetInstanceAtIndex(idx)) - return instance->get_type_callback; - return nullptr; + return GetInstrumentationRuntimeInstances().GetFieldAtIndex( + idx, (InstrumentationRuntimeGetType) nullptr, + [](const auto &i) { return i.get_type_callback; }); } InstrumentationRuntimeCreateInstance @@ -2010,16 +2070,15 @@ PluginManager::GetScriptedInterfaceDescriptionAtIndex(uint32_t index) { lldb::ScriptLanguage PluginManager::GetScriptedInterfaceLanguageAtIndex(uint32_t idx) { - if (auto instance = GetScriptedInterfaceInstances().GetInstanceAtIndex(idx)) - return instance->language; - return ScriptLanguage::eScriptLanguageNone; + return GetScriptedInterfaceInstances().GetFieldAtIndex( + idx, ScriptLanguage::eScriptLanguageNone, + [](const auto &i) { return i.language; }); } ScriptedInterfaceUsages PluginManager::GetScriptedInterfaceUsagesAtIndex(uint32_t idx) { - if (auto instance = GetScriptedInterfaceInstances().GetInstanceAtIndex(idx)) - return instance->usages; - return {}; + return GetScriptedInterfaceInstances().GetFieldAtIndex( + idx, ScriptedInterfaceUsages{}, [](const auto &i) { return i.usages; }); } #pragma mark REPL @@ -2057,9 +2116,8 @@ REPLCreateInstance PluginManager::GetREPLCreateCallbackAtIndex(uint32_t idx) { } LanguageSet PluginManager::GetREPLSupportedLanguagesAtIndex(uint32_t idx) { - if (auto instance = GetREPLInstances().GetInstanceAtIndex(idx)) - return instance->supported_languages; - return LanguageSet(); + return GetREPLInstances().GetFieldAtIndex( + idx, LanguageSet{}, [](const auto &i) { return i.supported_languages; }); } LanguageSet PluginManager::GetREPLAllTypeSystemSupportedLanguages() { `````````` </details> https://github.com/llvm/llvm-project/pull/184452 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
