Skip to content

Commit 99b7b41

Browse files
committed
[lldb][import-std-module] Do some basic file checks before trying to import a module
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 Reviewed By: JDevlieghere Differential Revision: https://reviews.llvm.org/D95096
1 parent ff41ae8 commit 99b7b41

File tree

17 files changed

+266
-45
lines changed

17 files changed

+266
-45
lines changed

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

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,38 @@ bool CppModuleConfiguration::analyzeFile(const FileSpec &f) {
5757
return true;
5858
}
5959

60+
/// Utility function for just appending two paths.
61+
static std::string MakePath(llvm::StringRef lhs, llvm::StringRef rhs) {
62+
llvm::SmallString<256> result(lhs);
63+
llvm::sys::path::append(result, rhs);
64+
return std::string(result);
65+
}
66+
6067
bool CppModuleConfiguration::hasValidConfig() {
61-
// We all these include directories to have a valid usable configuration.
62-
return m_c_inc.Valid() && m_std_inc.Valid();
68+
// We need to have a C and C++ include dir for a valid configuration.
69+
if (!m_c_inc.Valid() || !m_std_inc.Valid())
70+
return false;
71+
72+
// Do some basic sanity checks on the directories that we don't activate
73+
// the module when it's clear that it's not usable.
74+
const std::vector<std::string> files_to_check = {
75+
// * Check that the C library contains at least one random C standard
76+
// library header.
77+
MakePath(m_c_inc.Get(), "stdio.h"),
78+
// * Without a libc++ modulemap file we can't have a 'std' module that
79+
// could be imported.
80+
MakePath(m_std_inc.Get(), "module.modulemap"),
81+
// * Check for a random libc++ header (vector in this case) that has to
82+
// exist in a working libc++ setup.
83+
MakePath(m_std_inc.Get(), "vector"),
84+
};
85+
86+
for (llvm::StringRef file_to_check : files_to_check) {
87+
if (!FileSystem::Instance().Exists(file_to_check))
88+
return false;
89+
}
90+
91+
return true;
6392
}
6493

6594
CppModuleConfiguration::CppModuleConfiguration(
@@ -78,7 +107,8 @@ CppModuleConfiguration::CppModuleConfiguration(
78107
m_resource_inc = std::string(resource_dir.str());
79108

80109
// This order matches the way Clang orders these directories.
81-
m_include_dirs = {m_std_inc.Get(), m_resource_inc, m_c_inc.Get()};
110+
m_include_dirs = {m_std_inc.Get().str(), m_resource_inc,
111+
m_c_inc.Get().str()};
82112
m_imported_modules = {"std"};
83113
}
84114
}

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ class CppModuleConfiguration {
3232
/// the path was already set.
3333
LLVM_NODISCARD bool TrySet(llvm::StringRef path);
3434
/// Return the path if there is one.
35-
std::string Get() const {
35+
llvm::StringRef Get() const {
3636
assert(m_valid && "Called Get() on an invalid SetOncePath?");
3737
return m_path;
3838
}
@@ -57,9 +57,6 @@ class CppModuleConfiguration {
5757

5858
public:
5959
/// Creates a configuration by analyzing the given list of used source files.
60-
///
61-
/// Currently only looks at the used paths and doesn't actually access the
62-
/// files on the disk.
6360
explicit CppModuleConfiguration(const FileSpecList &support_files);
6461
/// Creates an empty and invalid configuration.
6562
CppModuleConfiguration() {}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// library module so the actual contents of the module are missing.
33
#ifdef ENABLE_STD_CONTENT
44

5-
#include "libc_header.h"
5+
#include "stdio.h"
66

77
namespace std {
88
inline namespace __1 {

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

Whitespace-only changes.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#include "libc_header.h"
1+
#include "stdio.h"
22

33
namespace std {
44
inline namespace __1 {
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# We don't have any standard include directories, so we can't
2+
# parse the test_common.h header we usually inject as it includes
3+
# system headers.
4+
NO_TEST_COMMON_H := 1
5+
6+
# Take the libc++ from the build directory (which will be later deleted).
7+
CXXFLAGS_EXTRAS = -I $(BUILDDIR)/root/usr/include/c++/v1/ -I $(BUILDDIR)/root/usr/include/ -nostdinc -nostdinc++
8+
CXX_SOURCES := main.cpp
9+
10+
include Makefile.rules
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
"""
2+
Check that missing module source files are correctly handled by LLDB.
3+
"""
4+
5+
from lldbsuite.test.decorators import *
6+
from lldbsuite.test.lldbtest import *
7+
from lldbsuite.test import lldbutil
8+
import os
9+
import shutil
10+
11+
12+
class TestCase(TestBase):
13+
14+
mydir = TestBase.compute_mydir(__file__)
15+
16+
# We only emulate a fake libc++ in this test and don't use the real libc++,
17+
# but we still add the libc++ category so that this test is only run in
18+
# test configurations where libc++ is actually supposed to be tested.
19+
@add_test_categories(["libc++"])
20+
@skipIf(compiler=no_match("clang"))
21+
def test(self):
22+
# The path to our temporary target root that contains the temporary
23+
# module sources.
24+
target_sysroot = self.getBuildArtifact("root")
25+
26+
# Copy the sources to the root.
27+
shutil.copytree(self.getSourcePath("root"), target_sysroot)
28+
# Build the binary with the copied sources.
29+
self.build()
30+
# Delete the copied sources so that they are now unavailable.
31+
shutil.rmtree(target_sysroot)
32+
33+
# Set the sysroot where our dummy libc++ used to exist. Just to make
34+
# sure we don't find some existing headers on the system that could
35+
# XPASS this test.
36+
self.runCmd("platform select --sysroot '" + target_sysroot + "' host")
37+
38+
lldbutil.run_to_source_breakpoint(self,
39+
"// Set break point at this line.",
40+
lldb.SBFileSpec("main.cpp"))
41+
42+
# Import the std C++ module and run an expression.
43+
# As we deleted the sources, LLDB should refuse the load the module
44+
# and just print the normal error we get from the expression.
45+
self.runCmd("settings set target.import-std-module true")
46+
self.expect("expr v.unknown_identifier", error=True,
47+
substrs=["no member named 'unknown_identifier'"])
48+
# Check that there is no confusing error about failing to build the
49+
# module.
50+
self.expect("expr v.unknown_identifier", error=True, matching=False,
51+
substrs=["could not build module 'std'"])
52+
53+
# Test the fallback mode. It should also just print the normal
54+
# error but not mention a failed module build.
55+
self.runCmd("settings set target.import-std-module fallback")
56+
57+
self.expect("expr v.unknown_identifier", error=True,
58+
substrs=["no member named 'unknown_identifier'"])
59+
self.expect("expr v.unknown_identifier", error=True, matching=False,
60+
substrs=["could not build module 'std'"])
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#include <vector>
2+
3+
int main(int argc, char **argv) {
4+
// Makes sure we have the mock libc headers in the debug information.
5+
libc_struct s;
6+
std::vector<int> v;
7+
return 0; // Set break point at this line.
8+
}

0 commit comments

Comments
 (0)