Lock accesses to OptionValueFileSpecList objects

[friss]

Summary

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.

[clayborg]

Almost seems like we can build the mutex into the base class OptionValue as we need general threaded protection for every setting. They any function that gets or sets the value should be able to protect itself using the base mutex

[friss]

In D60468#1460013, @clayborg wrote:

Almost seems like we can build the mutex into the base class OptionValue as we need general threaded protection for every setting. They any function that gets or sets the value should be able to protect itself using the base mutex

If we generalize this to most other properties, then this would make sense. Note that all properties that store very simple types don't need this, as they don't risk being accessed in a broken state.

[labath]

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

[jingham]

In D60468#1460065, @labath wrote:

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

Two reasons:

First, because you have to prime new debuggers with something, and that lives outside any Debugger. THen "settings set -g" is supposed to change the global settings for new debuggers, so there is both reading and writing to the global state.

Secondly, work done in the shared module cache or down in the symbol vendors and the like is generally done without a Debugger or Target, so if you need settable properties (e.g. search paths) then the global version of the property is the only one available.

[friss]

In D60468#1460065, @labath wrote:

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

As Jim said, there are parts of the debugger that simply do not have access to a debugger by design. I'm honestly not sure this is the correct design in hindsight. I tried to make the global state per-debugger, but I stopped when I needed to change the Plugin registration to take a debugger (because SymbolVendor plugins would need a debugger to access search paths for example). This approach also has other challenges and breaks command completion in non-trivial ways.

Note that once a target is created, there is no cross-debugger contamination anymore except if you force things by passing '-g' to 'settings set'. But it is super weird (or rather wrong) that simple 'settings set' on one debugger have an impact on other debuggers.

[jingham]

In D60468#1460212, @friss wrote:

In D60468#1460065, @labath wrote:

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

As Jim said, there are parts of the debugger that simply do not have access to a debugger by design. I'm honestly not sure this is the correct design in hindsight. I tried to make the global state per-debugger, but I stopped when I needed to change the Plugin registration to take a debugger (because SymbolVendor plugins would need a debugger to access search paths for example). This approach also has other challenges and breaks command completion in non-trivial ways.

Note that once a target is created, there is no cross-debugger contamination anymore except if you force things by passing '-g' to 'settings set'. But it is super weird (or rather wrong) that simple 'settings set' on one debugger have an impact on other debuggers.

Yeah, there are some settings (like target.default-arch, target.trap-handler-names, target.process.stop-on-exec and a few others) that are marked global in the PropertyDefinition, which means they always refer directly to the global (outside all debuggers) OptionValue. I can't remember why that was done.

[labath]

In D60468#1460212, @friss wrote:

In D60468#1460065, @labath wrote:

No opinion on the patch, but what is the reason for having settings that are shared between multiple Debugger instances? My expectation was that the debugger objects are completely independent, and I would be surprised if the value of some setting changed from under me because of something that happened in another debug session.

As Jim said, there are parts of the debugger that simply do not have access to a debugger by design. I'm honestly not sure this is the correct design in hindsight. I tried to make the global state per-debugger, but I stopped when I needed to change the Plugin registration to take a debugger (because SymbolVendor plugins would need a debugger to access search paths for example). This approach also has other challenges and breaks command completion in non-trivial ways.

Yeah, I think we're on the same page here. I'm going to hijack this patch a bit more (sorry), because this is a good opportunity to bring up something that I have been thinking about lately. I think sharing state between debugger instances is a good thing (particularly for expensive things like parsed debug info), but I think it should be done in a way that is invisible to the debugger instances. What this would mean in practice is that any piece of data that affects the contents of a shared object should be used in determining whether that object can be shared. For settings like exec-search-path and similar, this could mean that the logic to look up the actual executable and it's symbols happens within the context of a specific debugger instance (and it's own settings), but then, once all paths have been resolved, you check the shared cache to see if that specific path is present in there already. So, if two Debuggers have the same (or similar) search paths, they will still end up sharing the state, but if they have different search paths, things will behave just as if they each one had their own copy of all modules.

The reason I've been thinking about this is the "target symbols add" command, which adds/modifies the symbol file of a module. Despite the name, this command does not only modify the module symbols of the currently selected target, it can also modify the module of a target in a completely different debugger instance (!).

In D60468#1460022, @friss wrote:

In D60468#1460013, @clayborg wrote:

Almost seems like we can build the mutex into the base class OptionValue as we need general threaded protection for every setting. They any function that gets or sets the value should be able to protect itself using the base mutex

If we generalize this to most other properties, then this would make sense. Note that all properties that store very simple types don't need this, as they don't risk being accessed in a broken state.

While the simple types may not cause a crash, this access pattern would still qualify as a data race that would probably show up if you ran things under tsan. However, given how current OptionValue classes are implemented, I'm not sure how much can be actually done here in a generic way, and given that we seem to agree that this is not the best long term solution here, I don't see a reason to do everything in this patch.

[friss]

There were no objections to the patch, and it fixes a real crash seen in the field so I'm going to check it in.

There seems to be consensus that properties should be per-debugger, so I'll work on this when I get some spare cycles. Note that making that change won't prevent won't completely prevent concurrent accesses, so we might still want to investigate making all the properties thread-safe.

[clayborg]

Fine to fix the current issue. We should think about building this into the base class eventually. We can always have multi-threaded access, so we will always need to protect modifying operations.

include/lldb/Interpreter/OptionValueFileSpecList.h

 //===-- OptionValueFileSpecList.h -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_OptionValueFileSpecList_h_
 #define liblldb_OptionValueFileSpecList_h_
 
+#include <mutex>
+
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Interpreter/OptionValue.h"
 
 namespace lldb_private {
 
 class OptionValueFileSpecList : public OptionValue {
 public:
   OptionValueFileSpecList() : OptionValue(), m_current_value() {}
public:
   }
 
   lldb::OptionValueSP DeepCopy() const override;
 
   bool IsAggregateValue() const override { return true; }
 
   // 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;
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_OptionValueFileSpecList_h_

include/lldb/Target/Target.h

public:
 
   Environment GetEnvironment() const;
   void SetEnvironment(Environment env);
 
   bool GetSkipPrologue() const;
 
   PathMappingList &GetSourcePathMap() const;
 
-  FileSpecList &GetExecutableSearchPaths();
+  FileSpecList GetExecutableSearchPaths();
 
-  FileSpecList &GetDebugFileSearchPaths();
+  void AppendExecutableSearchPaths(const FileSpec&);
 
-  FileSpecList &GetClangModuleSearchPaths();
+  FileSpecList GetDebugFileSearchPaths();
+
+  FileSpecList GetClangModuleSearchPaths();
 
   bool GetEnableAutoImportClangModules() const;
 
   bool GetEnableImportStdModule() const;
 
   bool GetEnableAutoApplyFixIts() const;
 
   bool GetEnableNotifyAboutFixIts() const;

include/lldb/Target/Thread.h

public:
   /// thread won't stop in during "step-in" operations.
   ///
   /// \return
   ///    A pointer to a regular expression to compare against symbols,
   ///    or nullptr if all symbols are allowed.
   ///
   const RegularExpression *GetSymbolsToAvoidRegexp();
 
-  FileSpecList &GetLibrariesToAvoid() const;
+  FileSpecList GetLibrariesToAvoid() const;
 
   bool GetTraceEnabledState() const;
 
   bool GetStepInAvoidsNoDebug() const;
 
   bool GetStepOutAvoidsNoDebug() const;
 
   uint64_t GetMaxBacktraceDepth() const;

source/Commands/CommandObjectTarget.cpp

if (argc == 1 || core_file || remote_file) {
             if (!FileSystem::Instance().Readable(core_file)) {
               result.AppendMessageWithFormat(
                   "Core file '%s' is not readable.\n", core_path);
               result.SetStatus(eReturnStatusFailed);
               return false;
             }
             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(),
                 &core_file));
 
             if (process_sp) {
               // Seems weird that we Launch a core file, but that is what we
               // do!

source/Interpreter/OptionValueFileSpecLIst.cpp

Status OptionValueFileSpecList::SetValueFromString(llvm::StringRef value,
   case eVarSetOperationInvalid:
     error = OptionValue::SetValueFromString(value, op);
     break;
   }
   return error;
 }
 
 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));
 }

source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp

if (!m_module_sp && m_uuid.IsValid()) {
       PlatformSP platform_sp(target.GetPlatform());
       if (!m_module_sp && platform_sp) {
         ConstString platform_name(platform_sp->GetPluginName());
         static ConstString g_platform_name(
             PlatformDarwinKernel::GetPluginNameStatic());
         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);
         }
       }
 
       // Ask the Target to find this file on the local system, if possible.
       // This will search in the list of currently-loaded files, look in the
       // standard search paths on the system, and on a Mac it will try calling
       // the DebugSymbols framework with the UUID to find the binary via its
       // search methods.

source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

// Add additional search paths with { "-I", path } or { "-F", path } here.
     llvm::SmallString<128> path;
     auto props = ModuleList::GetGlobalModuleListProperties();
     props.GetClangModulesCachePath().GetPath(path);
     std::string module_cache_argument("-fmodules-cache-path=");
     module_cache_argument.append(path.str());
     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);
 
     std::string search_path_argument = "-I";
     search_path_argument.append(search_path.GetPath());
 
     compiler_invocation_arguments.push_back(search_path_argument);

source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp

public:
   virtual ~PlatformDarwinKernelProperties() {}
 
   bool GetSearchForKexts() const {
     const uint32_t idx = ePropertySearchForKexts;
     return m_collection_sp->GetPropertyAtIndexAsBoolean(
         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);
     return option_value->GetCurrentValue();
   }
 };
 
 typedef std::shared_ptr<PlatformDarwinKernelProperties>

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

public:
 
   static lldb_private::ConstString GetPluginNameStatic();
 
   static const char *GetPluginDescriptionStatic();
 
   static lldb_private::SymbolFile *
   CreateInstance(lldb_private::ObjectFile *obj_file);
 
-  static const lldb_private::FileSpecList &GetSymlinkPaths();
+  static lldb_private::FileSpecList GetSymlinkPaths();
 
   // Constructors and Destructors
 
   SymbolFileDWARF(lldb_private::ObjectFile *ofile);
 
   ~SymbolFileDWARF() override;
 
   uint32_t CalculateAbilities() override;

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

static ConstString GetSettingName() {
     return SymbolFileDWARF::GetPluginNameStatic();
   }
 
   PluginProperties() {
     m_collection_sp = std::make_shared<OptionValueProperties>(GetSettingName());
     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);
     return option_value->GetCurrentValue();
   }
 
   bool IgnoreFileIndexes() const {
     return m_collection_sp->GetPropertyAtIndexAsBoolean(
         nullptr, ePropertyIgnoreIndexes, false);
   }
 };
 
 typedef std::shared_ptr<PluginProperties> SymbolFileDWARFPropertiesSP;
 
 static const SymbolFileDWARFPropertiesSP &GetGlobalPluginProperties() {
   static const auto g_settings_sp(std::make_shared<PluginProperties>());
   return g_settings_sp;
 }
 
 } // anonymous namespace end
 
-const FileSpecList &SymbolFileDWARF::GetSymlinkPaths() {
+FileSpecList SymbolFileDWARF::GetSymlinkPaths() {
   return GetGlobalPluginProperties()->GetSymLinkPaths();
 }
 
 DWARFUnit *SymbolFileDWARF::GetBaseCompileUnit() {
   return nullptr;
 }
 
 void SymbolFileDWARF::Initialize() {

source/Target/Target.cpp

bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform) {
 
   if (executable_sp) {
     if (log)
       log->Printf("Target::SetArchitecture Trying to select executable file "
                   "architecture %s (%s)",
                   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) {
       SetExecutableModule(executable_sp, eLoadDependentsYes);
       return true;
     }
   }
   return false;
ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
 
   if (module_spec.GetUUID().IsValid())
     module_sp = m_images.FindFirstModule(module_spec);
 
   if (!module_sp) {
     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()) {
       ModuleSpec transformed_spec(module_spec);
       if (m_image_search_paths.RemapPath(
               module_spec.GetFileSpec().GetDirectory(),
               transformed_spec.GetFileSpec().GetDirectory())) {
         transformed_spec.GetFileSpec().GetFilename() =
             module_spec.GetFileSpec().GetFilename();
         error = ModuleList::GetSharedModule(transformed_spec, module_sp,
-                                            &GetExecutableSearchPaths(),
+                                            &search_paths,
                                             &old_module_sp, &did_create_module);
       }
     }
 
     if (!module_sp) {
       // If we have a UUID, we can check our global shared module list in case
       // we already have it. If we don't have a valid UUID, then we can't since
       // the path in "module_spec" will be a platform path, and we will need to
       // let the platform find that file. For example, we could be asking for
       // "/usr/lib/dyld" and if we do not have a UUID, we don't want to pick
       // the local copy of "/usr/lib/dyld" since our platform could be a remote
       // platform that has its own "/usr/lib/dyld" in an SDK or in a local file
       // cache.
       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);
       }
 
       if (!module_sp) {
         // The platform is responsible for finding and caching an appropriate
         // module in the shared module cache.
         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");
         }
       }
     }
 
     // We found a module that wasn't in our target list.  Let's make sure that
     // there wasn't an equivalent module in the list already, and if there was,
PathMappingList &TargetProperties::GetSourcePathMap() const {
   const uint32_t idx = ePropertySourceMap;
   OptionValuePathMappings *option_value =
       m_collection_sp->GetPropertyAtIndexAsOptionValuePathMappings(nullptr,
                                                                    false, idx);
   assert(option_value);
   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);
   return option_value->GetCurrentValue();
 }
 
 bool TargetProperties::GetEnableAutoImportClangModules() const {
   const uint32_t idx = ePropertyAutoImportClangModules;

source/Target/TargetList.cpp

if (!user_exe_path.empty()) {
       } else {
         // Use resolved path
         target_sp->SetArg0(file.GetPath().c_str());
       }
     }
     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.
     if (!is_dummy_target) {
       std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
       m_selected_target_idx = m_target_list.size();
       m_target_list.push_back(target_sp);
       // Now prime this from the dummy target:

source/Target/Thread.cpp

 
 ThreadProperties::~ThreadProperties() = default;
 
 const RegularExpression *ThreadProperties::GetSymbolsToAvoidRegexp() {
   const uint32_t idx = ePropertyStepAvoidRegex;
   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);
   return option_value->GetCurrentValue();
 }
 
 bool ThreadProperties::GetTraceEnabledState() const {
   const uint32_t idx = ePropertyEnableThreadTrace;