sammccall created this revision.
sammccall added reviewers: kadircet, madscientist.
Herald added a subscriber: arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Most headers that we discover are likely to be fairly portable across at least
clang/gcc, which is why we get away with adding them to clangd's search path.
This is not the case for the compiler builtin headers, which are shipped with
the parser and rely on parser builtins/features. We're better off *not* using
the the scanned builtin headers, and hoping our own intrinsics provide the
interface the code is relying on.
Fixes https://github.com/clangd/clangd/issues/1695
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156044
Files:
clang-tools-extra/clangd/SystemIncludeExtractor.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===================================================================
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -10,6 +10,7 @@
# %temp_dir%/my/dir2 as include search paths.
# RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
# RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ "$1" = "-print-file-name=include" ] && echo "%t.dir/builtin" && exit' >> %t.dir/bin/my_driver.sh
# RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
# Check that clangd preserves certain flags like `-nostdinc` from
# original invocation in compile_commands.json.
@@ -21,6 +22,7 @@
# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/bin/my_driver.sh
# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/builtin >&2' >> %t.dir/bin/my_driver.sh
# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
# RUN: chmod +x %t.dir/bin/my_driver.sh
@@ -29,6 +31,8 @@
# RUN: touch %t.dir/my/dir/a.h
# RUN: mkdir -p %t.dir/my/dir2
# RUN: touch %t.dir/my/dir2/b.h
+# RUN: mkdir -p %t.dir/builtin
+# RUN: touch %t.dir/builtin/c.h
# Generate a compile_commands.json that will query the mock driver we've
# created. Which should add a.h and b.h into include search path.
@@ -53,7 +57,7 @@
"uri": "file://INPUT_DIR/the-file.cpp",
"languageId":"cpp",
"version":1,
- "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif\n"
+ "text":"#include <a.h>\n#include <b.h>\n#if !defined(__ARM_ARCH) || !defined(__gnu_linux__)\n#error \"Invalid target\"\n#endif\n#if __has_include(<c.h>)\n#error \"wrong-toolchain builtins\"\n#endif\n"
}
}
}
Index: clang-tools-extra/clangd/SystemIncludeExtractor.cpp
===================================================================
--- clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -157,7 +157,7 @@
if (!StandardCXXIncludes)
Args.push_back("-nostdinc++");
if (!BuiltinIncludes)
- Args.push_back("-nobuiltininc++");
+ Args.push_back("-nobuiltininc");
if (!Sysroot.empty())
Args.append({"--sysroot", Sysroot});
if (!ISysroot.empty())
@@ -274,6 +274,42 @@
return std::move(Info);
}
+std::optional<std::string> run(llvm::ArrayRef<llvm::StringRef> Argv,
+ bool OutputIsStderr) {
+ llvm::SmallString<128> OutputPath;
+ if (auto EC = llvm::sys::fs::createTemporaryFile("system-includes", "clangd",
+ OutputPath)) {
+ elog("System include extraction: failed to create temporary file with "
+ "error {0}",
+ EC.message());
+ return std::nullopt;
+ }
+ auto CleanUp = llvm::make_scope_exit(
+ [&OutputPath]() { llvm::sys::fs::remove(OutputPath); });
+
+ std::optional<llvm::StringRef> Redirects[] = {{""}, {""}, {""}};
+ Redirects[OutputIsStderr ? 2 : 1] = OutputPath.str();
+
+ std::string ErrMsg;
+ if (int RC =
+ llvm::sys::ExecuteAndWait(Argv.front(), Argv, /*Env=*/std::nullopt,
+ Redirects, /*SecondsToWait=*/0,
+ /*MemoryLimit=*/0, &ErrMsg)) {
+ elog("System include extraction: driver execution failed with return code: "
+ "{0} - '{1}'. Args: [{2}]",
+ llvm::to_string(RC), ErrMsg, printArgv(Argv));
+ return std::nullopt;
+ }
+
+ auto BufOrError = llvm::MemoryBuffer::getFile(OutputPath);
+ if (!BufOrError) {
+ elog("System include extraction: failed to read {0} with error {1}",
+ OutputPath, BufOrError.getError().message());
+ return std::nullopt;
+ }
+ return BufOrError.get().get()->getBuffer().str();
+}
+
std::optional<DriverInfo>
extractSystemIncludesAndTarget(const DriverArgs &InputArgs,
const llvm::Regex &QueryDriverRegex) {
@@ -300,45 +336,37 @@
return std::nullopt;
}
- llvm::SmallString<128> StdErrPath;
- if (auto EC = llvm::sys::fs::createTemporaryFile("system-includes", "clangd",
- StdErrPath)) {
- elog("System include extraction: failed to create temporary file with "
- "error {0}",
- EC.message());
- return std::nullopt;
- }
- auto CleanUp = llvm::make_scope_exit(
- [&StdErrPath]() { llvm::sys::fs::remove(StdErrPath); });
-
- std::optional<llvm::StringRef> Redirects[] = {{""}, {""}, StdErrPath.str()};
-
llvm::SmallVector<llvm::StringRef> Args = {Driver, "-E", "-v"};
Args.append(InputArgs.render());
// Input needs to go after Lang flags.
Args.push_back("-");
-
- std::string ErrMsg;
- if (int RC = llvm::sys::ExecuteAndWait(Driver, Args, /*Env=*/std::nullopt,
- Redirects, /*SecondsToWait=*/0,
- /*MemoryLimit=*/0, &ErrMsg)) {
- elog("System include extraction: driver execution failed with return code: "
- "{0} - '{1}'. Args: [{2}]",
- llvm::to_string(RC), ErrMsg, printArgv(Args));
+ auto Output = run(Args, /*OutputIsStderr=*/true);
+ if (!Output)
return std::nullopt;
- }
- auto BufOrError = llvm::MemoryBuffer::getFile(StdErrPath);
- if (!BufOrError) {
- elog("System include extraction: failed to read {0} with error {1}",
- StdErrPath, BufOrError.getError().message());
+ std::optional<DriverInfo> Info = parseDriverOutput(*Output);
+ if (!Info)
return std::nullopt;
+
+ // The built-in headers are tightly coupled to parser builtins.
+ // (These are clang's "resource dir", GCC's GCC_INCLUDE_DIR.)
+ // We should keep using clangd's versions, so exclude the queried builtins.
+ // They're not specially marked in the -v output, but we can get the path
+ // with `$DRIVER -print-file-name=include`.
+ if (auto BuiltinHeaders =
+ run({Driver, "-print-file-name=include"}, /*OutputIsStderr=*/false)) {
+ auto Path = llvm::StringRef(*BuiltinHeaders).trim();
+ if (!Path.empty() && llvm::sys::path::is_absolute(Path)) {
+ auto Size = Info->SystemIncludes.size();
+ llvm::erase_if(Info->SystemIncludes,
+ [&](llvm::StringRef Entry) { return Path == Entry; });
+ vlog("System includes extractor: builtin headers {0} {1}", Path,
+ (Info->SystemIncludes.size() != Size)
+ ? "excluded"
+ : "not found in driver's response");
+ }
}
- std::optional<DriverInfo> Info =
- parseDriverOutput(BufOrError->get()->getBuffer());
- if (!Info)
- return std::nullopt;
log("System includes extractor: successfully executed {0}\n\tgot includes: "
"\"{1}\"\n\tgot target: \"{2}\"",
Driver, llvm::join(Info->SystemIncludes, ", "), Info->Target);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
