[lldb] Fix that symbols.clang-modules-cache-path is never initialized

[teemperor]

Summary

LLDB is supposed to ask the Clang Driver what the default module cache path is and
then use that value as the default for the symbols.clang-modules-cache-path setting.
However, we use the property type String to change symbols.clang-modules-cache-path
even though the type of that setting is FileSpec, so the setter will simply do nothing and
return false. We also don't check the return value of the setter, so this whole code ends up
not doing anything at all.

This changes the setter to use the correct property type and adds an assert that we actually
successfully set the default path. Also adds a test that checks that the default value for this
setting is never unset/empty path as this would effectively disable the import-std-module
feature from working by default.

[teemperor]

I had to add a lldb-noinit alias that creates a completely uninitialized lldb (as the default init file for the tests sets a cache path).

[JDevlieghere]

LGTM

lldb/source/Core/ModuleList.cpp

85–87

nit: no braces

[shafik]

LGTM

lldb/include/lldb/Core/ModuleList.h

class ModuleListProperties : public Properties {
   PathMappingList m_symlink_paths;
 
   void UpdateSymlinkMappings();
 
 public:
   ModuleListProperties();
 
   FileSpec GetClangModulesCachePath() const;
-  bool SetClangModulesCachePath(llvm::StringRef path);
+  bool SetClangModulesCachePath(const FileSpec &path);
   bool GetEnableExternalLookup() const;
   bool SetEnableExternalLookup(bool new_value);
 
   PathMappingList GetSymlinkMappings() const;
 };
 
 /// \class ModuleList ModuleList.h "lldb/Core/ModuleList.h"
 /// A collection class for Module objects.

lldb/source/Core/ModuleList.cpp

 ModuleListProperties::ModuleListProperties() {
   m_collection_sp =
       std::make_shared<OptionValueProperties>(ConstString("symbols"));
   m_collection_sp->Initialize(g_modulelist_properties);
   m_collection_sp->SetValueChangedCallback(ePropertySymLinkPaths,
                                            [this] { UpdateSymlinkMappings(); });
 
   llvm::SmallString<128> path;
-  clang::driver::Driver::getDefaultModuleCachePath(path);
-  SetClangModulesCachePath(path);
+  if (clang::driver::Driver::getDefaultModuleCachePath(path)) {
+    lldbassert(SetClangModulesCachePath(FileSpec(path)));
+  }
 }
 
 bool ModuleListProperties::GetEnableExternalLookup() const {
   const uint32_t idx = ePropertyEnableExternalLookup;
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_modulelist_properties[idx].default_uint_value != 0);
 }
 
 bool ModuleListProperties::SetEnableExternalLookup(bool new_value) {
   return m_collection_sp->SetPropertyAtIndexAsBoolean(
       nullptr, ePropertyEnableExternalLookup, new_value);
 }
 
 FileSpec ModuleListProperties::GetClangModulesCachePath() const {
   return m_collection_sp
       ->GetPropertyAtIndexAsOptionValueFileSpec(nullptr, false,
                                                 ePropertyClangModulesCachePath)
       ->GetCurrentValue();
 }
 
-bool ModuleListProperties::SetClangModulesCachePath(llvm::StringRef path) {
-  return m_collection_sp->SetPropertyAtIndexAsString(
+bool ModuleListProperties::SetClangModulesCachePath(const FileSpec &path) {
+  return m_collection_sp->SetPropertyAtIndexAsFileSpec(
       nullptr, ePropertyClangModulesCachePath, path);
 }
 
 void ModuleListProperties::UpdateSymlinkMappings() {
   FileSpecList list = m_collection_sp
                           ->GetPropertyAtIndexAsOptionValueFileSpecList(
                               nullptr, false, ePropertySymLinkPaths)
                           ->GetCurrentValue();

lldb/test/Shell/Settings/TestDefaultModuleCachePath.test

+# RUN: %lldb-noinit -x -s %s | FileCheck %s
+settings show symbols.clang-modules-cache-path
+q
+# This test checks that we get *any* clang modules cache path by default. The
+# actual path depends on the operating system.
+# CHECK: symbols.clang-modules-cache-path (file) = "
+# Clang treats an empty path in the same way as 'no path', so explicitly check
+# that we never have an empty path by default.
+# CHECK-NOT: symbols.clang-modules-cache-path (file) = ""

lldb/test/Shell/helper/toolchain.py

primary_tools = [
         ToolSubst('%lldb',
                   command=FindTool('lldb'),
                   extra_args=['--no-lldbinit', '-S', lldb_init],
                   unresolved='fatal'),
         ToolSubst('%lldb-init',
                   command=FindTool('lldb'),
                   extra_args=['-S', lldb_init],
                   unresolved='fatal'),
+        ToolSubst('%lldb-noinit',
+                  command=FindTool('lldb'),
+                  extra_args=['--no-lldbinit'],
+                  unresolved='fatal'),
         ToolSubst('%lldb-server',
                   command=FindTool("lldb-server"),
                   extra_args=[],
                   unresolved='ignore'),
         ToolSubst('%debugserver',
                   command=FindTool(dsname),
                   extra_args=dsargs,
                   unresolved='ignore'),