[clang][deps] Fix clang-cl output argument parsing

[jansvoboda11]

Summary

One goal of the ad-hoc command line argument parsing in clang-scan-deps is to get the last output path value.

Currently, the algorithm walks the arguments in reverse and identifies potention output path arguments.

Interestingly, in clang-cl mode, the output path can be specified in multiple ways. For example /o /opt/build and /o/opt/build are equivalent. The parser currently fails to correctly parse the former. It considers the value (/opt/build/) to be a shorthand for /o pt/build. It doesn't look at the preceding /o.

This patch simplifies the algorithm by doing forward iteration and fixes the bug by correctly handling separate /o argument.

[saudi]

Nice! It's much more straightforward now, the reverse iteration was pretty confusing.
Thanks!

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

473

A test for boundary (I+1 != FlagsEnd) will be needed here, to avoid an infinite loop in the case -Xclang is the last arg.

I think it would be worth doing continue here, both because Arg's value is -Xclang, which won't be used in this iteration, and also it is a bit confusing as Arg has now represents *(I-1) instead of *I

478

llvm:: is unnecessary here, as it's not used for other StringRef in this file

479

The test for the end should be on I+1

[jansvoboda11]

Code review feedback

[saudi]

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

487

Before the patch LastO string allocation was done only once (because of the !LastO.empty() test).
It probably would be better to have it of type StringRef and allocate once at the end

clang/test/ClangScanDeps/cl-output.c

+// This test checks that the output path is correctly deduced/recognized in clang-cl mode.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- deduce-cdb.json.template
+[{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang --driver-mode=cl /c -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c -- DIR/test.c"
+}]
+
+//--- recognize-cdb.json.template
+[{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c -o DIR/test.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c /o DIR/test.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c -oDIR/test.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c /oDIR/test.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c -FoDIR/test.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c /FoDIR/test.o -- DIR/test.c"
+}]
+
+//--- last-arg-cdb.json.template
+[{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c -o DIR/test.o -o DIR/last.o -- DIR/test.c"
+},{
+  "file": "DIR/test.c",
+  "directory": "DIR",
+  "command": "clang-cl /c /o /opt/test.o -- DIR/test.c"
+}]
+
+//--- test.c
+
+// Check that missing output path is deduced (with both clang-cl executable and driver mode flag):
+//
+// RUN: sed -e "s|DIR|%/t|g" %t/deduce-cdb.json.template > %t/deduce-cdb.json
+// RUN: clang-scan-deps -compilation-database %t/deduce-cdb.json -j 1 > %t/deduce-result.d
+// RUN: cat %t/deduce-result.d | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix=CHECK-DEDUCE
+// CHECK-DEDUCE:      test.obj:
+// CHECK-DEDUCE-NEXT:   [[PREFIX]]/test.c
+// CHECK-DEDUCE-NEXT: test.obj:
+// CHECK-DEDUCE-NEXT:   [[PREFIX]]/test.c
+
+// Check that all the different ways to specify output file are recognized:
+//
+// RUN: sed -e "s|DIR|%/t|g" %t/recognize-cdb.json.template > %t/recognize-cdb.json
+// RUN: clang-scan-deps -compilation-database %t/recognize-cdb.json -j 1 > %t/recognize-result.d
+// RUN: cat %t/recognize-result.d | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix=CHECK-RECOGNIZE
+// CHECK-RECOGNIZE:      [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+// CHECK-RECOGNIZE-NEXT: [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+// CHECK-RECOGNIZE-NEXT: [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+// CHECK-RECOGNIZE-NEXT: [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+// CHECK-RECOGNIZE-NEXT: [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+// CHECK-RECOGNIZE-NEXT: [[PREFIX]]/test.o:
+// CHECK-RECOGNIZE-NEXT:   [[PREFIX]]/test.c
+
+// Check that the last argument specifying the output path wins.
+//
+// RUN: sed -e "s|DIR|%/t|g" %t/last-arg-cdb.json.template > %t/last-arg-cdb.json
+// RUN: clang-scan-deps -compilation-database %t/last-arg-cdb.json -j 1 > %t/last-arg-result.d
+// RUN: cat %t/last-arg-result.d | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t --check-prefix=CHECK-LAST
+// CHECK-LAST:      [[PREFIX]]/last.o:
+// CHECK-LAST-NEXT:   [[PREFIX]]/test.c
+// CHECK-LAST-NEXT: /opt/test.o:
+// CHECK-LAST-NEXT:   [[PREFIX]]/test.c

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

AdjustingCompilations->appendArgumentsAdjuster(
         bool HasResourceDir = false;
         bool ClangCLMode = false;
         auto FlagsEnd = llvm::find(Args, "--");
         if (FlagsEnd != Args.begin()) {
           ClangCLMode =
               llvm::sys::path::stem(Args[0]).contains_insensitive("clang-cl") ||
               llvm::is_contained(Args, "--driver-mode=cl");
 
-          // Reverse scan, starting at the end or at the element before "--".
-          auto R = std::make_reverse_iterator(FlagsEnd);
-          for (auto I = R, E = Args.rend(); I != E; ++I) {
+          for (auto I = Args.begin(); I < FlagsEnd; ++I) {
             StringRef Arg = *I;
             if (ClangCLMode) {
               // Ignore arguments that are preceded by "-Xclang".
-              if ((I + 1) != E && I[1] == "-Xclang")
+              if (Arg == "-Xclang") {
+                ++I;
                 continue;
-              if (LastO.empty()) {
-                // With clang-cl, the output obj file can be specified with
-                // "/opath", "/o path", "/Fopath", and the dash counterparts.
-                // Also, clang-cl adds ".obj" extension if none is found.
-                if ((Arg == "-o" || Arg == "/o") && I != R)
-                  LastO = I[-1]; // Next argument (reverse iterator)
-                else if (Arg.startswith("/Fo") || Arg.startswith("-Fo"))
-                  LastO = Arg.drop_front(3).str();
-                else if (Arg.startswith("/o") || Arg.startswith("-o"))
-                  LastO = Arg.drop_front(2).str();
+              }
+
+              StringRef CurrentO;
+              if ((Arg == "/o" || Arg == "-o") && I + 1 < FlagsEnd)
+                CurrentO = *++I;
+                CurrentO = Arg.drop_front(3);
+                CurrentO = Arg.drop_front(2);
 
-                if (!LastO.empty() && !llvm::sys::path::has_extension(LastO))
-                  LastO.append(".obj");
+              if (!CurrentO.empty()) {
+                if (!llvm::sys::path::has_extension(CurrentO))
+                  LastO = (CurrentO + ".obj").str();
+                else
+                  LastO = CurrentO.str();
               }
             }
             if (Arg == "-resource-dir")
               HasResourceDir = true;
           }
         }
         tooling::CommandLineArguments AdjustedArgs(Args.begin(), FlagsEnd);
         // The clang-cl driver passes "-o -" to the frontend. Inject the real