diff --git a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h --- a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h @@ -32,7 +32,7 @@ /// 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; } @@ -57,9 +57,6 @@ 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() {} diff --git a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp --- a/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp @@ -57,9 +57,38 @@ 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 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( @@ -78,7 +107,8 @@ 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"}; } } diff --git a/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm b/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm --- a/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm +++ b/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm @@ -2,7 +2,7 @@ // 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 { diff --git a/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/vector new file mode 100644 diff --git a/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h b/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/stdio.h rename from lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h rename to lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/stdio.h diff --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector --- a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector +++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector @@ -1,4 +1,4 @@ -#include "libc_header.h" +#include "stdio.h" namespace std { inline namespace __1 { diff --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/stdio.h rename from lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h rename to lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/stdio.h diff --git a/lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile new file mode 100644 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile @@ -0,0 +1,10 @@ +# 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 diff --git a/lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py new file mode 100644 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py @@ -0,0 +1,60 @@ +""" +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'"]) diff --git a/lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp new file mode 100644 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp @@ -0,0 +1,8 @@ +#include + +int main(int argc, char **argv) { + // Makes sure we have the mock libc headers in the debug information. + libc_struct s; + std::vector v; + return 0; // Set break point at this line. +} diff --git a/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap new file mode 100644 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap @@ -0,0 +1,3 @@ +module std { + module "vector" { header "vector" export * } +} diff --git a/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector new file mode 100644 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector @@ -0,0 +1,9 @@ +#include "stdio.h" + +namespace std { + inline namespace __1 { + template + struct vector { + }; + } +} diff --git a/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/libc_header.h b/lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/stdio.h rename from lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/libc_header.h rename to lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/stdio.h diff --git a/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm b/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm --- a/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm +++ b/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm @@ -1,4 +1,4 @@ -#include "libc_header.h" +#include "stdio.h" namespace std { // Makes sure we get a support file for this header. diff --git a/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/vector new file mode 100644 diff --git a/lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h b/lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/stdio.h rename from lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h rename to lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/stdio.h diff --git a/lldb/unittests/Expression/CppModuleConfigurationTest.cpp b/lldb/unittests/Expression/CppModuleConfigurationTest.cpp --- a/lldb/unittests/Expression/CppModuleConfigurationTest.cpp +++ b/lldb/unittests/Expression/CppModuleConfigurationTest.cpp @@ -11,6 +11,7 @@ #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" @@ -19,7 +20,33 @@ namespace { struct CppModuleConfigurationTest : public testing::Test { - SubsystemRAII subsystems; + llvm::MemoryBufferRef m_empty_buffer; + llvm::IntrusiveRefCntPtr m_fs; + + CppModuleConfigurationTest() + : m_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 paths) { + FileSpecList result; + for (const std::string &path : paths) { + result.Append(FileSpec(path, FileSpec::Style::posix)); + if (!m_fs->addFileNoOwn(path, static_cast(0), m_empty_buffer)) + llvm_unreachable("Invalid test configuration?"); + } + return result; + } }; } // namespace @@ -31,20 +58,18 @@ return std::string(resource_dir); } -/// Utility function turningn a list of paths into a FileSpecList. -static FileSpecList makeFiles(llvm::ArrayRef 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 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)); @@ -52,10 +77,15 @@ 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 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)); @@ -63,10 +93,15 @@ 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 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)); @@ -74,10 +109,17 @@ 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 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)); @@ -85,12 +127,19 @@ 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 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)); @@ -98,8 +147,14 @@ 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 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(), @@ -109,8 +164,15 @@ 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 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(), @@ -119,22 +181,40 @@ 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 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 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 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()); } @@ -142,9 +222,20 @@ 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 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()); } @@ -152,9 +243,21 @@ 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 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()); }