[lldb] Remove raw access to PluginInstances vector (#132884)

Remove raw access to PluginInstances vector

This commit modifies the PluginInstances class to remove direct access
to the m_instances vector. Instead, we expose a new `GetSnapshot` method
that returns a copy of the current state of the instances vector.  All
external iteration over the instances is updated to use the new method.

The motivation for the change is to allow modifying the way we store
instances without having to change all the clients. This is a
preliminary change to allow enabling/disabling of plugins in which case
we want to iterate over only enabled plugins.

We also considered using a custom iterator that wraps the vector
iterator and can skip over disabled instances. That works, but the
iterator code is a bit messy with all template and typedefs to make a
compliant iterator.

Author: dmpots

lldb/source/Core/PluginManager.cpp

@@ -226,30 +226,26 @@ template <typename Instance> class PluginInstances {
   }
 
   typename Instance::CallbackType GetCallbackAtIndex(uint32_t idx) {
-    if (Instance *instance = GetInstanceAtIndex(idx))
+    if (const Instance *instance = GetInstanceAtIndex(idx))
       return instance->create_callback;
     return nullptr;
   }
 
   llvm::StringRef GetDescriptionAtIndex(uint32_t idx) {
-    if (Instance *instance = GetInstanceAtIndex(idx))
+    if (const Instance *instance = GetInstanceAtIndex(idx))
       return instance->description;
     return "";
   }
 
   llvm::StringRef GetNameAtIndex(uint32_t idx) {
-    if (Instance *instance = GetInstanceAtIndex(idx))
+    if (const Instance *instance = GetInstanceAtIndex(idx))
       return instance->name;
     return "";
   }
 
   typename Instance::CallbackType GetCallbackForName(llvm::StringRef name) {
-    if (name.empty())
-      return nullptr;
-    for (auto &instance : m_instances) {
-      if (name == instance.name)
-        return instance.create_callback;
-    }
+    if (const Instance *instance = GetInstanceForName(name))
+      return instance->create_callback;
     return nullptr;
   }
 
@@ -260,12 +256,33 @@ template <typename Instance> class PluginInstances {
     }
   }
 
-  const std::vector<Instance> &GetInstances() const { return m_instances; }
-  std::vector<Instance> &GetInstances() { return m_instances; }
+  // Return a copy of all the enabled instances.
+  // Note that this is a copy of the internal state so modifications
+  // to the returned instances will not be reflected back to instances
+  // stored by the PluginInstances object.
+  std::vector<Instance> GetSnapshot() { return m_instances; }
+
+  const Instance *GetInstanceAtIndex(uint32_t idx) {
+    uint32_t count = 0;
+
+    return FindEnabledInstance(
+        [&](const Instance &instance) { return count++ == idx; });
+  }
+
+  const Instance *GetInstanceForName(llvm::StringRef name) {
+    if (name.empty())
+      return nullptr;
 
-  Instance *GetInstanceAtIndex(uint32_t idx) {
-    if (idx < m_instances.size())
-      return &m_instances[idx];
+    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) {
+      if (predicate(instance))
+        return &instance;
+    }
     return nullptr;
   }
 
@@ -571,17 +588,15 @@ PluginManager::GetLanguageRuntimeCreateCallbackAtIndex(uint32_t idx) {
 
 LanguageRuntimeGetCommandObject
 PluginManager::GetLanguageRuntimeGetCommandObjectAtIndex(uint32_t idx) {
-  const auto &instances = GetLanguageRuntimeInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].command_callback;
+  if (auto instance = GetLanguageRuntimeInstances().GetInstanceAtIndex(idx))
+    return instance->command_callback;
   return nullptr;
 }
 
 LanguageRuntimeGetExceptionPrecondition
 PluginManager::GetLanguageRuntimeGetExceptionPreconditionAtIndex(uint32_t idx) {
-  const auto &instances = GetLanguageRuntimeInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].precondition_callback;
+  if (auto instance = GetLanguageRuntimeInstances().GetInstanceAtIndex(idx))
+    return instance->precondition_callback;
   return nullptr;
 }
 
@@ -643,12 +658,7 @@ bool PluginManager::IsRegisteredObjectFilePluginName(llvm::StringRef name) {
   if (name.empty())
     return false;
 
-  const auto &instances = GetObjectFileInstances().GetInstances();
-  for (auto &instance : instances) {
-    if (instance.name == name)
-      return true;
-  }
-  return false;
+  return GetObjectFileInstances().GetInstanceForName(name) != nullptr;
 }
 
 bool PluginManager::RegisterPlugin(
@@ -674,29 +684,24 @@ PluginManager::GetObjectFileCreateCallbackAtIndex(uint32_t idx) {
 
 ObjectFileCreateMemoryInstance
 PluginManager::GetObjectFileCreateMemoryCallbackAtIndex(uint32_t idx) {
-  const auto &instances = GetObjectFileInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].create_memory_callback;
+  if (auto instance = GetObjectFileInstances().GetInstanceAtIndex(idx))
+    return instance->create_memory_callback;
   return nullptr;
 }
 
 ObjectFileGetModuleSpecifications
 PluginManager::GetObjectFileGetModuleSpecificationsCallbackAtIndex(
     uint32_t idx) {
-  const auto &instances = GetObjectFileInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].get_module_specifications;
+  if (auto instance = GetObjectFileInstances().GetInstanceAtIndex(idx))
+    return instance->get_module_specifications;
   return nullptr;
 }
 
 ObjectFileCreateMemoryInstance
 PluginManager::GetObjectFileCreateMemoryCallbackForPluginName(
     llvm::StringRef name) {
-  const auto &instances = GetObjectFileInstances().GetInstances();
-  for (auto &instance : instances) {
-    if (instance.name == name)
-      return instance.create_memory_callback;
-  }
+  if (auto instance = GetObjectFileInstances().GetInstanceForName(name))
+    return instance->create_memory_callback;
   return nullptr;
 }
 
@@ -729,7 +734,7 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
 
   // Fall back to object plugins.
   const auto &plugin_name = options.GetPluginName().value_or("");
-  auto &instances = GetObjectFileInstances().GetInstances();
+  auto instances = GetObjectFileInstances().GetSnapshot();
   for (auto &instance : instances) {
     if (plugin_name.empty() || instance.name == plugin_name) {
       if (instance.save_core && instance.save_core(process_sp, options, error))
@@ -791,18 +796,16 @@ PluginManager::GetObjectContainerCreateCallbackAtIndex(uint32_t idx) {
 
 ObjectContainerCreateMemoryInstance
 PluginManager::GetObjectContainerCreateMemoryCallbackAtIndex(uint32_t idx) {
-  const auto &instances = GetObjectContainerInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].create_memory_callback;
+  if (auto instance = GetObjectContainerInstances().GetInstanceAtIndex(idx))
+    return instance->create_memory_callback;
   return nullptr;
 }
 
 ObjectFileGetModuleSpecifications
 PluginManager::GetObjectContainerGetModuleSpecificationsCallbackAtIndex(
     uint32_t idx) {
-  const auto &instances = GetObjectContainerInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].get_module_specifications;
+  if (auto instance = GetObjectContainerInstances().GetInstanceAtIndex(idx))
+    return instance->get_module_specifications;
   return nullptr;
 }
 
@@ -849,7 +852,7 @@ PluginManager::GetPlatformCreateCallbackForPluginName(llvm::StringRef name) {
 
 void PluginManager::AutoCompletePlatformName(llvm::StringRef name,
                                              CompletionRequest &request) {
-  for (const auto &instance : GetPlatformInstances().GetInstances()) {
+  for (const auto &instance : GetPlatformInstances().GetSnapshot()) {
     if (instance.name.starts_with(name))
       request.AddCompletion(instance.name);
   }
@@ -897,7 +900,7 @@ PluginManager::GetProcessCreateCallbackForPluginName(llvm::StringRef name) {
 
 void PluginManager::AutoCompleteProcessName(llvm::StringRef name,
                                             CompletionRequest &request) {
-  for (const auto &instance : GetProcessInstances().GetInstances()) {
+  for (const auto &instance : GetProcessInstances().GetSnapshot()) {
     if (instance.name.starts_with(name))
       request.AddCompletion(instance.name, instance.description);
   }
@@ -935,11 +938,11 @@ bool PluginManager::UnregisterPlugin(
 
 lldb::RegisterTypeBuilderSP
 PluginManager::GetRegisterTypeBuilder(Target &target) {
-  const auto &instances = GetRegisterTypeBuilderInstances().GetInstances();
   // We assume that RegisterTypeBuilderClang is the only instance of this plugin
   // type and is always present.
-  assert(instances.size());
-  return instances[0].create_callback(target);
+  auto instance = GetRegisterTypeBuilderInstances().GetInstanceAtIndex(0);
+  assert(instance);
+  return instance->create_callback(target);
 }
 
 #pragma mark ScriptInterpreter
@@ -984,7 +987,7 @@ PluginManager::GetScriptInterpreterCreateCallbackAtIndex(uint32_t idx) {
 lldb::ScriptInterpreterSP
 PluginManager::GetScriptInterpreterForLanguage(lldb::ScriptLanguage script_lang,
                                                Debugger &debugger) {
-  const auto &instances = GetScriptInterpreterInstances().GetInstances();
+  const auto instances = GetScriptInterpreterInstances().GetSnapshot();
   ScriptInterpreterCreateInstance none_instance = nullptr;
   for (const auto &instance : instances) {
     if (instance.language == lldb::eScriptLanguageNone)
@@ -1046,13 +1049,12 @@ PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(uint32_t idx) {
 StructuredDataFilterLaunchInfo
 PluginManager::GetStructuredDataFilterCallbackAtIndex(
     uint32_t idx, bool &iteration_complete) {
-  const auto &instances = GetStructuredDataPluginInstances().GetInstances();
-  if (idx < instances.size()) {
+  if (auto instance =
+          GetStructuredDataPluginInstances().GetInstanceAtIndex(idx)) {
     iteration_complete = false;
-    return instances[idx].filter_callback;
-  } else {
-    iteration_complete = true;
+    return instance->filter_callback;
   }
+  iteration_complete = true;
   return nullptr;
 }
 
@@ -1167,7 +1169,7 @@ PluginManager::GetSymbolLocatorCreateCallbackAtIndex(uint32_t idx) {
 
 ModuleSpec
 PluginManager::LocateExecutableObjectFile(const ModuleSpec &module_spec) {
-  auto &instances = GetSymbolLocatorInstances().GetInstances();
+  auto instances = GetSymbolLocatorInstances().GetSnapshot();
   for (auto &instance : instances) {
     if (instance.locate_executable_object_file) {
       std::optional<ModuleSpec> result =
@@ -1181,7 +1183,7 @@ PluginManager::LocateExecutableObjectFile(const ModuleSpec &module_spec) {
 
 FileSpec PluginManager::LocateExecutableSymbolFile(
     const ModuleSpec &module_spec, const FileSpecList &default_search_paths) {
-  auto &instances = GetSymbolLocatorInstances().GetInstances();
+  auto instances = GetSymbolLocatorInstances().GetSnapshot();
   for (auto &instance : instances) {
     if (instance.locate_executable_symbol_file) {
       std::optional<FileSpec> result = instance.locate_executable_symbol_file(
@@ -1197,7 +1199,7 @@ bool PluginManager::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
                                                 Status &error,
                                                 bool force_lookup,
                                                 bool copy_executable) {
-  auto &instances = GetSymbolLocatorInstances().GetInstances();
+  auto instances = GetSymbolLocatorInstances().GetSnapshot();
   for (auto &instance : instances) {
     if (instance.download_object_symbol_file) {
       if (instance.download_object_symbol_file(module_spec, error, force_lookup,
@@ -1211,7 +1213,7 @@ bool PluginManager::DownloadObjectAndSymbolFile(ModuleSpec &module_spec,
 FileSpec PluginManager::FindSymbolFileInBundle(const FileSpec &symfile_bundle,
                                                const UUID *uuid,
                                                const ArchSpec *arch) {
-  auto &instances = GetSymbolLocatorInstances().GetInstances();
+  auto instances = GetSymbolLocatorInstances().GetSnapshot();
   for (auto &instance : instances) {
     if (instance.find_symbol_file_in_bundle) {
       std::optional<FileSpec> result =
@@ -1272,21 +1274,20 @@ PluginManager::GetTraceCreateCallback(llvm::StringRef plugin_name) {
 
 TraceCreateInstanceForLiveProcess
 PluginManager::GetTraceCreateCallbackForLiveProcess(llvm::StringRef plugin_name) {
-  for (const TraceInstance &instance : GetTracePluginInstances().GetInstances())
-    if (instance.name == plugin_name)
-      return instance.create_callback_for_live_process;
+  if (auto instance = GetTracePluginInstances().GetInstanceForName(plugin_name))
+    return instance->create_callback_for_live_process;
+
   return nullptr;
 }
 
 llvm::StringRef PluginManager::GetTraceSchema(llvm::StringRef plugin_name) {
-  for (const TraceInstance &instance : GetTracePluginInstances().GetInstances())
-    if (instance.name == plugin_name)
-      return instance.schema;
+  if (auto instance = GetTracePluginInstances().GetInstanceForName(plugin_name))
+    return instance->schema;
   return llvm::StringRef();
 }
 
 llvm::StringRef PluginManager::GetTraceSchema(size_t index) {
-  if (TraceInstance *instance =
+  if (const TraceInstance *instance =
           GetTracePluginInstances().GetInstanceAtIndex(index))
     return instance->schema;
   return llvm::StringRef();
@@ -1335,7 +1336,7 @@ bool PluginManager::UnregisterPlugin(
 
 ThreadTraceExportCommandCreator
 PluginManager::GetThreadTraceExportCommandCreatorAtIndex(uint32_t index) {
-  if (TraceExporterInstance *instance =
+  if (const TraceExporterInstance *instance =
           GetTraceExporterInstances().GetInstanceAtIndex(index))
     return instance->create_thread_trace_export_command;
   return nullptr;
@@ -1438,9 +1439,9 @@ bool PluginManager::UnregisterPlugin(
 
 InstrumentationRuntimeGetType
 PluginManager::GetInstrumentationRuntimeGetTypeCallbackAtIndex(uint32_t idx) {
-  const auto &instances = GetInstrumentationRuntimeInstances().GetInstances();
-  if (idx < instances.size())
-    return instances[idx].get_type_callback;
+  if (auto instance =
+          GetInstrumentationRuntimeInstances().GetInstanceAtIndex(idx))
+    return instance->get_type_callback;
   return nullptr;
 }
 
@@ -1493,15 +1494,15 @@ PluginManager::GetTypeSystemCreateCallbackAtIndex(uint32_t idx) {
 }
 
 LanguageSet PluginManager::GetAllTypeSystemSupportedLanguagesForTypes() {
-  const auto &instances = GetTypeSystemInstances().GetInstances();
+  const auto instances = GetTypeSystemInstances().GetSnapshot();
   LanguageSet all;
   for (unsigned i = 0; i < instances.size(); ++i)
     all.bitvector |= instances[i].supported_languages_for_types.bitvector;
   return all;
 }
 
 LanguageSet PluginManager::GetAllTypeSystemSupportedLanguagesForExpressions() {
-  const auto &instances = GetTypeSystemInstances().GetInstances();
+  const auto instances = GetTypeSystemInstances().GetSnapshot();
   LanguageSet all;
   for (unsigned i = 0; i < instances.size(); ++i)
     all.bitvector |= instances[i].supported_languages_for_expressions.bitvector;
@@ -1545,7 +1546,7 @@ bool PluginManager::UnregisterPlugin(
 }
 
 uint32_t PluginManager::GetNumScriptedInterfaces() {
-  return GetScriptedInterfaceInstances().GetInstances().size();
+  return GetScriptedInterfaceInstances().GetSnapshot().size();
 }
 
 llvm::StringRef PluginManager::GetScriptedInterfaceNameAtIndex(uint32_t index) {
@@ -1559,17 +1560,16 @@ PluginManager::GetScriptedInterfaceDescriptionAtIndex(uint32_t index) {
 
 lldb::ScriptLanguage
 PluginManager::GetScriptedInterfaceLanguageAtIndex(uint32_t idx) {
-  const auto &instances = GetScriptedInterfaceInstances().GetInstances();
-  return idx < instances.size() ? instances[idx].language
-                                : ScriptLanguage::eScriptLanguageNone;
+  if (auto instance = GetScriptedInterfaceInstances().GetInstanceAtIndex(idx))
+    return instance->language;
+  return ScriptLanguage::eScriptLanguageNone;
 }
 
 ScriptedInterfaceUsages
 PluginManager::GetScriptedInterfaceUsagesAtIndex(uint32_t idx) {
-  const auto &instances = GetScriptedInterfaceInstances().GetInstances();
-  if (idx >= instances.size())
-    return {};
-  return instances[idx].usages;
+  if (auto instance = GetScriptedInterfaceInstances().GetInstanceAtIndex(idx))
+    return instance->usages;
+  return {};
 }
 
 #pragma mark REPL
@@ -1606,13 +1606,13 @@ REPLCreateInstance PluginManager::GetREPLCreateCallbackAtIndex(uint32_t idx) {
 }
 
 LanguageSet PluginManager::GetREPLSupportedLanguagesAtIndex(uint32_t idx) {
-  const auto &instances = GetREPLInstances().GetInstances();
-  return idx < instances.size() ? instances[idx].supported_languages
-                                : LanguageSet();
+  if (auto instance = GetREPLInstances().GetInstanceAtIndex(idx))
+    return instance->supported_languages;
+  return LanguageSet();
 }
 
 LanguageSet PluginManager::GetREPLAllTypeSystemSupportedLanguages() {
-  const auto &instances = GetREPLInstances().GetInstances();
+  const auto instances = GetREPLInstances().GetSnapshot();
   LanguageSet all;
   for (unsigned i = 0; i < instances.size(); ++i)
     all.bitvector |= instances[i].supported_languages.bitvector;