[clang] d60d345 - [clangd] Move standard options adaptor to CommandMangler
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 17 03:26:53 PDT 2023
Author: Dmitry Polukhin
Date: 2023-03-17T03:10:36-07:00
New Revision: d60d3455eb2b375d026a4aa74c4ba0c38f5d323c
URL: https://github.com/llvm/llvm-project/commit/d60d3455eb2b375d026a4aa74c4ba0c38f5d323c
DIFF: https://github.com/llvm/llvm-project/commit/d60d3455eb2b375d026a4aa74c4ba0c38f5d323c.diff
LOG: [clangd] Move standard options adaptor to CommandMangler
There is a discrepancy between how clangd processes CDB loaded from
JSON file on disk and pushed via LSP. Thus the same CDB pushed via
LSP protocol may not work as expected. Some difference between these two
paths is expected but we still need to insert driver mode and target from
binary name and expand response files.
Test Plan: check-clang-tools
Differential Revision: https://reviews.llvm.org/D143436
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/CMakeLists.txt
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
clang/include/clang/Testing/CommandLineArgs.h
clang/include/clang/Tooling/Tooling.h
clang/lib/Testing/CommandLineArgs.cpp
clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
clang/lib/Tooling/Tooling.cpp
clang/unittests/Tooling/ToolingTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index bcd39b2d38ba5..39b180e002a68 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -14,6 +14,7 @@
#include "clang/Driver/Options.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
@@ -27,6 +28,7 @@
#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>
@@ -185,6 +187,12 @@ 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();
@@ -201,9 +209,18 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
trace::Span S("AdjustCompileFlags");
// Most of the modifications below assumes the Cmd starts with a driver name.
// We might consider injecting a generic driver name like "cc" or "c++", but
- // a Cmd missing the driver is probably rare enough in practice and errnous.
+ // a Cmd missing the driver is probably rare enough in practice and erroneous.
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;
@@ -212,7 +229,7 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
OriginalArgs.push_back(S.c_str());
bool IsCLMode = driver::IsClangCL(driver::getDriverMode(
OriginalArgs[0], llvm::ArrayRef(OriginalArgs).slice(1)));
- // ParseArgs propagates missig arg/opt counts on error, but preserves
+ // ParseArgs propagates missing arg/opt counts on error, but preserves
// everything it could parse in ArgList. So we just ignore those counts.
unsigned IgnoredCount;
// Drop the executable name, as ParseArgs doesn't expect it. This means
@@ -307,12 +324,16 @@ void CommandMangler::operator()(tooling::CompileCommand &Command,
// 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 addTargetAndModeForProgramName(), because gcc doesn't support
+ // the target flag that might be added.
// - 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);
}
+ tooling::addTargetAndModeForProgramName(Cmd, Cmd.front());
+
// 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 eb318d18baf63..1b37f44f0b9db 100644
--- a/clang-tools-extra/clangd/CompileCommands.h
+++ b/clang-tools-extra/clangd/CompileCommands.h
@@ -12,6 +12,7 @@
#include "support/Threading.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
#include <deque>
#include <optional>
#include <string>
@@ -50,10 +51,11 @@ struct CommandMangler {
llvm::StringRef TargetFile) const;
private:
- CommandMangler() = default;
+ 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 def43683e0ab7..d1833759917a3 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -244,15 +244,7 @@ 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)) {
- // 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).
- auto FS = llvm::vfs::getRealFileSystem();
- return tooling::inferTargetAndDriverMode(
- tooling::inferMissingCompileCommands(
- expandResponseFiles(std::move(CDB), std::move(FS))));
+ return tooling::inferMissingCompileCommands(std::move(CDB));
}
return nullptr;
}
diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt
index 39fd6ee85378d..056e5803d711b 100644
--- a/clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -157,6 +157,7 @@ clang_target_link_libraries(ClangdTests
clangLex
clangSema
clangSerialization
+ clangTesting
clangTooling
clangToolingCore
clangToolingInclusions
diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index be44ce59f6657..28f0d85d332ca 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -11,6 +11,7 @@
#include "TestFS.h"
#include "support/Context.h"
+#include "clang/Testing/CommandLineArgs.h"
#include "clang/Tooling/ArgumentsAdjusters.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
@@ -20,6 +21,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Process.h"
+#include "llvm/Support/TargetSelect.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -41,15 +43,18 @@ using ::testing::Not;
// Make use of all features and assert the exact command we get out.
// Other tests just verify presence/absence of certain args.
TEST(CommandMangler, Everything) {
+ llvm::InitializeAllTargetInfos(); // As in ClangdMain
+ std::string Target = getAnyTargetForTesting();
auto Mangler = CommandMangler::forTests();
Mangler.ClangPath = testPath("fake/clang");
Mangler.ResourceDir = testPath("fake/resources");
Mangler.Sysroot = testPath("fake/sysroot");
tooling::CompileCommand Cmd;
- Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
+ Cmd.CommandLine = {Target + "-clang++", "--", "foo.cc", "bar.cc"};
Mangler(Cmd, "foo.cc");
EXPECT_THAT(Cmd.CommandLine,
- ElementsAre(testPath("fake/clang++"),
+ ElementsAre(testPath("fake/" + Target + "-clang++"),
+ "--target=" + Target, "--driver-mode=g++",
"-resource-dir=" + testPath("fake/resources"),
"-isysroot", testPath("fake/sysroot"), "--",
"foo.cc"));
@@ -197,8 +202,26 @@ TEST(CommandMangler, ConfigEdits) {
Mangler(Cmd, "foo.cc");
}
// Edits are applied in given order and before other mangling and they always
- // go before filename.
- EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
+ // go before filename. `--driver-mode=g++` here is in lower case because
+ // options inserted by addTargetAndModeForProgramName are not editable,
+ // see discussion in https://reviews.llvm.org/D138546
+ EXPECT_THAT(Cmd.CommandLine,
+ 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) {
diff --git a/clang/include/clang/Testing/CommandLineArgs.h b/clang/include/clang/Testing/CommandLineArgs.h
index e668781ee2ce1..4dd28718dfa67 100644
--- a/clang/include/clang/Testing/CommandLineArgs.h
+++ b/clang/include/clang/Testing/CommandLineArgs.h
@@ -38,6 +38,11 @@ std::vector<std::string> getCC1ArgsForTesting(TestLanguage Lang);
StringRef getFilenameForTesting(TestLanguage Lang);
+/// Find a target name such that looking for it in TargetRegistry by that name
+/// returns the same target. We expect that there is at least one target
+/// configured with this property.
+std::string getAnyTargetForTesting();
+
} // end namespace clang
#endif
diff --git a/clang/include/clang/Tooling/Tooling.h b/clang/include/clang/Tooling/Tooling.h
index 52a81cd9e7787..7a1c62e3a3d57 100644
--- a/clang/include/clang/Tooling/Tooling.h
+++ b/clang/include/clang/Tooling/Tooling.h
@@ -506,6 +506,12 @@ llvm::Expected<std::string> getAbsolutePath(llvm::vfs::FileSystem &FS,
void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
StringRef InvokedAs);
+/// Helper function that expands response files in command line.
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+ llvm::StringRef WorkingDir,
+ llvm::cl::TokenizerCallback Tokenizer,
+ llvm::vfs::FileSystem &FS);
+
/// Creates a \c CompilerInvocation.
CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
ArrayRef<const char *> CC1Args,
diff --git a/clang/lib/Testing/CommandLineArgs.cpp b/clang/lib/Testing/CommandLineArgs.cpp
index bf517e2dd312e..0da087c33e3f4 100644
--- a/clang/lib/Testing/CommandLineArgs.cpp
+++ b/clang/lib/Testing/CommandLineArgs.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "clang/Testing/CommandLineArgs.h"
+#include "llvm/MC/TargetRegistry.h"
#include "llvm/Support/ErrorHandling.h"
namespace clang {
@@ -109,4 +110,18 @@ StringRef getFilenameForTesting(TestLanguage Lang) {
llvm_unreachable("Unhandled TestLanguage enum");
}
+std::string getAnyTargetForTesting() {
+ for (const auto &Target : llvm::TargetRegistry::targets()) {
+ std::string Error;
+ StringRef TargetName(Target.getName());
+ if (TargetName == "x86-64")
+ TargetName = "x86_64";
+ if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
+ &Target) {
+ return std::string(TargetName);
+ }
+ }
+ return "";
+}
+
} // end namespace clang
diff --git a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
index dd9b9c992d846..ebf8aa2a7628c 100644
--- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Tooling.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/ConvertUTF.h"
@@ -48,28 +49,9 @@ class ExpandResponseFilesDatabase : public CompilationDatabase {
private:
std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
- for (auto &Cmd : Cmds) {
- bool SeenRSPFile = false;
- llvm::SmallVector<const char *, 20> Argv;
- Argv.reserve(Cmd.CommandLine.size());
- for (auto &Arg : Cmd.CommandLine) {
- Argv.push_back(Arg.c_str());
- if (!Arg.empty())
- SeenRSPFile |= Arg.front() == '@';
- }
- if (!SeenRSPFile)
- continue;
- llvm::BumpPtrAllocator Alloc;
- llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
- llvm::Error Err = ECtx.setVFS(FS.get())
- .setCurrentDir(Cmd.Directory)
- .expandResponseFiles(Argv);
- if (Err)
- llvm::errs() << Err;
- // Don't assign directly, Argv aliases CommandLine.
- std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
- Cmd.CommandLine = std::move(ExpandedArgv);
- }
+ for (auto &Cmd : Cmds)
+ tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
+ Tokenizer, *FS);
return Cmds;
}
diff --git a/clang/lib/Tooling/Tooling.cpp b/clang/lib/Tooling/Tooling.cpp
index 3048f69eef251..dd22cfedd0ffe 100644
--- a/clang/lib/Tooling/Tooling.cpp
+++ b/clang/lib/Tooling/Tooling.cpp
@@ -43,6 +43,7 @@
#include "llvm/Option/OptTable.h"
#include "llvm/Option/Option.h"
#include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
@@ -299,6 +300,31 @@ void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
}
}
+void addExpandedResponseFiles(std::vector<std::string> &CommandLine,
+ llvm::StringRef WorkingDir,
+ llvm::cl::TokenizerCallback Tokenizer,
+ llvm::vfs::FileSystem &FS) {
+ bool SeenRSPFile = false;
+ llvm::SmallVector<const char *, 20> Argv;
+ Argv.reserve(CommandLine.size());
+ for (auto &Arg : CommandLine) {
+ Argv.push_back(Arg.c_str());
+ if (!Arg.empty())
+ SeenRSPFile |= Arg.front() == '@';
+ }
+ if (!SeenRSPFile)
+ return;
+ llvm::BumpPtrAllocator Alloc;
+ llvm::cl::ExpansionContext ECtx(Alloc, Tokenizer);
+ llvm::Error Err =
+ ECtx.setVFS(&FS).setCurrentDir(WorkingDir).expandResponseFiles(Argv);
+ if (Err)
+ llvm::errs() << Err;
+ // Don't assign directly, Argv aliases CommandLine.
+ std::vector<std::string> ExpandedArgv(Argv.begin(), Argv.end());
+ CommandLine = std::move(ExpandedArgv);
+}
+
} // namespace tooling
} // namespace clang
@@ -684,7 +710,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
if (!Invocation.run())
return nullptr;
-
+
assert(ASTs.size() == 1);
return std::move(ASTs[0]);
}
diff --git a/clang/unittests/Tooling/ToolingTest.cpp b/clang/unittests/Tooling/ToolingTest.cpp
index 93dbd03719b8b..ebe03fda78f1e 100644
--- a/clang/unittests/Tooling/ToolingTest.cpp
+++ b/clang/unittests/Tooling/ToolingTest.cpp
@@ -17,11 +17,11 @@
#include "clang/Frontend/FrontendAction.h"
#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/TextDiagnosticBuffer.h"
+#include "clang/Testing/CommandLineArgs.h"
#include "clang/Tooling/ArgumentsAdjusters.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
-#include "llvm/MC/TargetRegistry.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/TargetSelect.h"
#include "llvm/TargetParser/Host.h"
@@ -871,28 +871,10 @@ TEST(ClangToolTest, StripPluginsAdjuster) {
EXPECT_FALSE(HasFlag("-random-plugin"));
}
-namespace {
-/// Find a target name such that looking for it in TargetRegistry by that name
-/// returns the same target. We expect that there is at least one target
-/// configured with this property.
-std::string getAnyTarget() {
+TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
llvm::InitializeAllTargets();
- for (const auto &Target : llvm::TargetRegistry::targets()) {
- std::string Error;
- StringRef TargetName(Target.getName());
- if (TargetName == "x86-64")
- TargetName = "x86_64";
- if (llvm::TargetRegistry::lookupTarget(std::string(TargetName), Error) ==
- &Target) {
- return std::string(TargetName);
- }
- }
- return "";
-}
-}
-TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
- std::string Target = getAnyTarget();
+ std::string Target = getAnyTargetForTesting();
ASSERT_FALSE(Target.empty());
std::vector<std::string> Args = {"clang", "-foo"};
@@ -905,7 +887,8 @@ TEST(addTargetAndModeForProgramName, AddsTargetAndMode) {
}
TEST(addTargetAndModeForProgramName, PathIgnored) {
- std::string Target = getAnyTarget();
+ llvm::InitializeAllTargets();
+ std::string Target = getAnyTargetForTesting();
ASSERT_FALSE(Target.empty());
SmallString<32> ToolPath;
@@ -919,7 +902,8 @@ TEST(addTargetAndModeForProgramName, PathIgnored) {
}
TEST(addTargetAndModeForProgramName, IgnoresExistingTarget) {
- std::string Target = getAnyTarget();
+ llvm::InitializeAllTargets();
+ std::string Target = getAnyTargetForTesting();
ASSERT_FALSE(Target.empty());
std::vector<std::string> Args = {"clang", "-foo", "-target", "something"};
@@ -936,7 +920,8 @@ TEST(addTargetAndModeForProgramName, IgnoresExistingTarget) {
}
TEST(addTargetAndModeForProgramName, IgnoresExistingMode) {
- std::string Target = getAnyTarget();
+ llvm::InitializeAllTargets();
+ std::string Target = getAnyTargetForTesting();
ASSERT_FALSE(Target.empty());
std::vector<std::string> Args = {"clang", "-foo", "--driver-mode=abc"};
More information about the cfe-commits
mailing list