cpsauer updated this revision to Diff 489208.
cpsauer added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
@nridge, I took a shot at the change you suggested. Confirming that this what
you were thinking of, removing `inferTargetAndDriverMode` from database load
and replacing it with a call to the underlying `addTargetAndModeForProgramName`
in the `CommandMangler` after the `SystemIncludeExtractor` is invoked?
Note that I also saw a parallel call to `inferTargetAndDriverMode` in
`JSONCompilationDatabasePlugin`, which seemed like it might cause the same
problem, so I eliminated the call in there, too. I then culled the dead code,
since there weren't other call sites for the `addTargetAndModeForProgramName`
wrappings. Could I ask you to sanity check that? I know that's a more general
tooling library; I was having trouble determining whether other tooling needed
the target inference or not.
Also, @ArcsinX, could I get your take? Do you think this would fix the layering
issue you raised?
We're definitely stretching my (currently quite limited) understanding of how
clangd's implementation fits together, so I think I'll need to ask for
help/guidance if that looks wrong.
Thanks again, guys!
-CS
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138546/new/
https://reviews.llvm.org/D138546
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/SystemIncludeExtractor.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
clang/docs/tools/clang-formatted-files.txt
clang/include/clang/Tooling/CompilationDatabase.h
clang/lib/Tooling/CMakeLists.txt
clang/lib/Tooling/GuessTargetAndModeCompilationDatabase.cpp
clang/lib/Tooling/JSONCompilationDatabase.cpp
clang/unittests/Tooling/CompilationDatabaseTest.cpp
llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn
Index: llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn
===================================================================
--- llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Tooling/BUILD.gn
@@ -37,7 +37,6 @@
"ExpandResponseFilesCompilationDatabase.cpp",
"FileMatchTrie.cpp",
"FixIt.cpp",
- "GuessTargetAndModeCompilationDatabase.cpp",
"InterpolatingCompilationDatabase.cpp",
"JSONCompilationDatabase.cpp",
"NodeIntrospection.cpp",
Index: clang/unittests/Tooling/CompilationDatabaseTest.cpp
===================================================================
--- clang/unittests/Tooling/CompilationDatabaseTest.cpp
+++ clang/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -929,31 +929,6 @@
EXPECT_TRUE(CCRef != CCTest);
}
-class TargetAndModeTest : public MemDBTest {
-public:
- TargetAndModeTest() { llvm::InitializeAllTargetInfos(); }
-
-protected:
- // Look up the command from a relative path, and return it in string form.
- std::string getCommand(llvm::StringRef F) {
- auto Results = inferTargetAndDriverMode(std::make_unique<MemCDB>(Entries))
- ->getCompileCommands(path(F));
- if (Results.empty())
- return "none";
- return llvm::join(Results[0].CommandLine, " ");
- }
-};
-
-TEST_F(TargetAndModeTest, TargetAndMode) {
- add("foo.cpp", "clang-cl", "");
- add("bar.cpp", "clang++", "");
-
- EXPECT_EQ(getCommand("foo.cpp"),
- "clang-cl --driver-mode=cl foo.cpp -D foo.cpp");
- EXPECT_EQ(getCommand("bar.cpp"),
- "clang++ --driver-mode=g++ bar.cpp -D bar.cpp");
-}
-
class ExpandResponseFilesTest : public MemDBTest {
public:
ExpandResponseFilesTest() : FS(new llvm::vfs::InMemoryFileSystem) {}
Index: clang/lib/Tooling/JSONCompilationDatabase.cpp
===================================================================
--- clang/lib/Tooling/JSONCompilationDatabase.cpp
+++ clang/lib/Tooling/JSONCompilationDatabase.cpp
@@ -165,9 +165,8 @@
llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
auto Base = JSONCompilationDatabase::loadFromFile(
JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
- return Base ? inferTargetAndDriverMode(
- inferMissingCompileCommands(expandResponseFiles(
- std::move(Base), llvm::vfs::getRealFileSystem())))
+ return Base ? inferMissingCompileCommands(expandResponseFiles(
+ std::move(Base), llvm::vfs::getRealFileSystem()))
: nullptr;
}
};
Index: clang/lib/Tooling/GuessTargetAndModeCompilationDatabase.cpp
===================================================================
--- clang/lib/Tooling/GuessTargetAndModeCompilationDatabase.cpp
+++ /dev/null
@@ -1,57 +0,0 @@
-//===- GuessTargetAndModeCompilationDatabase.cpp --------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang/Tooling/CompilationDatabase.h"
-#include "clang/Tooling/Tooling.h"
-#include <memory>
-
-namespace clang {
-namespace tooling {
-
-namespace {
-class TargetAndModeAdderDatabase : public CompilationDatabase {
-public:
- TargetAndModeAdderDatabase(std::unique_ptr<CompilationDatabase> Base)
- : Base(std::move(Base)) {
- assert(this->Base != nullptr);
- }
-
- std::vector<std::string> getAllFiles() const override {
- return Base->getAllFiles();
- }
-
- std::vector<CompileCommand> getAllCompileCommands() const override {
- return addTargetAndMode(Base->getAllCompileCommands());
- }
-
- std::vector<CompileCommand>
- getCompileCommands(StringRef FilePath) const override {
- return addTargetAndMode(Base->getCompileCommands(FilePath));
- }
-
-private:
- std::vector<CompileCommand>
- addTargetAndMode(std::vector<CompileCommand> Cmds) const {
- for (auto &Cmd : Cmds) {
- if (Cmd.CommandLine.empty())
- continue;
- addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine.front());
- }
- return Cmds;
- }
- std::unique_ptr<CompilationDatabase> Base;
-};
-} // namespace
-
-std::unique_ptr<CompilationDatabase>
-inferTargetAndDriverMode(std::unique_ptr<CompilationDatabase> Base) {
- return std::make_unique<TargetAndModeAdderDatabase>(std::move(Base));
-}
-
-} // namespace tooling
-} // namespace clang
Index: clang/lib/Tooling/CMakeLists.txt
===================================================================
--- clang/lib/Tooling/CMakeLists.txt
+++ clang/lib/Tooling/CMakeLists.txt
@@ -107,7 +107,6 @@
ExpandResponseFilesCompilationDatabase.cpp
FileMatchTrie.cpp
FixIt.cpp
- GuessTargetAndModeCompilationDatabase.cpp
InterpolatingCompilationDatabase.cpp
JSONCompilationDatabase.cpp
Refactoring.cpp
Index: clang/include/clang/Tooling/CompilationDatabase.h
===================================================================
--- clang/include/clang/Tooling/CompilationDatabase.h
+++ clang/include/clang/Tooling/CompilationDatabase.h
@@ -228,12 +228,6 @@
std::unique_ptr<CompilationDatabase>
inferMissingCompileCommands(std::unique_ptr<CompilationDatabase>);
-/// Returns a wrapped CompilationDatabase that will add -target and -mode flags
-/// to commandline when they can be deduced from argv[0] of commandline returned
-/// by underlying database.
-std::unique_ptr<CompilationDatabase>
-inferTargetAndDriverMode(std::unique_ptr<CompilationDatabase> Base);
-
/// Returns a wrapped CompilationDatabase that will expand all rsp(response)
/// files on commandline returned by underlying database.
std::unique_ptr<CompilationDatabase>
Index: clang/docs/tools/clang-formatted-files.txt
===================================================================
--- clang/docs/tools/clang-formatted-files.txt
+++ clang/docs/tools/clang-formatted-files.txt
@@ -563,7 +563,6 @@
clang/lib/Tooling/Execution.cpp
clang/lib/Tooling/ExpandResponseFilesCompilationDatabase.cpp
clang/lib/Tooling/FixIt.cpp
-clang/lib/Tooling/GuessTargetAndModeCompilationDatabase.cpp
clang/lib/Tooling/NodeIntrospection.cpp
clang/lib/Tooling/StandaloneExecution.cpp
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===================================================================
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -16,6 +16,8 @@
# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/bin/my_driver.sh
# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >> %t.dir/bin/my_driver.sh
# RUN: echo '[ -z "${args##*"--sysroot /my/sysroot/path"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-target arm-linux-gnueabi"*}" ] || exit' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"--target=arm-linux-gnueabi"*}" ] || exit' >> %t.dir/bin/my_driver.sh
# RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >> %t.dir/bin/my_driver.sh
# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >> %t.dir/bin/my_driver.sh
@@ -32,7 +34,7 @@
# Generate a compile_commands.json that will query the mock driver we've
# created. Which should add a.h and b.h into include search path.
-# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot -target arm-linux-gnueabi --target=arm-linux-gnueabi", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
@@ -70,7 +72,7 @@
{"jsonrpc":"2.0","method":"exit"}
# Generate a different compile_commands.json which does not point to the mock driver
-# RUN: echo '[{"directory": "%/t.dir", "command": "gcc the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "gcc the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot -target arm-linux-gnueabi --target=arm-linux-gnueabi", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
# Generate a clangd config file which points to the mock driver instead
# RUN: echo 'CompileFlags:' > %t.dir/.clangd
Index: clang-tools-extra/clangd/SystemIncludeExtractor.cpp
===================================================================
--- clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -184,13 +184,18 @@
const llvm::StringRef FlagsToPreserve[] = {
"-nostdinc", "--no-standard-includes", "-nostdinc++", "-nobuiltininc"};
// Preserves these flags and their values, either as separate args or with an
- // equalsbetween them
+ // equals between them
const llvm::StringRef ArgsToPreserve[] = {"--sysroot", "-isysroot"};
for (size_t I = 0, E = CommandLine.size(); I < E; ++I) {
llvm::StringRef Arg = CommandLine[I];
if (llvm::is_contained(FlagsToPreserve, Arg)) {
Args.push_back(Arg);
+ } else if (Arg.startswith("--target=")) {
+ Args.push_back(Arg);
+ } else if (I + 1 < E && Arg.equals("-target")) {
+ Args.push_back(CommandLine[I]);
+ Args.push_back(CommandLine[++I]);
} else {
const auto *Found =
llvm::find_if(ArgsToPreserve, [&Arg](llvm::StringRef S) {
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -251,9 +251,8 @@
// 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(
+ expandResponseFiles(std::move(CDB), std::move(FS)));
}
return nullptr;
}
Index: clang-tools-extra/clangd/CompileCommands.cpp
===================================================================
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ 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"
@@ -200,7 +201,7 @@
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 a driver is probably rare enough in practice and erroneous.
if (Cmd.empty())
return;
auto &OptTable = clang::driver::getDriverOptTable();
@@ -306,12 +307,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);
}
+ addTargetAndModeForProgramName(Cmd, Cmd.front());
+
// Check whether the flag exists, either as -flag or -flag=*
auto Has = [&](llvm::StringRef Flag) {
for (llvm::StringRef Arg : Cmd) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
