This revision was automatically updated to reflect the committed changes.
Closed by commit rG99b7b41edf4f: [lldb][import-std-module] Do some basic file
checks before trying to import a… (authored by teemperor).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
Changed prior to commit:
https://reviews.llvm.org/D95096?vs=318068&id=318145#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95096/new/
https://reviews.llvm.org/D95096
Files:
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp
lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm
lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/vector
lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/libc_header.h
lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/stdio.h
lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h
lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/stdio.h
lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile
lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py
lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp
lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap
lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector
lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/stdio.h
lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm
lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/vector
lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/libc_header.h
lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/stdio.h
lldb/unittests/Expression/CppModuleConfigurationTest.cpp
Index: lldb/unittests/Expression/CppModuleConfigurationTest.cpp
===================================================================
--- lldb/unittests/Expression/CppModuleConfigurationTest.cpp
+++ 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<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
@@ -31,20 +58,18 @@
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));
@@ -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<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));
@@ -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<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));
@@ -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<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));
@@ -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<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));
@@ -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<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(),
@@ -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<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(),
@@ -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<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());
}
@@ -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<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());
}
@@ -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<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());
}
Index: lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm
===================================================================
--- lldb/test/API/commands/expression/import-std-module/sysroot/root/usr/include/c++/v1/algorithm
+++ 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.
Index: lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/vector
===================================================================
--- /dev/null
+++ 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<typename T>
+ struct vector {
+ };
+ }
+}
Index: lldb/test/API/commands/expression/import-std-module/missing-module-sources/root/usr/include/c++/v1/module.modulemap
===================================================================
--- /dev/null
+++ 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 * }
+}
Index: lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp
===================================================================
--- /dev/null
+++ lldb/test/API/commands/expression/import-std-module/missing-module-sources/main.cpp
@@ -0,0 +1,8 @@
+#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.
+}
Index: lldb/test/API/commands/expression/import-std-module/missing-module-sources/TestStdModuleSourcesMissing.py
===================================================================
--- /dev/null
+++ 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'"])
Index: lldb/test/API/commands/expression/import-std-module/missing-module-sources/Makefile
===================================================================
--- /dev/null
+++ 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
Index: lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
===================================================================
--- lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
+++ 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 {
Index: lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm
===================================================================
--- lldb/test/API/commands/expression/import-std-module/empty-module/root/usr/include/c++/v1/algorithm
+++ 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 {
Index: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.h
+++ 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() {}
Index: lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/CppModuleConfiguration.cpp
+++ 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<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(
@@ -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"};
}
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
