[clang-tools-extra] 68e230a - [clangd] Perform system include extraction inside CommandMangler
Nathan Ridge via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 7 15:03:10 PST 2022
Author: Nathan Ridge
Date: 2022-11-07T17:58:39-05:00
New Revision: 68e230aa29f71ed840a0ea9c0be97c8c6ead1c69
URL: https://github.com/llvm/llvm-project/commit/68e230aa29f71ed840a0ea9c0be97c8c6ead1c69
DIFF: https://github.com/llvm/llvm-project/commit/68e230aa29f71ed840a0ea9c0be97c8c6ead1c69.diff
LOG: [clangd] Perform system include extraction inside CommandMangler
It needs to run after edits from config files are applied to
the compile command (because the config file may specify the
compiler), and before resolveDriver() runs at the end of
CommandMangler.
As part of this change, QueryDriverDatabase is renamed to
SystemIncludeExtractor and is no longer a GlobalCompilationDatabase.
Fixes https://github.com/clangd/clangd/issues/1089
Fixes https://github.com/clangd/clangd/issues/1173
Fixes https://github.com/clangd/clangd/issues/1263
Differential Revision: https://reviews.llvm.org/D133757
Added:
Modified:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/GlobalCompilationDatabase.h
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
clang-tools-extra/clangd/tool/Check.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index b84d1c706d406..01cd178c5b35c 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -502,10 +502,10 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
CDBOpts.ContextProvider = Opts.ContextProvider;
BaseCDB =
std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
- BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
- std::move(BaseCDB));
}
auto Mangler = CommandMangler::detect();
+ Mangler.SystemIncludeExtractor =
+ getSystemIncludeExtractor(llvm::makeArrayRef(Opts.QueryDriverGlobs));
if (Opts.ResourceDir)
Mangler.ResourceDir = *Opts.ResourceDir;
CDB.emplace(BaseCDB.get(), Params.initializationOptions.fallbackFlags,
@@ -1815,5 +1815,6 @@ void ClangdLSPServer::onSemanticsMaybeChanged(PathRef File) {
});
}
}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index 47e92bdf4ec37..e84eb0aa30328 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -301,6 +301,17 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
for (auto &Edit : Config::current().CompileFlags.Edits)
Edit(Cmd);
+ // The system include extractor needs to run:
+ // - AFTER transferCompileCommand(), because the -x flag it adds may be
+ // necessary for the system include extractor to identify the file type
+ // - AFTER applying CompileFlags.Edits, because the name of the compiler
+ // that needs to be invoked may come from the CompileFlags->Compiler key
+ // - BEFORE resolveDriver() because that can mess up the driver path,
+ // e.g. changing gcc to /path/to/clang/bin/gcc
+ if (SystemIncludeExtractor) {
+ SystemIncludeExtractor(Command, File);
+ }
+
// Check whether the flag exists, either as -flag or -flag=*
auto Has = [&](llvm::StringRef Flag) {
for (llvm::StringRef Arg : Cmd) {
diff --git a/clang-tools-extra/clangd/CompileCommands.h b/clang-tools-extra/clangd/CompileCommands.h
index 2139b0602809e..3cf41afd4ccf1 100644
--- a/clang-tools-extra/clangd/CompileCommands.h
+++ b/clang-tools-extra/clangd/CompileCommands.h
@@ -32,6 +32,7 @@ struct CommandMangler {
llvm::Optional<std::string> ResourceDir;
// Root for searching for standard library (passed to -isysroot).
llvm::Optional<std::string> Sysroot;
+ SystemIncludeExtractorFn SystemIncludeExtractor;
// A command-mangler that doesn't know anything about the system.
// This is hermetic for unit-tests, but won't work well in production.
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.h b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
index ae8ef97e1ebd2..c0d751f82f9bb 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -162,11 +162,12 @@ class DirectoryBasedGlobalCompilationDatabase
};
/// Extracts system include search path from drivers matching QueryDriverGlobs
-/// and adds them to the compile flags. Base may not be nullptr.
-/// Returns Base when \p QueryDriverGlobs is empty.
-std::unique_ptr<GlobalCompilationDatabase>
-getQueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
- std::unique_ptr<GlobalCompilationDatabase> Base);
+/// and adds them to the compile flags.
+/// Returns null when \p QueryDriverGlobs is empty.
+using SystemIncludeExtractorFn = llvm::unique_function<void(
+ tooling::CompileCommand &, llvm::StringRef) const>;
+SystemIncludeExtractorFn
+getSystemIncludeExtractor(llvm::ArrayRef<std::string> QueryDriverGlobs);
/// Wraps another compilation database, and supports overriding the commands
/// using an in-memory mapping.
diff --git a/clang-tools-extra/clangd/QueryDriverDatabase.cpp b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
index c36fb4f042a9f..c5c5c84b05fbe 100644
--- a/clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ b/clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -315,24 +315,20 @@ llvm::Regex convertGlobsToRegex(llvm::ArrayRef<std::string> Globs) {
/// Extracts system includes from a trusted driver by parsing the output of
/// include search path and appends them to the commands coming from underlying
/// compilation database.
-class QueryDriverDatabase : public DelegatingCDB {
+class SystemIncludeExtractor {
public:
- QueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
- std::unique_ptr<GlobalCompilationDatabase> Base)
- : DelegatingCDB(std::move(Base)),
- QueryDriverRegex(convertGlobsToRegex(QueryDriverGlobs)) {}
+ SystemIncludeExtractor(llvm::ArrayRef<std::string> QueryDriverGlobs)
+ : QueryDriverRegex(convertGlobsToRegex(QueryDriverGlobs)) {}
- llvm::Optional<tooling::CompileCommand>
- getCompileCommand(PathRef File) const override {
- auto Cmd = DelegatingCDB::getCompileCommand(File);
- if (!Cmd || Cmd->CommandLine.empty())
- return Cmd;
+ void operator()(tooling::CompileCommand &Cmd, llvm::StringRef File) const {
+ if (Cmd.CommandLine.empty())
+ return;
llvm::StringRef Lang;
- for (size_t I = 0, E = Cmd->CommandLine.size(); I < E; ++I) {
- llvm::StringRef Arg = Cmd->CommandLine[I];
+ for (size_t I = 0, E = Cmd.CommandLine.size(); I < E; ++I) {
+ llvm::StringRef Arg = Cmd.CommandLine[I];
if (Arg == "-x" && I + 1 < E)
- Lang = Cmd->CommandLine[I + 1];
+ Lang = Cmd.CommandLine[I + 1];
else if (Arg.startswith("-x"))
Lang = Arg.drop_front(2).trim();
}
@@ -341,26 +337,25 @@ class QueryDriverDatabase : public DelegatingCDB {
auto Type = driver::types::lookupTypeForExtension(Ext);
if (Type == driver::types::TY_INVALID) {
elog("System include extraction: invalid file type for {0}", Ext);
- return Cmd;
+ return;
}
Lang = driver::types::getTypeName(Type);
}
- llvm::SmallString<128> Driver(Cmd->CommandLine.front());
+ llvm::SmallString<128> Driver(Cmd.CommandLine.front());
if (llvm::any_of(Driver,
[](char C) { return llvm::sys::path::is_separator(C); }))
// Driver is a not a single executable name but instead a path (either
// relative or absolute).
- llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+ llvm::sys::fs::make_absolute(Cmd.Directory, Driver);
if (auto Info =
QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
return extractSystemIncludesAndTarget(
- Driver, Lang, Cmd->CommandLine, QueryDriverRegex);
+ Driver, Lang, Cmd.CommandLine, QueryDriverRegex);
})) {
- setTarget(addSystemIncludes(*Cmd, Info->SystemIncludes), Info->Target);
+ setTarget(addSystemIncludes(Cmd, Info->SystemIncludes), Info->Target);
}
- return Cmd;
}
private:
@@ -370,14 +365,11 @@ class QueryDriverDatabase : public DelegatingCDB {
};
} // namespace
-std::unique_ptr<GlobalCompilationDatabase>
-getQueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
- std::unique_ptr<GlobalCompilationDatabase> Base) {
- assert(Base && "Null base to SystemIncludeExtractor");
+SystemIncludeExtractorFn
+getSystemIncludeExtractor(llvm::ArrayRef<std::string> QueryDriverGlobs) {
if (QueryDriverGlobs.empty())
- return Base;
- return std::make_unique<QueryDriverDatabase>(QueryDriverGlobs,
- std::move(Base));
+ return nullptr;
+ return SystemIncludeExtractor(QueryDriverGlobs);
}
} // namespace clangd
diff --git a/clang-tools-extra/clangd/test/system-include-extractor.test b/clang-tools-extra/clangd/test/system-include-extractor.test
index b109aa67aad1c..ba6aaf6efb9de 100644
--- a/clang-tools-extra/clangd/test/system-include-extractor.test
+++ b/clang-tools-extra/clangd/test/system-include-extractor.test
@@ -40,7 +40,9 @@
# RUN: sed -E -e 's|"file://([A-Z]):/|"file:///\1:/|g' %t.test.1 > %t.test
# Bless the mock driver we've just created so that clangd can execute it.
-# RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test | FileCheck -strict-whitespace %t.test
+# Note: include clangd's stderr in the FileCheck input with "2>&1" so that we
+# can match output lines like "ASTWorker building file"
+# RUN: clangd -lit-test -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
---
{
@@ -55,10 +57,25 @@
}
}
}
+# Look for the "ASTWorker building file" line so that the subsequent diagnostics
+# that are matches are for the C++ source file and not a config file.
+# CHECK: ASTWorker building file
# CHECK: "method": "textDocument/publishDiagnostics",
# CHECK-NEXT: "params": {
# CHECK-NEXT: "diagnostics": [],
+# CHECK-NEXT: "uri": "file://INPUT_DIR/the-file.cpp",
---
{"jsonrpc":"2.0","id":10000,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
+
+# Generate a
diff erent compile_commands.json which does not point to the mock driver
+# RUN: echo '[{"directory": "%/t.dir", "command": "gcc the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+
+# Generate a clangd config file which points to the mock driver instead
+# RUN: echo 'CompileFlags:' > %t.dir/.clangd
+# RUN: echo ' Compiler: my_driver.sh' >> %t.dir/.clangd
+
+# Run clangd a second time, to make sure it picks up the driver name from the config file
+# Note, we need to pass -enable-config because -lit-test otherwise disables it
+# RUN: clangd -lit-test -enable-config -query-driver="**.test,**.sh" < %t.test 2>&1 | FileCheck -strict-whitespace %t.test
diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
index 64ada30c084fa..d216c9d08e89a 100644
--- a/clang-tools-extra/clangd/tool/Check.cpp
+++ b/clang-tools-extra/clangd/tool/Check.cpp
@@ -101,9 +101,9 @@ class Checker {
Config::current().CompileFlags.CDBSearch.FixedCDBPath;
std::unique_ptr<GlobalCompilationDatabase> BaseCDB =
std::make_unique<DirectoryBasedGlobalCompilationDatabase>(CDBOpts);
- BaseCDB = getQueryDriverDatabase(llvm::makeArrayRef(Opts.QueryDriverGlobs),
- std::move(BaseCDB));
auto Mangler = CommandMangler::detect();
+ Mangler.SystemIncludeExtractor =
+ getSystemIncludeExtractor(llvm::makeArrayRef(Opts.QueryDriverGlobs));
if (Opts.ResourceDir)
Mangler.ResourceDir = *Opts.ResourceDir;
auto CDB = std::make_unique<OverlayCDB>(
More information about the cfe-commits
mailing list