[clang-scan-deps] : Support -- in clang command lines.

[saudi]

Summary

This fixes argument injection when clang command line contains "--" in the argument.

Previously, the arguments were injected at the end of the command line. It could be added after --, and be interpreted as input file paths.

This fix is needed for a subsequent patch, D92191

[saudi]

Bugfixes on the use of the iterator.

[dexonsmith]

Thanks for splitting this out! The looks good, just a few nits in line below. I also have a question about the test.

clang/lib/Tooling/Tooling.cpp

440

I think another name that doesn't sound like a synonym of Args.end() would be more clear. Maybe LastFlag or FlagsEnd?

clang/test/ClangScanDeps/Inputs/regular_cdb.json

12–16

It's not obvious to me how this exercises the new -resource-dir logic. Can you walk me through it?

clang/tools/clang-scan-deps/ClangScanDeps.cpp

424

I think it'd be a bit clearer to name this LastFlag or LastOption or FlagsEnd or something, since this is not the last argument, just the last flag/option. Especially helpful on the last usage below when referencing the range from ArgsEnd to Args.end().

426–427

Should this condition be instead check for LastFlag != Args.begin()?

429

Nit: since you're touching this line anyway, would be nice to update it to follow the usual LLVM style of calling rend() just once instead of on every iteration:

for (auto RB = llvm::make_reverse_iterator(LastFlag), RI = RB, RE = Args.rend();
     RI != RE; ++RI) {

Up to you, but I find the initialism RI a bit more indicative of a reverse iterator... this patch touches every use so you could rename it without pulling in any extra diff. If you prefer the names you have that's fine too though.

Also, if you narrow the scope of RB / ArgsRBegin you can probably reduce nesting by deleting the redundant if, but that'd be nice to commit separately (in a follow up?) to avoid cluttering up this patch with NFC indentation changes.

435

Does RI[-1] work here? Personally I find x[y] more clear than *(x + y).

481

Can we use AdjustedArgs.append(LastFlag, Args.end()) here?

[saudi]

Updated for suggested modifications

[saudi]

clang/test/ClangScanDeps/Inputs/regular_cdb.json

12–16

clang tools like clang-scan-deps may edit the provided clang command line, especially to override the resource directory with their own default value in case it is not found as an argument.

This patch avoids inserting new arguments after --, since they would be wrongly interpreted as extra input files.

By adding -- in this test, we broadly verify that no argument inserted by clang-scan-deps is treated as an extra input file. -resource-dir is merely one possible case.

Note that -resource-dir case is a bit special: clang-scan-deps will first try to force its value using the result of clang -print-resource-dir (see ClangScanDeps.cpp).
Yet, this heuristic may fail (and it does in this clang-scan-deps test, where the clang command is not given as an absolute path). In that case ClangTool will also try to add -resource-dir (see Tooling.cpp).

clang/tools/clang-scan-deps/ClangScanDeps.cpp

481

AdjustedArgs is of type tooling::CommandLineArguments which inherits from std::vector, it doesn't have append().

[saudi]

Ping!

[dexonsmith]

LGTM!

clang/lib/Tooling/Tooling.cpp

 
 void ClangTool::clearArgumentsAdjusters() {
   ArgsAdjuster = nullptr;
 }
 
 static void injectResourceDir(CommandLineArguments &Args, const char *Argv0,
                               void *MainAddr) {
   // Allow users to override the resource dir.
-  for (StringRef Arg : Args)
+  // Don't search/inject past "--", as the subsequent arguments are only file
+  // names.
+  auto FlagsEnd = Args.end();
+  for (auto B = Args.begin(), I = B, E = Args.end(); I != E; ++I) {
+    StringRef Arg = *I;
+    if (Arg == "--") {
+      FlagsEnd = I;
+      break;
+    }
     if (Arg.startswith("-resource-dir"))
       return;
+  }
 
   // If there's no override in place add our resource dir.
-  Args.push_back("-resource-dir=" +
-                 CompilerInvocation::GetResourcesPath(Argv0, MainAddr));
+  Args.insert(FlagsEnd, "-resource-dir=" + CompilerInvocation::GetResourcesPath(
+                                               Argv0, MainAddr));
 }
 
 int ClangTool::run(ToolAction *Action) {
   // Exists solely for the purpose of lookup of the resource path.
   // This just needs to be some symbol in the binary.
   static int StaticSymbol;
 
   // First insert all absolute paths into the in-memory VFS. These are global

clang/test/ClangScanDeps/Inputs/regular_cdb.json

 [
 {
   "directory": "DIR",
   "command": "clang -E -fsyntax-only DIR/regular_cdb_input2.cpp -IInputs -D INCLUDE_HEADER2 -MD -MF DIR/regular_cdb2.d",
   "file": "DIR/regular_cdb_input2.cpp"
 },
 {
   "directory": "DIR",
   "command": "clang -E DIR/regular_cdb_input.cpp -IInputs",
   "file": "DIR/regular_cdb_input.cpp"
 },
 {
   "directory": "DIR",
-  "command": "clang -E DIR/regular_cdb_input.cpp -IInputs -o adena.o",
+  "command": "clang -E -IInputs -o adena.o -- DIR/regular_cdb_input.cpp",
   "file": "DIR/regular_cdb_input.cpp"
 }
 ]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

int main(int argc, const char **argv) {
   AdjustingCompilations->appendArgumentsAdjuster(
       [&ResourceDirCache](const tooling::CommandLineArguments &Args,
                           StringRef FileName) {
         std::string LastO = "";
         bool HasMT = false;
         bool HasMQ = false;
         bool HasMD = false;
         bool HasResourceDir = false;
+        auto FlagsEnd = llvm::find(Args, "--");
+
         // We need to find the last -o value.
-        if (!Args.empty()) {
-          std::size_t Idx = Args.size() - 1;
-          for (auto It = Args.rbegin(); It != Args.rend(); ++It) {
-            StringRef Arg = Args[Idx];
+        if (FlagsEnd != Args.begin()) {
+          // Reverse scan, starting at the end or at the element before "--".
+          for (auto RB = llvm::make_reverse_iterator(FlagsEnd), RI = RB,
+                    RE = Args.rend();
+               RI != RE; ++RI) {
+            StringRef Arg = *RI;
             if (LastO.empty()) {
-              if (Arg == "-o" && It != Args.rbegin())
-                LastO = Args[Idx + 1];
+              if (Arg == "-o" && RI != RB)
+                LastO = RI[-1]; // Next argument (reverse iterator)
               else if (Arg.startswith("-o"))
                 LastO = Arg.drop_front(2).str();
             }
             if (Arg == "-MT")
               HasMT = true;
             if (Arg == "-MQ")
               HasMQ = true;
             if (Arg == "-MD")
               HasMD = true;
             if (Arg == "-resource-dir")
               HasResourceDir = true;
-            --Idx;
           }
         }
         // If there's no -MT/-MQ Driver would add -MT with the value of the last
         // -o option.
-        tooling::CommandLineArguments AdjustedArgs = Args;
+        tooling::CommandLineArguments AdjustedArgs(Args.begin(), FlagsEnd);
         AdjustedArgs.push_back("-o");
         AdjustedArgs.push_back("/dev/null");
         if (!HasMT && !HasMQ) {
           AdjustedArgs.push_back("-M");
           AdjustedArgs.push_back("-MT");
           // We're interested in source dependencies of an object file.
           if (!HasMD) {
             // FIXME: We are missing the directory unless the -o value is an
AdjustingCompilations->appendArgumentsAdjuster(
         if (!HasResourceDir) {
           StringRef ResourceDir =
               ResourceDirCache.findResourceDir(Args);
           if (!ResourceDir.empty()) {
             AdjustedArgs.push_back("-resource-dir");
             AdjustedArgs.push_back(std::string(ResourceDir));
           }
         }
+        AdjustedArgs.insert(AdjustedArgs.end(), FlagsEnd, Args.end());
         return AdjustedArgs;
       });
   AdjustingCompilations->appendArgumentsAdjuster(
       tooling::getClangStripSerializeDiagnosticAdjuster());
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());