[llvm-branch-commits] [lldb] 208e3f5 - [lldb] Fix that symbols.clang-modules-cache-path is never initialized

[Raphael Isemann via llvm-branch-commits]

Author: Raphael Isemann
Date: 2020-12-10T13:37:40+01:00
New Revision: 208e3f5d9b6c172f65dbb9cdbc9354c81c6d8911

URL: https://github.com/llvm/llvm-project/commit/208e3f5d9b6c172f65dbb9cdbc9354c81c6d8911
DIFF: https://github.com/llvm/llvm-project/commit/208e3f5d9b6c172f65dbb9cdbc9354c81c6d8911.diff

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

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.

Reviewed By: JDevlieghere, shafik

Differential Revision: https://reviews.llvm.org/D92772

Added: 
    lldb/test/Shell/Settings/TestDefaultModuleCachePath.test

Modified: 
    lldb/include/lldb/Core/ModuleList.h
    lldb/source/Core/ModuleList.cpp
    lldb/test/Shell/helper/toolchain.py

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h
index d90b27e474ac..1609f0f77c56 100644
--- a/lldb/include/lldb/Core/ModuleList.h
+++ b/lldb/include/lldb/Core/ModuleList.h
@@ -56,7 +56,7 @@ class ModuleListProperties : public Properties {
   ModuleListProperties();
 
   FileSpec GetClangModulesCachePath() const;
-  bool SetClangModulesCachePath(llvm::StringRef path);
+  bool SetClangModulesCachePath(const FileSpec &path);
   bool GetEnableExternalLookup() const;
   bool SetEnableExternalLookup(bool new_value);
 

diff  --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp
index cf276ba65931..b6e1ceb40889 100644
--- a/lldb/source/Core/ModuleList.cpp
+++ b/lldb/source/Core/ModuleList.cpp
@@ -82,8 +82,9 @@ ModuleListProperties::ModuleListProperties() {
                                            [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 {
@@ -104,8 +105,8 @@ FileSpec ModuleListProperties::GetClangModulesCachePath() const {
       ->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);
 }
 

diff  --git a/lldb/test/Shell/Settings/TestDefaultModuleCachePath.test b/lldb/test/Shell/Settings/TestDefaultModuleCachePath.test
new file mode 100644
index 000000000000..80dc0f25ce58
--- /dev/null
+++ b/lldb/test/Shell/Settings/TestDefaultModuleCachePath.test
@@ -0,0 +1,9 @@
+# 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) = ""

diff  --git a/lldb/test/Shell/helper/toolchain.py b/lldb/test/Shell/helper/toolchain.py
index ebf9e03d81a0..59b078411c1c 100644
--- a/lldb/test/Shell/helper/toolchain.py
+++ b/lldb/test/Shell/helper/toolchain.py
@@ -54,6 +54,10 @@ def use_lldb_substitutions(config):
                   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=[],