[lldb][import-std-module] Do some basic file checks before trying to import a module

[teemperor]

Summary

Currently when LLDB has enough data in the debug information to import the std module,
it will just try to import it. However when debugging libraries where the sources aren't
available anymore, importing the module will generate a confusing diagnostic that
the module couldn't be built.

For the fallback mode (where we retry failed expressions with the loaded module), this
will cause the second expression to fail with a module built error instead of the
actual parsing issue in the user expression.

This patch adds checks that ensures that we at least have any source files in the found
include paths before we try to import the module. This prevents the module from being
loaded in the situation described above which means we don't emit the bogus 'can't
import module' diagnostic and also don't waste any time retrying the expression in the
fallback mode.

For the unit tests I did some refactoring as they now require a VFS with the files in it
and not just the paths. The Python test just builds a binary with a fake C++ module,
then deletes the module before debugging.

Fixes rdar://73264458

[JDevlieghere]

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp

78

llvm::sys::path::append

88

The policy is that you should use the FileSystem instance unless (1) you're passing off the VFS to clang/swift (which tracks its own dependencies) or (2) when you explicitly want to bypass it for the reproducers. To spoof things in the unittests you can initialize the FileSystem with a custom VFS to achieve the same result.

[shafik]

LGTM, I will let Jonas approve since he has some issues that need addressing.

[teemperor]

• using llvm's path::append
• Now initializing and using the global FileSystem instance
• Added some stub files to tests with fake libc++ roots to make them pass the new detection mechanism.

[JDevlieghere]

LGTM!

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h

class SetOncePath {
     /// True iff this path hasn't been set yet.
     bool m_first = true;
 
   public:
     /// Try setting the path. Returns true if the path was set and false if
     /// the path was already set.
     LLVM_NODISCARD bool TrySet(llvm::StringRef path);
     /// Return the path if there is one.
-    std::string Get() const {
+    llvm::StringRef Get() const {
       assert(m_valid && "Called Get() on an invalid SetOncePath?");
       return m_path;
     }
     /// Returns true iff this path was set exactly once so far.
     bool Valid() const { return m_valid; }
   };
 
   /// If valid, the include path used for the std module.
   SetOncePath m_std_inc;
   /// If valid, the include path to the C library (e.g. /usr/include).
   SetOncePath m_c_inc;
   /// The Clang resource include path for this configuration.
   std::string m_resource_inc;
 
   std::vector<std::string> m_include_dirs;
   std::vector<std::string> m_imported_modules;
 
   /// Analyze a given source file to build the current configuration.
   /// Returns false iff there was a fatal error that makes analyzing any
   /// further files pointless as the configuration is now invalid.
   bool analyzeFile(const FileSpec &f);
 
 public:
   /// Creates a configuration by analyzing the given list of used source files.
-  ///
-  /// Currently only looks at the used paths and doesn't actually access the
-  /// files on the disk.
   explicit CppModuleConfiguration(const FileSpecList &support_files);
   /// Creates an empty and invalid configuration.
   CppModuleConfiguration() {}
 
   /// Returns true iff this is a valid configuration that can be used to
   /// load and compile modules.
   bool hasValidConfig();
 

lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp

if (posix_dir.endswith("/usr/include/bits"))
     posix_dir.consume_back("/bits");
   if (posix_dir.endswith("/usr/include"))
     return m_c_inc.TrySet(posix_dir);
 
   // File wasn't interesting, continue analyzing.
   return true;
 }
 
+/// Utility function for just appending two paths.
+static std::string MakePath(llvm::StringRef lhs, llvm::StringRef rhs) {
+  llvm::SmallString<256> result(lhs);
+  llvm::sys::path::append(result, rhs);
+  return std::string(result);
+}
+
 bool CppModuleConfiguration::hasValidConfig() {
-  // We all these include directories to have a valid usable configuration.
-  return m_c_inc.Valid() && m_std_inc.Valid();
+  // We need to have a C and C++ include dir for a valid configuration.
+  if (!m_c_inc.Valid() || !m_std_inc.Valid())
+    return false;
+
+  // Do some basic sanity checks on the directories that we don't activate
+  // the module when it's clear that it's not usable.
+  const std::vector<std::string> files_to_check = {
+      // * Check that the C library contains at least one random C standard
+      //   library header.
+      MakePath(m_c_inc.Get(), "stdio.h"),
+      // * Without a libc++ modulemap file we can't have a 'std' module that
+      //   could be imported.
+      MakePath(m_std_inc.Get(), "module.modulemap"),
+      // * Check for a random libc++ header (vector in this case) that has to
+      //   exist in a working libc++ setup.
+      MakePath(m_std_inc.Get(), "vector"),
+  };
+
+  for (llvm::StringRef file_to_check : files_to_check) {
+    if (!FileSystem::Instance().Exists(file_to_check))
+      return false;
+  }
+
+  return true;
 }
 
 CppModuleConfiguration::CppModuleConfiguration(
     const FileSpecList &support_files) {
   // Analyze all files we were given to build the configuration.
   bool error = !llvm::all_of(support_files,
                              std::bind(&CppModuleConfiguration::analyzeFile,
                                        this, std::placeholders::_1));
   // If we have a valid configuration at this point, set the
   // include directories and module list that should be used.
   if (!error && hasValidConfig()) {
     // Calculate the resource directory for LLDB.
     llvm::SmallString<256> resource_dir;
     llvm::sys::path::append(resource_dir, GetClangResourceDir().GetPath(),
                             "include");
     m_resource_inc = std::string(resource_dir.str());
 
     // This order matches the way Clang orders these directories.
-    m_include_dirs = {m_std_inc.Get(), m_resource_inc, m_c_inc.Get()};
+    m_include_dirs = {m_std_inc.Get().str(), m_resource_inc,
+                      m_c_inc.Get().str()};
     m_imported_modules = {"std"};
   }
 }

lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm

 // This is only defined when building, but LLDB is missing this flag when loading the standard
 // library module so the actual contents of the module are missing.
 #ifdef ENABLE_STD_CONTENT
 
-#include "libc_header.h"
+#include "stdio.h"
 
 namespace std {
   inline namespace __1 {
     // Pretend to be a std::vector template we need to instantiate
     // in LLDB.
     template<typename T>
     struct vector { T i; int size() { return 2; } };
   }

lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector

-#include "libc_header.h"
+#include "stdio.h"
 
 namespace std {
   inline namespace __1 {
     // A forward decl of `vector`.
     template<typename T> class vector;
     // Pretend to be a std::vector template we need to instantiate in LLDB
     // when import-std-module is enabled.
     template<typename T>
     struct vector { class F; F *f = nullptr; };
     // The definition of our forward declared nested class.
     template<typename T> class vector<T>::F { int x; };
   }
 }

lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile

+# We don't have any standard include directories, so we can't
+# parse the test_common.h header we usually inject as it includes
+# system headers.
+NO_TEST_COMMON_H := 1
+
+# Take the libc++ from the build directory (which will be later deleted).
+CXXFLAGS_EXTRAS = -I $(BUILDDIR)/root/usr/include/c++/v1/ -I $(BUILDDIR)/root/usr/include/ -nostdinc -nostdinc++
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py

+"""
+Check that missing module source files are correctly handled by LLDB.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import os
+import shutil
+
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    # We only emulate a fake libc++ in this test and don't use the real libc++,
+    # but we still add the libc++ category so that this test is only run in
+    # test configurations where libc++ is actually supposed to be tested.
+    @add_test_categories(["libc++"])
+    @skipIf(compiler=no_match("clang"))
+    def test(self):
+        # The path to our temporary target root that contains the temporary
+        # module sources.
+        target_sysroot = self.getBuildArtifact("root")
+
+        # Copy the sources to the root.
+        shutil.copytree(self.getSourcePath("root"), target_sysroot)
+        # Build the binary with the copied sources.
+        self.build()
+        # Delete the copied sources so that they are now unavailable.
+        shutil.rmtree(target_sysroot)
+
+        # Set the sysroot where our dummy libc++ used to exist. Just to make
+        # sure we don't find some existing headers on the system that could
+        # XPASS this test.
+        self.runCmd("platform select --sysroot '" + target_sysroot + "' host")
+
+        lldbutil.run_to_source_breakpoint(self,
+                                          "// Set break point at this line.",
+                                          lldb.SBFileSpec("main.cpp"))
+
+        # Import the std C++ module and run an expression.
+        # As we deleted the sources, LLDB should refuse the load the module
+        # and just print the normal error we get from the expression.
+        self.runCmd("settings set target.import-std-module true")
+        self.expect("expr v.unknown_identifier", error=True,
+                    substrs=["no member named 'unknown_identifier'"])
+        # Check that there is no confusing error about failing to build the
+        # module.
+        self.expect("expr v.unknown_identifier", error=True, matching=False,
+                    substrs=["could not build module 'std'"])
+
+        # Test the fallback mode. It should also just print the normal
+        # error but not mention a failed module build.
+        self.runCmd("settings set target.import-std-module fallback")
+
+        self.expect("expr v.unknown_identifier", error=True,
+                     substrs=["no member named 'unknown_identifier'"])
+        self.expect("expr v.unknown_identifier", error=True, matching=False,
+                    substrs=["could not build module 'std'"])

lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp

+#include <vector>
+
+int main(int argc, char **argv) {
+  // Makes sure we have the mock libc headers in the debug information.
+  libc_struct s;
+  std::vector<int> v;
+  return 0; // Set break point at this line.
+}

lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap

+module std {
+  module "vector" { header "vector" export * }
+}

lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector

+#include "stdio.h"
+
+namespace std {
+  inline namespace __1 {
+    template<typename T>
+    struct vector {
+    };
+  }
+}

lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm

-#include "libc_header.h"
+#include "stdio.h"
 
 namespace std {
   // Makes sure we get a support file for this header.
   struct vector { int i; };
 
   __attribute__((__nodebug__))
   inline int myabs(int i = -123) {
     double nil;
     return i < 0 ? -i : i;
   }
 }

lldb/unittests/Expression/CppModuleConfigurationTest.cpp

 //===-- CppModuleConfigurationTest.cpp ------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/ExpressionParser/Clang/CppModuleConfiguration.h"
 #include "Plugins/ExpressionParser/Clang/ClangHost.h"
 #include "TestingSupport/SubsystemRAII.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
+#include "llvm/Support/SmallVectorMemoryBuffer.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace lldb_private;
 
 namespace {
 struct CppModuleConfigurationTest : public testing::Test {
-  SubsystemRAII<FileSystem, HostInfo> subsystems;
+  llvm::MemoryBufferRef m_empty_buffer;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> m_fs;
+
+  CppModuleConfigurationTest()
+      : m_empty_buffer("", "<empty buffer>"),
+        m_fs(new llvm::vfs::InMemoryFileSystem()) {}
+
+  void SetUp() override {
+    FileSystem::Initialize(m_fs);
+    HostInfo::Initialize();
+  }
+
+  void TearDown() override {
+    HostInfo::Terminate();
+    FileSystem::Terminate();
+  }
+
+  /// Utility function turning a list of paths into a FileSpecList.
+  FileSpecList makeFiles(llvm::ArrayRef<std::string> paths) {
+    FileSpecList result;
+    for (const std::string &path : paths) {
+      result.Append(FileSpec(path, FileSpec::Style::posix));
+      if (!m_fs->addFileNoOwn(path, static_cast<time_t>(0), m_empty_buffer))
+        llvm_unreachable("Invalid test configuration?");
+    }
+    return result;
+  }
 };
 } // namespace
 
 /// Returns the Clang resource include directory.
 static std::string ResourceInc() {
   llvm::SmallString<256> resource_dir;
   llvm::sys::path::append(resource_dir, GetClangResourceDir().GetPath(),
                           "include");
   return std::string(resource_dir);
 }
 
-/// Utility function turningn a list of paths into a FileSpecList.
-static FileSpecList makeFiles(llvm::ArrayRef<std::string> paths) {
-  FileSpecList result;
-  for (const std::string &path : paths)
-    result.Append(FileSpec(path, FileSpec::Style::posix));
-  return result;
-}
 
 TEST_F(CppModuleConfigurationTest, Linux) {
   // Test the average Linux configuration.
-  std::string libcpp = "/usr/include/c++/v1";
+
   std::string usr = "/usr/include";
-  CppModuleConfiguration config(
-      makeFiles({usr + "/bits/types.h", libcpp + "/vector"}));
+  std::string libcpp = "/usr/include/c++/v1";
+  std::vector<std::string> files = {// C library
+                                    usr + "/stdio.h",
+                                    // C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre(libcpp, ResourceInc(), usr));
 }
 
 TEST_F(CppModuleConfigurationTest, Sysroot) {
   // Test that having a sysroot for the whole system works fine.
+
   std::string libcpp = "/home/user/sysroot/usr/include/c++/v1";
   std::string usr = "/home/user/sysroot/usr/include";
-  CppModuleConfiguration config(
-      makeFiles({usr + "/bits/types.h", libcpp + "/vector"}));
+  std::vector<std::string> files = {// C library
+                                    usr + "/stdio.h",
+                                    // C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre(libcpp, ResourceInc(), usr));
 }
 
 TEST_F(CppModuleConfigurationTest, LinuxLocalLibCpp) {
   // Test that a locally build libc++ is detected.
-  std::string libcpp = "/home/user/llvm-build/include/c++/v1";
+
   std::string usr = "/usr/include";
-  CppModuleConfiguration config(
-      makeFiles({usr + "/bits/types.h", libcpp + "/vector"}));
+  std::string libcpp = "/home/user/llvm-build/include/c++/v1";
+  std::vector<std::string> files = {// C library
+                                    usr + "/stdio.h",
+                                    // C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre(libcpp, ResourceInc(), usr));
 }
 
 TEST_F(CppModuleConfigurationTest, UnrelatedLibrary) {
   // Test that having an unrelated library in /usr/include doesn't break.
-  std::string libcpp = "/home/user/llvm-build/include/c++/v1";
+
   std::string usr = "/usr/include";
-  CppModuleConfiguration config(makeFiles(
-      {usr + "/bits/types.h", libcpp + "/vector", usr + "/boost/vector"}));
+  std::string libcpp = "/home/user/llvm-build/include/c++/v1";
+  std::vector<std::string> files = {// C library
+                                    usr + "/stdio.h",
+                                    // unrelated library
+                                    usr + "/boost/vector",
+                                    // C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre(libcpp, ResourceInc(), usr));
 }
 
 TEST_F(CppModuleConfigurationTest, Xcode) {
   // Test detection of libc++ coming from Xcode with generic platform names.
+
   std::string p = "/Applications/Xcode.app/Contents/Developer/";
   std::string libcpp = p + "Toolchains/B.xctoolchain/usr/include/c++/v1";
   std::string usr =
       p + "Platforms/A.platform/Developer/SDKs/OSVers.sdk/usr/include";
-  CppModuleConfiguration config(
-      makeFiles({libcpp + "/unordered_map", usr + "/stdio.h"}));
+  std::vector<std::string> files = {
+      // C library
+      usr + "/stdio.h",
+      // C++ library
+      libcpp + "/vector",
+      libcpp + "/module.modulemap",
+  };
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre(libcpp, ResourceInc(), usr));
 }
 
 TEST_F(CppModuleConfigurationTest, LibCppV2) {
   // Test that a "v2" of libc++ is still correctly detected.
-  CppModuleConfiguration config(
-      makeFiles({"/usr/include/bits/types.h", "/usr/include/c++/v2/vector"}));
+
+  std::string libcpp = "/usr/include/c++/v2";
+  std::vector<std::string> files = {// C library
+                                    "/usr/include/stdio.h",
+                                    // C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre("/usr/include/c++/v2", ResourceInc(),
                                    "/usr/include"));
 }
 
 TEST_F(CppModuleConfigurationTest, UnknownLibCppFile) {
   // Test that having some unknown file in the libc++ path doesn't break
   // anything.
-  CppModuleConfiguration config(makeFiles(
-      {"/usr/include/bits/types.h", "/usr/include/c++/v1/non_existing_file"}));
+
+  std::string libcpp = "/usr/include/c++/v1";
+  std::vector<std::string> files = {// C library
+                                    "/usr/include/stdio.h",
+                                    // C++ library
+                                    libcpp + "/non_existing_file",
+                                    libcpp + "/module.modulemap",
+                                    libcpp + "/vector"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre("std"));
   EXPECT_THAT(config.GetIncludeDirs(),
               testing::ElementsAre("/usr/include/c++/v1", ResourceInc(),
                                    "/usr/include"));
 }
 
 TEST_F(CppModuleConfigurationTest, MissingUsrInclude) {
   // Test that we don't load 'std' if we can't find the C standard library.
-  CppModuleConfiguration config(makeFiles({"/usr/include/c++/v1/vector"}));
+
+  std::string libcpp = "/usr/include/c++/v1";
+  std::vector<std::string> files = {// C++ library
+                                    libcpp + "/vector",
+                                    libcpp + "/module.modulemap"};
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre());
   EXPECT_THAT(config.GetIncludeDirs(), testing::ElementsAre());
 }
 
 TEST_F(CppModuleConfigurationTest, MissingLibCpp) {
   // Test that we don't load 'std' if we don't have a libc++.
-  CppModuleConfiguration config(makeFiles({"/usr/include/bits/types.h"}));
+
+  std::string usr = "/usr/include";
+  std::vector<std::string> files = {
+      // C library
+      usr + "/stdio.h",
+  };
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre());
   EXPECT_THAT(config.GetIncludeDirs(), testing::ElementsAre());
 }
 
 TEST_F(CppModuleConfigurationTest, IgnoreLibStdCpp) {
   // Test that we don't do anything bad when we encounter libstdc++ paths.
-  CppModuleConfiguration config(makeFiles(
-      {"/usr/include/bits/types.h", "/usr/include/c++/8.0.1/vector"}));
+
+  std::string usr = "/usr/include";
+  std::vector<std::string> files = {
+      // C library
+      usr + "/stdio.h",
+      // C++ library
+      usr + "/c++/8.0.1/vector",
+  };
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre());
   EXPECT_THAT(config.GetIncludeDirs(), testing::ElementsAre());
 }
 
 TEST_F(CppModuleConfigurationTest, AmbiguousCLib) {
   // Test that we don't do anything when we are not sure where the
   // right C standard library is.
-  CppModuleConfiguration config(
-      makeFiles({"/usr/include/bits/types.h", "/usr/include/c++/v1/vector",
-                 "/sysroot/usr/include/bits/types.h"}));
+
+  std::string usr1 = "/usr/include";
+  std::string usr2 = "/usr/include/other/path";
+  std::string libcpp = usr1 + "c++/v1";
+  std::vector<std::string> files = {
+      // First C library
+      usr1 + "/stdio.h",
+      // Second C library
+      usr2 + "/stdio.h",
+      // C++ library
+      libcpp + "/vector",
+      libcpp + "/module.modulemap",
+  };
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre());
   EXPECT_THAT(config.GetIncludeDirs(), testing::ElementsAre());
 }
 
 TEST_F(CppModuleConfigurationTest, AmbiguousLibCpp) {
   // Test that we don't do anything when we are not sure where the
   // right libc++ is.
-  CppModuleConfiguration config(
-      makeFiles({"/usr/include/bits/types.h", "/usr/include/c++/v1/vector",
-                 "/usr/include/c++/v2/vector"}));
+
+  std::string usr = "/usr/include";
+  std::string libcpp1 = usr + "c++/v1";
+  std::string libcpp2 = usr + "c++/v2";
+  std::vector<std::string> files = {
+      // C library
+      usr + "/stdio.h",
+      // First C++ library
+      libcpp1 + "/vector",
+      libcpp1 + "/module.modulemap",
+      // Second C++ library
+      libcpp2 + "/vector",
+      libcpp2 + "/module.modulemap",
+  };
+  CppModuleConfiguration config(makeFiles(files));
   EXPECT_THAT(config.GetImportedModules(), testing::ElementsAre());
   EXPECT_THAT(config.GetIncludeDirs(), testing::ElementsAre());
 }