[clang-tools-extra] 73cf485 - [clangd] Expand response files before CDB interpolation (#75753)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 19 00:25:53 PST 2023
Author: Dmitry Polukhin
Date: 2023-12-19T03:25:48-05:00
New Revision: 73cf485151190fc61bf3cff996fe8ee3eea3a257
URL: https://github.com/llvm/llvm-project/commit/73cf485151190fc61bf3cff996fe8ee3eea3a257
DIFF: https://github.com/llvm/llvm-project/commit/73cf485151190fc61bf3cff996fe8ee3eea3a257.diff
LOG: [clangd] Expand response files before CDB interpolation (#75753)
Summary:
After https://reviews.llvm.org/D143436 response files stopped working
with CDB interpolation. It has happened because interpolation removes
all unknown flags and extra input files. Response file is treated as an
extra input because it is not a flag. Moreover inference needs full
command line for driver mode and file type detection so all response
files have to be expanded for correct inference.
This patch partially reverts D143436 and add additional response file
expansion in OverlayCDB for CDBs pushed via LSP.
Test Plan: check-clangd
Tasks: https://github.com/llvm/llvm-project/issues/69690
Added:
Modified:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index f43ce928463b90..f4e8e7e74a3bee 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -28,7 +28,6 @@
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-#include "llvm/TargetParser/Host.h"
#include <iterator>
#include <optional>
#include <string>
@@ -187,12 +186,6 @@ static std::string resolveDriver(llvm::StringRef Driver, bool FollowSymlink,
} // namespace
-CommandMangler::CommandMangler() {
- Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
- ? llvm::cl::TokenizeWindowsCommandLine
- : llvm::cl::TokenizeGNUCommandLine;
-}
-
CommandMangler CommandMangler::detect() {
CommandMangler Result;
Result.ClangPath = detectClangPath();
@@ -213,14 +206,6 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
if (Cmd.empty())
return;
- // FS used for expanding response files.
- // FIXME: ExpandResponseFiles appears not to provide the usual
- // thread-safety guarantees, as the access to FS is not locked!
- // For now, use the real FS, which is known to be threadsafe (if we don't
- // use/change working directory, which ExpandResponseFiles doesn't).
- auto FS = llvm::vfs::getRealFileSystem();
- tooling::addExpandedResponseFiles(Cmd, Command.Directory, Tokenizer, *FS);
-
auto &OptTable = clang::driver::getDriverOptTable();
// OriginalArgs needs to outlive ArgList.
llvm::SmallVector<const char *, 16> OriginalArgs;
diff --git a/clang-tools-extra/clangd/CompileCommands.h b/clang-tools-extra/clangd/CompileCommands.h
index 1b37f44f0b9db4..1ee0dba2dba80d 100644
--- a/clang-tools-extra/clangd/CompileCommands.h
+++ b/clang-tools-extra/clangd/CompileCommands.h
@@ -51,11 +51,8 @@ struct CommandMangler {
llvm::StringRef TargetFile) const;
private:
- CommandMangler();
-
Memoize<llvm::StringMap<std::string>> ResolvedDrivers;
Memoize<llvm::StringMap<std::string>> ResolvedDriversNoFollow;
- llvm::cl::TokenizerCallback Tokenizer;
};
// Removes args from a command-line in a semantically-aware way.
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index d1833759917a30..5bec7966a9c3a9 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -18,6 +18,7 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "clang/Tooling/CompilationDatabasePluginRegistry.h"
#include "clang/Tooling/JSONCompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/PointerIntPair.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
@@ -25,6 +26,7 @@
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/TargetParser/Host.h"
#include <atomic>
#include <chrono>
#include <condition_variable>
@@ -244,7 +246,16 @@ static std::unique_ptr<tooling::CompilationDatabase>
parseJSON(PathRef Path, llvm::StringRef Data, std::string &Error) {
if (auto CDB = tooling::JSONCompilationDatabase::loadFromBuffer(
Data, Error, tooling::JSONCommandLineSyntax::AutoDetect)) {
- return tooling::inferMissingCompileCommands(std::move(CDB));
+ // FS used for expanding response files.
+ // FIXME: ExpandResponseFilesDatabase appears not to provide the usual
+ // thread-safety guarantees, as the access to FS is not locked!
+ // For now, use the real FS, which is known to be threadsafe (if we don't
+ // use/change working directory, which ExpandResponseFilesDatabase doesn't).
+ // NOTE: response files have to be expanded before inference because
+ // inference needs full command line to check/fix driver mode and file type.
+ auto FS = llvm::vfs::getRealFileSystem();
+ return tooling::inferMissingCompileCommands(
+ expandResponseFiles(std::move(CDB), std::move(FS)));
}
return nullptr;
}
@@ -744,6 +755,22 @@ OverlayCDB::getCompileCommand(PathRef File) const {
if (It != Commands.end())
Cmd = It->second;
}
+ if (Cmd) {
+ // FS used for expanding response files.
+ // FIXME: ExpandResponseFiles appears not to provide the usual
+ // thread-safety guarantees, as the access to FS is not locked!
+ // For now, use the real FS, which is known to be threadsafe (if we don't
+ // use/change working directory, which ExpandResponseFiles doesn't).
+ auto FS = llvm::vfs::getRealFileSystem();
+ auto Tokenizer = llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()
+ ? llvm::cl::TokenizeWindowsCommandLine
+ : llvm::cl::TokenizeGNUCommandLine;
+ // Compile command pushed via LSP protocol may have response files that need
+ // to be expanded before further processing. For CDB for files it happens in
+ // the main CDB when reading it from the JSON file.
+ tooling::addExpandedResponseFiles(Cmd->CommandLine, Cmd->Directory,
+ Tokenizer, *FS);
+ }
if (!Cmd)
Cmd = DelegatingCDB::getCompileCommand(File);
if (!Cmd)
diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index 28f0d85d332caa..772177b60b5eed 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -209,21 +209,6 @@ TEST(CommandMangler, ConfigEdits) {
ElementsAre(_, "--driver-mode=g++", "--hello", "--", "FOO.CC"));
}
-TEST(CommandMangler, ExpandedResponseFiles) {
- SmallString<1024> Path;
- int FD;
- ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("args", "", FD, Path));
- llvm::raw_fd_ostream OutStream(FD, true);
- OutStream << "-Wall";
- OutStream.close();
-
- auto Mangler = CommandMangler::forTests();
- tooling::CompileCommand Cmd;
- Cmd.CommandLine = {"clang", ("@" + Path).str(), "foo.cc"};
- Mangler(Cmd, "foo.cc");
- EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "-Wall", "--", "foo.cc"));
-}
-
static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
llvm::SmallVector<llvm::StringRef> Parts;
llvm::SplitString(Argv, Parts);
diff --git a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
index 2a6ae9c325b736..a2ffdefe1bbcb2 100644
--- a/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ b/clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -163,6 +163,21 @@ TEST_F(OverlayCDBTest, Adjustments) {
"-DFallback", "-DAdjust_baz.cc"));
}
+TEST_F(OverlayCDBTest, ExpandedResponseFiles) {
+ SmallString<1024> Path;
+ int FD;
+ ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("args", "", FD, Path));
+ llvm::raw_fd_ostream OutStream(FD, true);
+ OutStream << "-Wall";
+ OutStream.close();
+
+ OverlayCDB CDB(Base.get(), {"-DFallback"});
+ auto Override = cmd(testPath("foo.cc"), ("@" + Path).str());
+ CDB.setCompileCommand(testPath("foo.cc"), Override);
+ EXPECT_THAT(CDB.getCompileCommand(testPath("foo.cc"))->CommandLine,
+ Contains("-Wall"));
+}
+
TEST(GlobalCompilationDatabaseTest, DiscoveryWithNestedCDBs) {
const char *const CDBOuter =
R"cdb(
@@ -421,6 +436,45 @@ TEST_F(OverlayCDBTest, GetProjectInfo) {
EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
}
+
+TEST(GlobalCompilationDatabaseTest, InferenceWithResponseFile) {
+ MockFS FS;
+ auto Command = [&](llvm::StringRef Relative) {
+ DirectoryBasedGlobalCompilationDatabase::Options Opts(FS);
+ return DirectoryBasedGlobalCompilationDatabase(Opts)
+ .getCompileCommand(testPath(Relative))
+ .value_or(tooling::CompileCommand())
+ .CommandLine;
+ };
+ EXPECT_THAT(Command("foo.cc"), IsEmpty());
+
+ // Have to use real FS for response file.
+ SmallString<1024> Path;
+ int FD;
+ ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("args", "", FD, Path));
+ llvm::raw_fd_ostream OutStream(FD, true);
+ OutStream << "-DXYZZY";
+ OutStream.close();
+
+ const char *const CDB =
+ R"cdb(
+ [
+ {
+ "file": "{0}/foo.cc",
+ "command": "clang @{1} {0}/foo.cc",
+ "directory": "{0}",
+ }
+ ]
+ )cdb";
+ FS.Files[testPath("compile_commands.json")] =
+ llvm::formatv(CDB, llvm::sys::path::convert_to_slash(testRoot()),
+ llvm::sys::path::convert_to_slash(Path));
+
+ // File from CDB.
+ EXPECT_THAT(Command("foo.cc"), Contains("-DXYZZY"));
+ // File not in CDB, use inference.
+ EXPECT_THAT(Command("foo.h"), Contains("-DXYZZY"));
+}
} // namespace
// Friend test has access to internals.
More information about the cfe-commits
mailing list