[PATCH] D122385: [clang][deps] Fix clang-cl output argument parsing

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith, saudi.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122385

Files:
  clang/test/ClangScanDeps/cl-output.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===================================================================
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -470,27 +470,29 @@
               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")
-                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();
-
-                if (!LastO.empty() && !llvm::sys::path::has_extension(LastO))
-                  LastO.append(".obj");
+              if (Arg == "-Xclang")
+                ++I;
+
+              // 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.
+              llvm::StringRef CurrentO;
+              if ((Arg == "/o" || Arg == "-o") && I != FlagsEnd)
+                CurrentO = *++I;
+              else if (Arg.startswith("/Fo") || Arg.startswith("-Fo"))
+                CurrentO = Arg.drop_front(3);
+              else if (Arg.startswith("/o") || Arg.startswith("-o"))
+                CurrentO = Arg.drop_front(2);
+
+              if (!CurrentO.empty()) {
+                if (!llvm::sys::path::has_extension(CurrentO))
+                  LastO = (CurrentO + ".obj").str();
+                else
+                  LastO = CurrentO.str();
               }
             }
             if (Arg == "-resource-dir")
Index: clang/test/ClangScanDeps/cl-output.c
===================================================================
--- clang/test/ClangScanDeps/cl-output.c
+++ clang/test/ClangScanDeps/cl-output.c
@@ -46,6 +46,10 @@
   "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
@@ -81,7 +85,9 @@
 // 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 > %t/last-arg-result.d
+// 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122385.417888.patch
Type: text/x-patch
Size: 3674 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220324/944760a8/attachment.bin>