[clang-tools-extra] 2a84c53 - Revert "[clangd] Move standard options adaptor to CommandMangler"
Dmitry Polukhin via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 13 07:03:01 PDT 2023
Author: Dmitry Polukhin
Date: 2023-03-13T07:00:56-07:00
New Revision: 2a84c53ccdc015a7f53a144aa4f7c0dddf839604
URL: https://github.com/llvm/llvm-project/commit/2a84c53ccdc015a7f53a144aa4f7c0dddf839604
DIFF: https://github.com/llvm/llvm-project/commit/2a84c53ccdc015a7f53a144aa4f7c0dddf839604.diff
LOG: Revert "[clangd] Move standard options adaptor to CommandMangler"
This reverts commit 34de7da6246cdfa6ff6f3d3c514583cddc0a10ec.
Added:
Modified:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/CompileCommands.h
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/test/did-change-configuration-params.test
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
clang/include/clang/Tooling/Tooling.h
clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
clang/lib/Tooling/Tooling.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp
index 39b180e002a68..bcd39b2d38ba5 100644
--- a/clang-tools-extra/clangd/CompileCommands.cpp
+++ b/clang-tools-extra/clangd/CompileCommands.cpp
@@ -14,7 +14,6 @@
#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"
@@ -28,7 +27,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 +185,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();
@@ -209,18 +201,9 @@ 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 erroneous.
+ // a Cmd missing the driver is probably rare enough in practice and errnous.
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;
@@ -229,7 +212,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 missing arg/opt counts on error, but preserves
+ // ParseArgs propagates missig 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
@@ -324,16 +307,12 @@ 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 1b37f44f0b9db..eb318d18baf63 100644
--- a/clang-tools-extra/clangd/CompileCommands.h
+++ b/clang-tools-extra/clangd/CompileCommands.h
@@ -12,7 +12,6 @@
#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>
@@ -51,11 +50,10 @@ struct CommandMangler {
llvm::StringRef TargetFile) const;
private:
- CommandMangler();
+ CommandMangler() = default;
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 d1833759917a3..def43683e0ab7 100644
--- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -244,7 +244,15 @@ 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).
+ auto FS = llvm::vfs::getRealFileSystem();
+ return tooling::inferTargetAndDriverMode(
+ tooling::inferMissingCompileCommands(
+ expandResponseFiles(std::move(CDB), std::move(FS))));
}
return nullptr;
}
diff --git a/clang-tools-extra/clangd/test/did-change-configuration-params.test b/clang-tools-extra/clangd/test/did-change-configuration-params.test
index edda1e1f16787..08c7b4bcb57ad 100644
--- a/clang-tools-extra/clangd/test/did-change-configuration-params.test
+++ b/clang-tools-extra/clangd/test/did-change-configuration-params.test
@@ -45,17 +45,13 @@
# CHECK-NEXT: "uri": "file://{{.*}}/foo.c",
# CHECK-NEXT: "version": 0
# CHECK-NEXT: }
----
-{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c": {"workingDirectory":"/clangd-test2", "compilationCommand": ["x86_64-linux-unknown-gcc", "-c", "foo.c", "-Wall", "-Werror"]}}}}}
-# CHECK: "method": "textDocument/publishDiagnostics",
#
# ERR: ASTWorker building file {{.*}}foo.c version 0 with command
# ERR: [{{.*}}clangd-test2]
# ERR: clang -c -Wall -Werror {{.*}} -- {{.*}}foo.c
-# ERR: ASTWorker building file {{.*}}foo.c version 0 with command
-# ERR: [{{.*}}clangd-test2]
-# ERR: x86_64-linux-unknown-gcc --target=x86_64-linux-unknown -c -Wall -Werror {{.*}} -- {{.*}}foo.c
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
{"jsonrpc":"2.0","method":"exit"}
+
+
diff --git a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
index acb1044287a59..be44ce59f6657 100644
--- a/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -20,7 +20,6 @@
#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"
@@ -42,17 +41,15 @@ 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
auto Mangler = CommandMangler::forTests();
Mangler.ClangPath = testPath("fake/clang");
Mangler.ResourceDir = testPath("fake/resources");
Mangler.Sysroot = testPath("fake/sysroot");
tooling::CompileCommand Cmd;
- Cmd.CommandLine = {"x86_64-linux-unknown-clang++", "--", "foo.cc", "bar.cc"};
+ Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
Mangler(Cmd, "foo.cc");
EXPECT_THAT(Cmd.CommandLine,
- ElementsAre(testPath("fake/x86_64-linux-unknown-clang++"),
- "--target=x86_64-linux-unknown", "--driver-mode=g++",
+ ElementsAre(testPath("fake/clang++"),
"-resource-dir=" + testPath("fake/resources"),
"-isysroot", testPath("fake/sysroot"), "--",
"foo.cc"));
@@ -200,26 +197,8 @@ TEST(CommandMangler, ConfigEdits) {
Mangler(Cmd, "foo.cc");
}
// Edits are applied in given order and before other mangling and they always
- // 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"));
+ // go before filename.
+ EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
}
static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
diff --git a/clang/include/clang/Tooling/Tooling.h b/clang/include/clang/Tooling/Tooling.h
index 7a1c62e3a3d57..52a81cd9e7787 100644
--- a/clang/include/clang/Tooling/Tooling.h
+++ b/clang/include/clang/Tooling/Tooling.h
@@ -506,12 +506,6 @@ 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/Tooling/ExpandResponseFilesCompilationDatabase.cpp b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
index ebf8aa2a7628c..dd9b9c992d846 100644
--- a/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
+++ b/clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#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"
@@ -49,9 +48,28 @@ class ExpandResponseFilesDatabase : public CompilationDatabase {
private:
std::vector<CompileCommand> expand(std::vector<CompileCommand> Cmds) const {
- for (auto &Cmd : Cmds)
- tooling::addExpandedResponseFiles(Cmd.CommandLine, Cmd.Directory,
- Tokenizer, *FS);
+ 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);
+ }
return Cmds;
}
diff --git a/clang/lib/Tooling/Tooling.cpp b/clang/lib/Tooling/Tooling.cpp
index dd22cfedd0ffe..3048f69eef251 100644
--- a/clang/lib/Tooling/Tooling.cpp
+++ b/clang/lib/Tooling/Tooling.cpp
@@ -43,7 +43,6 @@
#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"
@@ -300,31 +299,6 @@ 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
@@ -710,7 +684,7 @@ std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
if (!Invocation.run())
return nullptr;
-
+
assert(ASTs.size() == 1);
return std::move(ASTs[0]);
}
More information about the cfe-commits
mailing list