[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

This can be closed, as the change has been made in 
https://github.com/llvm/llvm-project/pull/65824


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Sorry I was waiting for D139277  to land, but 
it seems to be stuck so sent out D146941 . 
Let's take a look at this again after that one lands


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

@kadircet, could I get your review on this one?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

@kadircet, friendly check in, since I got reminded of this: How would you like 
to proceed from here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Thanks, Nathan. Makes sense; sounds good.

@kadircet, I think this one is ready for you, whenever you want it. Lmk how 
you'd like to proceed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D138546#4075681 , @cpsauer wrote:

> Sounds like you're not concerned, then, that the 
> JSONCompilationDatabasePlugin will get invoked and the results then passed to 
> a SystemIncludeExtractor-enabled mangler?
> (I'd seen some fallback calls into the plugin registry in 
> GlobalCompilationDatabase, hence the original change, but, lacking broader 
> context, I was having trouble tracing the expected control flow.)

I did initially overlook the mentioned fallback codepath 
,
 thanks for catching that. However, it looks like it's only taken if we've 
already explicitly checked for `compile_commands.json` and didn't find one, so 
`JsonCompilationDatabasePlugin` won't find anything either (i.e. the fallback 
is for potential other plugins).

As for consumers of the plugin registry outside of clangd, we can assume they 
won't use `SystemIncludeExtractor` which is currently a clangd-internal 
component. (There has been an idea floated about upstreaming 
`SystemIncludeExtractor` from clangd to libTooling, but it remains to be 
determined how that would look at the API level, and we can make adjustments as 
necessary if/when that happens.)

Anyways, +1 from me on the TargetAndMode related changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 491593.
cpsauer added a comment.

Rebased


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-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -49,7 +49,7 @@
   Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
   Mangler(Cmd, "foo.cc");
   EXPECT_THAT(Cmd.CommandLine,
-  ElementsAre(testPath("fake/clang++"),
+  ElementsAre(testPath("fake/clang++"), "--driver-mode=g++",
   "-resource-dir=" + testPath("fake/resources"),
   "-isysroot", testPath("fake/sysroot"), "--",
   "foo.cc"));
@@ -198,7 +198,8 @@
   }
   // Edits are applied in given order and before other mangling and they always
   // go before filename.
-  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
+  EXPECT_THAT(Cmd.CommandLine,
+  ElementsAre(_, "--driver-mode=g++", "--hello", "--", "FOO.CC"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
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
@@ -185,13 +185,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 {

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 491592.

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-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -49,7 +49,7 @@
   Cmd.CommandLine = {"clang++", "--", "foo.cc", "bar.cc"};
   Mangler(Cmd, "foo.cc");
   EXPECT_THAT(Cmd.CommandLine,
-  ElementsAre(testPath("fake/clang++"),
+  ElementsAre(testPath("fake/clang++"), "--driver-mode=g++",
   "-resource-dir=" + testPath("fake/resources"),
   "-isysroot", testPath("fake/sysroot"), "--",
   "foo.cc"));
@@ -198,7 +198,8 @@
   }
   // Edits are applied in given order and before other mangling and they always
   // go before filename.
-  EXPECT_THAT(Cmd.CommandLine, ElementsAre(_, "--hello", "--", "FOO.CC"));
+  EXPECT_THAT(Cmd.CommandLine,
+  ElementsAre(_, "--driver-mode=g++", "--hello", "--", "FOO.CC"));
 }
 
 static std::string strip(llvm::StringRef Arg, llvm::StringRef Argv) {
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 =
   

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Yay. Thanks, Nathan, for guiding, replying, and just generally being so kind, 
great, and thorough.

I'll back out the plugin part of the changes changes momentarily, then.
Just to confirm: Sounds like you're not concerned, then, that the 
JSONCompilationDatabasePlugin will get invoked and the results then passed to a 
SystemIncludeExtractor-enabled mangler?
(I'd seen some fallback calls into the plugin registry in 
GlobalCompilationDatabase, hence the original change, but, lacking broader 
context, I was having trouble tracing the expected control flow.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D138546#4053538 , @cpsauer wrote:

> @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?

Yup, that's what I had in mind. Thanks for writing that up!

> 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.

Oh, huh, I didn't realize that `JSONCompilationDatabasePlugin` has this 
behaviour baked in.

I would err on the side of leaving that call site as is.

My thinking is as follows:

- The general purpose of the compilation database utilities in libTooling is to 
produce command lines that will be fed to the clang driver library.
- The clang driver does support `--target`, and in some cases having that flag 
present is important.
- For clangd's call site, we're not //removing// the logic to add the 
`--target` flag, we're just moving it from an ealier stage of the pipeline to a 
later stage of the pipeline, because our pipeline contains a specialized step 
(SystemIncludeExtractor) that may involve executing a gcc driver with (parts of 
the) the command.
- But for other tools built on libTooling that use 
`JSONCompilationDatabasePlugin`, this change would be removing the `--target` 
logic altogether, which could regress behaviour.

> I then culled the dead code, since there weren't other call sites for the 
> `addTargetAndModeForProgramName` wrappings.

Reagrdless of what we decide about `JSONCompilationDatabasePlugin`, we probably 
shouldn't remove `clang::tooling::inferTargetAndDriverMode()`, as that's a 
public libTooling API that may be used by external projects that use libTooling 
as a library.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

(If anyone knows what's going on with the (unrelated seeming?) testing timeouts 
please do say.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 489317.

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-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/bindings/python/tests/cindex/test_cdb.py
  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(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 
-
-namespace clang {
-namespace tooling {
-
-namespace {
-class TargetAndModeAdderDatabase : public CompilationDatabase {
-public:
-  TargetAndModeAdderDatabase(std::unique_ptr Base)
-  : Base(std::move(Base)) {
-assert(this->Base != nullptr);
-  }
-
-  std::vector getAllFiles() const override {
-return Base->getAllFiles();
-  }
-
-  std::vector getAllCompileCommands() const override {
-return addTargetAndMode(Base->getAllCompileCommands());
-  }
-
-  std::vector
-  getCompileCommands(StringRef FilePath) const override {
-return addTargetAndMode(Base->getCompileCommands(FilePath));
-  }
-
-private:
-  std::vector
-  addTargetAndMode(std::vector Cmds) const {
-for (auto &Cmd : Cmds) {
-  if (Cmd.CommandLine.empty())
-continue;
-  addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine.front());
-}
-return Cmds;
-  }
-  std

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 489217.

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-tools-extra/clangd/unittests/CompileCommandsTests.cpp
  clang/bindings/python/tests/cindex/test_cdb.py
  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(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 
-
-namespace clang {
-namespace tooling {
-
-namespace {
-class TargetAndModeAdderDatabase : public CompilationDatabase {
-public:
-  TargetAndModeAdderDatabase(std::unique_ptr Base)
-  : Base(std::move(Base)) {
-assert(this->Base != nullptr);
-  }
-
-  std::vector getAllFiles() const override {
-return Base->getAllFiles();
-  }
-
-  std::vector getAllCompileCommands() const override {
-return addTargetAndMode(Base->getAllCompileCommands());
-  }
-
-  std::vector
-  getCompileCommands(StringRef FilePath) const override {
-return addTargetAndMode(Base->getCompileCommands(FilePath));
-  }
-
-private:
-  std::vector
-  addTargetAndMode(std::vector Cmds) const {
-for (auto &Cmd : Cmds) {
-  if (Cmd.CommandLine.empty())
-continue;
-  addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine.front());
-}
-return Cmds;
-  }
-  std

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 489216.

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/bindings/python/tests/cindex/test_cdb.py
  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(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 
-
-namespace clang {
-namespace tooling {
-
-namespace {
-class TargetAndModeAdderDatabase : public CompilationDatabase {
-public:
-  TargetAndModeAdderDatabase(std::unique_ptr Base)
-  : Base(std::move(Base)) {
-assert(this->Base != nullptr);
-  }
-
-  std::vector getAllFiles() const override {
-return Base->getAllFiles();
-  }
-
-  std::vector getAllCompileCommands() const override {
-return addTargetAndMode(Base->getAllCompileCommands());
-  }
-
-  std::vector
-  getCompileCommands(StringRef FilePath) const override {
-return addTargetAndMode(Base->getCompileCommands(FilePath));
-  }
-
-private:
-  std::vector
-  addTargetAndMode(std::vector Cmds) const {
-for (auto &Cmd : Cmds) {
-  if (Cmd.CommandLine.empty())
-continue;
-  addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine.front());
-}
-return Cmds;
-  }
-  std::unique_ptr Base;
-};
-} // namespace
-
-std::unique_ptr
-inf

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 489214.

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(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 
-
-namespace clang {
-namespace tooling {
-
-namespace {
-class TargetAndModeAdderDatabase : public CompilationDatabase {
-public:
-  TargetAndModeAdderDatabase(std::unique_ptr Base)
-  : Base(std::move(Base)) {
-assert(this->Base != nullptr);
-  }
-
-  std::vector getAllFiles() const override {
-return Base->getAllFiles();
-  }
-
-  std::vector getAllCompileCommands() const override {
-return addTargetAndMode(Base->getAllCompileCommands());
-  }
-
-  std::vector
-  getCompileCommands(StringRef FilePath) const override {
-return addTargetAndMode(Base->getCompileCommands(FilePath));
-  }
-
-private:
-  std::vector
-  addTargetAndMode(std::vector Cmds) const {
-for (auto &Cmd : Cmds) {
-  if (Cmd.CommandLine.empty())
-continue;
-  addTargetAndModeForProgramName(Cmd.CommandLine, Cmd.CommandLine.front());
-}
-return Cmds;
-  }
-  std::unique_ptr Base;
-};
-} // namespace
-
-std::unique_ptr
-inferTargetAndDriverMode(std::unique_ptr Base) {
-  

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

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(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: A

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

@nridge, yep, confirming: For Android, --target is being added explicitly as 
part of the command and we'll need to pass through explicit (but not implicit) 
--target flags to the system includes extractor to get the correct paths.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D138546#4017356 , @ArcsinX wrote:

> In other words, with this patch we preserve `--target=...`, but this option 
> can appear from 
> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp#L253

Huh, I had no idea we had logic to add a `--target` flag there. (Nor that gcc 
does not recognize this option.)

@cpsauer just to double-check, in your case the `--target` flag is explicitly 
written in the `compile_commands.json`, and the idea is that in such case we 
want to pass it to the system include extractor (since it's safe to assume the 
compiler supports the flag in that case), but not in the case where we add it?

In D138546#4018119 , @cpsauer wrote:

> probably we should be relayering things to operate on raw commands for query 
> driver?

Not quite **raw** commands: the fix for 
https://github.com/clangd/clangd/issues/1173 was to make sure the inferred 
language (e.g. `-x c++`) is added before query-driver, and the fix for 
https://github.com/clangd/clangd/issues/1089 was to make sure that edits from 
the config file are applied before query-driver (because `Compiler` can be 
overridden there).

But I think we could achieve the intended effect by removing the call to 
`inferTargetAndDriverMode()` from `GlobalCompilationDatabase.cpp`, and instead 
performing the equivalent logic in `CommandMangler`, //after// invoking the 
`SystemIncludeExtractor`?

Any thoughts on whether that would be reasonable / whether it would break 
anything else, are welcome.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a subscriber: nridge.
cpsauer added a comment.

Ooh interesting. Thanks for your careful look, @ArcsinX.

To check my understanding: Sounds like, more generally, commands get massaged 
into clang argument form before they're passed into the query-driver machinery, 
which can break break calls through to other compilers.

So far that hadn't affected things, but probably we should be relayering things 
to operate on raw commands for query driver?
@nridge, I know you'd fixed a bunch of problems by relayering in the past. Any 
thoughts on this one?

More generally folks more experienced in the codebase: Any thoughts on how we 
should do this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

It seems this patch can break GCC toolchains support

This patch expects that we never get target-related options in `CommandLine` 
passed to `extractSystemIncludesAndTarget()` for **GCC** toolchain. But seems 
this is not always true because of 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp#L253

In other words, with this patch we preserve `--target=...`, but this option can 
appear from 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/GlobalCompilationDatabase.cpp#L253
 (but not from `compile_commands.json`). As the result of it, we can call 
**GCC** toolchain with `--target=..` option at trying to query-driver and this 
call will fail (because GCC has no --target option)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Sweet. Thanks again, Nathan.

Guys, lmk how you'd like to go from here. If you approve; feel free to copy 
in/land as part of the other change if that would save time. 
(I don't have commit access anyway.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge resigned from this revision.
nridge added a comment.

In D138546#3980350 , @cpsauer wrote:

> @nridge, I took a shot at amending the test. Thanks for the pointer! Please 
> me know if that looks good to you!

The test changes look good, thanks.

I'm going to leave the approval to Kadir because his comment sounded like he 
would maybe prefer for https://github.com/clangd/clangd/issues/1403 (note, it 
has a patch at D139277 ) to be fixed before 
merging this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

@kadircet, you were asking about what's requiring the use of query-driver--I'll 
reply over in the issue (as you suggest), keeping everything in one place.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

@nridge, I took a shot at amending the test. Thanks for the pointer! Please me 
know if that looks good to you!
The host-target cross-compile situation you hypothesize is exactly what caused 
me to notice this. Super common with Bazel.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer updated this revision to Diff 481141.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

Files:
  clang-tools-extra/clangd/SystemIncludeExtractor.cpp
  clang-tools-extra/clangd/test/system-include-extractor.test


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/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": "%/

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Sorry for being so slow this time myself, guys. Took me a while to dig myself 
out of a deep inbox hole after Thanksgiving (US holiday). Really appreciate you 
all and your friendly responsiveness, especially as I get up to speed here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Sorry for noticing this so late. Yes the initial reason we left `sysroot` and 
`std/builtin-inc` related flags were exactly that, in theory they could vary 
but in practice they were applied to all or none of the project.

Regarding this change itself, surely preserving target makes sense and similar 
to sysroot and others, i'd actually expect it to be the same across all files 
in practice. But they're more likely to change without a clangd restart (e.g. 
because user invokes build in a different configuration). So i think they 
should be part of the cache key nevertheless. My main question about this 
change is, you seem to be using a `clang` driver underneath. Why is clang 
driver we have in clangd not able to infer the same search paths etc? Is the 
clang provided in android ndk a custom clang? can you provide logs in the issue 
you've mentioned above, especially the compile flags retrieved from CDB and cc1 
produced by it?

Regarding the mishandling of `-isysroot` vs `-isysroot ` (they still 
both have a single `-`, but expecting an equals in between is a mistake 
apparently), surely we should fix that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D138546#3946144 , @sammccall wrote:

> In D138546#3946046 , @cpsauer wrote:
>
>> Sam, my read, too, is that the memoizing design isn't safe--also that the 
>> key bug is preexisting. 
>> Nathan and I were thinking, though, that we'd should post this incremental 
>> fix for review rather than getting bogged down in trying to fix multiple 
>> things atomically.
>
> Sure. At first glance the design looks like it's been changed in a way that's 
> broken, but maybe there's some deeper reason that it's safe. That reason may 
> or may not also apply to `-target` (e.g. if `-target` could plausibly differ 
> across a project but other flags couldn't). I wanted to understand 
> whether/why it's broken today before concluding it's safe to break it 
> further. Probably @kadircet is the best person to make a call here.

Given that a "project" is a fuzzy concept -- where e.g. someone in a 
cross-compilation scenario may choose to group host tools and target tools 
together in the same project -- surely `-isysroot`, `-nostdinc` etc. could also 
potentially differ across a project (but it's probably not common).

I think the simple answer here is that not including those flags in the key was 
an oversight, albeit a low-impact one.

I'd say taking this change as-is would be a net improvement (but we should also 
fix the oversight, now that we're aware of it, in a follow-up change).

My only suggestion regarding this patch would be to amend this test 

 to check for the preservation of the target flag as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

I suppose, if it ever might help make the case with said employer (Google, 
right?) the broken, non-stock-clang use case that's motivating this is Android 
(and also usage from Bazel/Blaze).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Makes sense! Thanks for your time, Sam, and for being great, as always.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D138546#3946046 , @cpsauer wrote:

> Sam, my read, too, is that the memoizing design isn't safe--also that the key 
> bug is preexisting. 
> Nathan and I were thinking, though, that we'd should post this incremental 
> fix for review rather than getting bogged down in trying to fix multiple 
> things atomically.

Sure. At first glance the design looks like it's been changed in a way that's 
broken, but maybe there's some deeper reason that it's safe. That reason may or 
may not also apply to `-target` (e.g. if `-target` could plausibly differ 
across a project but other flags couldn't). I wanted to understand whether/why 
it's broken today before concluding it's safe to break it further. Probably 
@kadircet is the best person to make a call here.

> I had the same reaction reading through this file after spotting problems in 
> the log. That's what spawned https://github.com/clangd/clangd/issues/1378.
>
> Any chance I could get you (and others) to quickly read through that issue if 
> you haven't already? (The relevant section to this part: "If we think 
> sysroots, targets, and the other flags enumerated effect the system includes, 
> we'd better include them as part of the memoization key.") )

The discussion in this bug makes sense to me. I agree with the need for 
memoization, and the handling of `-isysroot` indeed looks dodgy and could 
probably be fixed.
Framework support sounds important for Mac, and we'd be happy to take a 
contribution from a Mac person who can explain the issues there.
Reusing clang's arg parser is tricky: it's a large change, it's often more 
complex than naive string-bashing, and it's also significantly slower.

Unfortunately at the moment I'm not really able to spend work time on improving 
these things, beyond reviewing patches.
(The system include extractor is mostly useful for things that don't build with 
stock clang, which at $employer is a minority).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer added a comment.

Sam, my read, too, is that the memoizing design isn't safe--also that the key 
bug is preexisting.

I had the same reaction reading through this file after spotting problems in 
the log. That's what spawned https://github.com/clangd/clangd/issues/1378.

Any chance I could get you to quickly read through that issue if you haven't 
already? (The relevant section to this part: "If we think sysroots, targets, 
and the other flags enumerated effect the system includes, we'd better include 
them as part of the memoization key.")

There are sadly *lots* of problems in this file that leap out on a quick read. 
Nathan and I were thinking, though, that we'd should post this incremental fix 
for review rather than getting bogged down in trying to fix multiple things 
atomically.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

My biggest concern here is propagating more information here without including 
it in the cache key.
Originally the computation was strictly using the cache key as input, but 
started using the command-line flags in D73453 
. @kadircet any extra context on when/why this 
is safe?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Christopher Sauer via Phabricator via cfe-commits]

cpsauer created this revision.
cpsauer added reviewers: nridge, sammccall, kadircet.
Herald added a subscriber: arphaman.
Herald added a project: All.
cpsauer requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

This preserves --target and -target flags in the system includes extractor, 
which is needed for cross-compiling to, e.g. Android, since the target flags 
can influence the default include paths.

Note that, like isysroot (which is handled incorrectly above and elsewhere), 
the target flag doesn't fit cleanly into the ArgsToPreserve abstraction and 
does indeed have a different number of - in its = and non = forms. (ref 
)
 There are plenty of bugs in this file, but this is an incremental improvement.

For more context, please see https://github.com/clangd/clangd/issues/1378

Thanks for all you do, wonderful LLVM folks :)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138546

Files:
  clang-tools-extra/clangd/SystemIncludeExtractor.cpp


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/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) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits