[clang-tools-extra] [clangd] Expand response files before CDB interpolation (PR #75753)
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 18 04:58:22 PST 2023
https://github.com/dmpolukhin updated https://github.com/llvm/llvm-project/pull/75753
>From 957951483dab19b0982a5dbe7d5370bd9061d931 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Sun, 17 Dec 2023 14:11:11 -0800
Subject: [PATCH 1/2] [clangd] Expand response files before CDB interpolation
Summary:
After https://reviews.llvm.org/D143436 response files stopped working
with CDB interpolation. It has happened because interpolation removes
all unknwn 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 implements the simplest approach that add extra response file
expansion before inference. Response file expansion in CommandMangler
keep working for CDB pushed via LSP and will do nothing if all response
files are already expanded.
Test Plan: TBD
Reviewers:
Subscribers:
Tasks: https://github.com/llvm/llvm-project/issues/69690
Tags:
---
.../clangd/GlobalCompilationDatabase.cpp | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
index d1833759917a30..03ac1aa132d0bf 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -244,7 +244,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;
}
>From f4b1a02354fa5085caf50a460363e75e10afc3bb Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin <dmitry.polukhin at gmail.com>
Date: Sun, 17 Dec 2023 14:11:11 -0800
Subject: [PATCH 2/2] [clangd] Expand response files before CDB interpolation
Summary:
After https://reviews.llvm.org/D143436 response files stopped working
with CDB interpolation. It has happened because interpolation removes
all unknwn 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: unittests
Reviewers:
Subscribers:
Tasks: https://github.com/llvm/llvm-project/issues/69690
Tags:
---
clang-tools-extra/clangd/CompileCommands.cpp | 15 ------
clang-tools-extra/clangd/CompileCommands.h | 3 --
.../clangd/GlobalCompilationDatabase.cpp | 24 +++++++--
.../clangd/unittests/CompileCommandsTests.cpp | 15 ------
.../GlobalCompilationDatabaseTests.cpp | 53 +++++++++++++++++++
5 files changed, 74 insertions(+), 36 deletions(-)
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 03ac1aa132d0bf..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>
@@ -249,11 +251,11 @@ parseJSON(PathRef Path, llvm::StringRef Data, std::string &Error) {
// 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.
+ // 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)));
+ expandResponseFiles(std::move(CDB), std::move(FS)));
}
return nullptr;
}
@@ -753,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..72f47862442c4d 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,44 @@ 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()), 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