[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 ↗

(On Diff #318214)

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

420–421

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

421

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

423

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.

429

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

475

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

475

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

[saudi]

Ping!

[dexonsmith]

LGTM!

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) {
   ResourceDirectoryCache ResourceDirCache;
   AdjustingCompilations->appendArgumentsAdjuster(
       [&ResourceDirCache](const tooling::CommandLineArguments &Args,
                           StringRef FileName) {
         std::string LastO = "";
         bool HasMT = false;
         bool HasMQ = false;
         bool HasMD = false;
         bool HasResourceDir = false;
-        // 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];
+        auto FlagsEnd = llvm::find(Args, "--");
+        if (FlagsEnd != Args.begin()) {
+          // Reverse scan, starting at the end or at the element before "--".
+          auto R = llvm::make_reverse_iterator(FlagsEnd);
+          for (auto I = R, E = Args.rend(); I != E; ++I) {
+            StringRef Arg = *I;
             if (LastO.empty()) {
-              if (Arg == "-o" && It != Args.rbegin())
-                LastO = Args[Idx + 1];
+              if (Arg == "-o" && I != R)
+                LastO = I[-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());