diff --git a/clang-tools-extra/clangd/CompileCommands.h b/clang-tools-extra/clangd/CompileCommands.h --- 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 #include #include @@ -50,10 +51,11 @@ llvm::StringRef TargetFile) const; private: - CommandMangler() = default; + CommandMangler(); Memoize> ResolvedDrivers; Memoize> ResolvedDriversNoFollow; + llvm::cl::TokenizerCallback Tokenizer; }; // Removes args from a command-line in a semantically-aware way. diff --git a/clang-tools-extra/clangd/CompileCommands.cpp b/clang-tools-extra/clangd/CompileCommands.cpp --- 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 #include #include @@ -185,6 +187,12 @@ } // 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 @@ 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 OriginalArgs; @@ -212,7 +229,7 @@ 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 @@ // 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/GlobalCompilationDatabase.cpp b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp --- a/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp +++ b/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp @@ -244,15 +244,7 @@ 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 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -157,6 +157,7 @@ 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 --- 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 @@ // 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 @@ 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 --- a/clang/include/clang/Testing/CommandLineArgs.h +++ b/clang/include/clang/Testing/CommandLineArgs.h @@ -38,6 +38,11 @@ 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 --- a/clang/include/clang/Tooling/Tooling.h +++ b/clang/include/clang/Tooling/Tooling.h @@ -506,6 +506,12 @@ void addTargetAndModeForProgramName(std::vector &CommandLine, StringRef InvokedAs); +/// Helper function that expands response files in command line. +void addExpandedResponseFiles(std::vector &CommandLine, + llvm::StringRef WorkingDir, + llvm::cl::TokenizerCallback Tokenizer, + llvm::vfs::FileSystem &FS); + /// Creates a \c CompilerInvocation. CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics, ArrayRef CC1Args, diff --git a/clang/lib/Testing/CommandLineArgs.cpp b/clang/lib/Testing/CommandLineArgs.cpp --- 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 @@ 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 --- 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 @@ private: std::vector expand(std::vector Cmds) const { - for (auto &Cmd : Cmds) { - bool SeenRSPFile = false; - llvm::SmallVector 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 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 --- 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 addExpandedResponseFiles(std::vector &CommandLine, + llvm::StringRef WorkingDir, + llvm::cl::TokenizerCallback Tokenizer, + llvm::vfs::FileSystem &FS) { + bool SeenRSPFile = false; + llvm::SmallVector 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 ExpandedArgv(Argv.begin(), Argv.end()); + CommandLine = std::move(ExpandedArgv); +} + } // namespace tooling } // namespace clang @@ -684,7 +710,7 @@ 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 --- 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 @@ 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 Args = {"clang", "-foo"}; @@ -905,7 +887,8 @@ } 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, IgnoresExistingTarget) { - std::string Target = getAnyTarget(); + llvm::InitializeAllTargets(); + std::string Target = getAnyTargetForTesting(); ASSERT_FALSE(Target.empty()); std::vector Args = {"clang", "-foo", "-target", "something"}; @@ -936,7 +920,8 @@ } TEST(addTargetAndModeForProgramName, IgnoresExistingMode) { - std::string Target = getAnyTarget(); + llvm::InitializeAllTargets(); + std::string Target = getAnyTargetForTesting(); ASSERT_FALSE(Target.empty()); std::vector Args = {"clang", "-foo", "--driver-mode=abc"};