getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[shivanshu3]

Summary

MSVC's cl.exe has a few command line arguments which start with -M such as "-MD", "-MDd", "-MT", "-MTd", "-MP".
These arguments are not dependency file generation related, and these arguments were being removed by getClangStripDependencyFileAdjuster() which was wrong.

[hans]

Seems reasonable to me. Just some nits.

clang/lib/Tooling/ArgumentsAdjusters.cpp

29

Instead of manually indexing into the StringRef, the I think the if-statement could be replaced by

if (ArgRef.consume_front("--driver-mode="))
  return ArgRef;

32

I'd drop the Optional and just let the empty StringRef signal not finding the driver mode.

107

If getDriverMode() didn't return Optional, this could be simplified to

bool UsingClDriver = (getDriverMode() == "cl");

110–111

This line and the "All dependency-file options begin with -M" line above seem to contradict each other. Perhaps the could be united somehow.

112

Instead of a bool and if below, I'd suggest if-statements and early continues instead. Breaking up the old if-statement is nice though, and maybe the comment from above about what flags to ignore could be moved down here to those if statements. For example:

// -M flags blah blah
if (Arg.startswith("-M") && !UsingClDriver)
  continue;
// MSVC flags blah blah
if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
  continue;
AdjustedArgs.push_back(Args[i]);

126–127

Need to handle -MT too I suppose

[shivanshu3]

• Simplified the implementation of getDriverMode and got rid of the Optional return type.
• When using the cl driver mode, we do not want to skip the next argument for -MF, -MT, -MQ.

[shivanshu3]

clang/lib/Tooling/ArgumentsAdjusters.cpp

112

I realized that with my original change, we would skip the next argument under cl driver mode when -MT was used, which would be wrong. The next argument should only be skipped when IsDependencyFileArg is true. So I think it might be cleaner to keep that extra boolean so the code is easy to read and understand. What do you think?

[hans]

clang/lib/Tooling/ArgumentsAdjusters.cpp

112

I think having the boolean variable is still more confusing (it adds more state to keep track of) than just handling the cases with if-statements and early continue.

How about:

// -M flags that take an arg..
if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
  ++i;
  continue;
}
// -M flags blah blah
if (!UsingClDriver && Arg.startswith("-M"))
  continue;
// MSVC flags blah blah
if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
  continue;

AdjustedArgs.push_back(Args[i]);

?

[shivanshu3]

• Remove the bool IsDependencyFileArg in the implementation of getClangStripDependencyFileAdjuster() to make it simpler.
• Add an extra argument after -MT in the test case to ensure we do not strip arguments after -MT when using the cl driver mode.

[shivanshu3]

clang/lib/Tooling/ArgumentsAdjusters.cpp

112

Yup I agree that looks better :)

[shivanshu3]

Note that I do not have commit access and this change will have to be committed by someone else on my behalf. Thanks!

clang/lib/Tooling/ArgumentsAdjusters.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include <cstddef>
 #include <vector>
 
 namespace clang {
 namespace tooling {
 
+static StringRef getDriverMode(const CommandLineArguments &Args) {
+  for (const auto &Arg : Args) {
+    StringRef ArgRef = Arg;
+    if (ArgRef.consume_front("--driver-mode=")) {
+      return ArgRef;
+    }
+  }
+  return StringRef();
+}
+
 /// Add -fsyntax-only option and drop options that triggers output generation.
 ArgumentsAdjuster getClangSyntaxOnlyAdjuster() {
   return [](const CommandLineArguments &Args, StringRef /*unused*/) {
     CommandLineArguments AdjustedArgs;
     bool HasSyntaxOnly = false;
     constexpr llvm::StringRef OutputCommands[] = {
         // FIXME: Add other options that generate output.
         "-save-temps",
for (size_t i = 0, e = Args.size(); i < e; ++i) {
       AdjustedArgs.push_back(Args[i]);
     }
     return AdjustedArgs;
   };
 }
 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments &Args, StringRef /*unused*/) {
+    auto UsingClDriver = (getDriverMode(Args) == "cl");
+
     CommandLineArguments AdjustedArgs;
     for (size_t i = 0, e = Args.size(); i < e; ++i) {
       StringRef Arg = Args[i];
-      // All dependency-file options begin with -M. These include -MM,
-      // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-      if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes") &&
-          !Arg.startswith("-showIncludes")) {
-        AdjustedArgs.push_back(Args[i]);
-        continue;
-      }
 
-      if (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")
-        // These flags take an argument: -MX foo. Skip the next argument also.
+      if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
         ++i;
+        continue;
+      }
+      // When not using the cl driver mode, dependency file generation options
+      // begin with -M. These include -MM, -MF, -MG, -MP, -MT, -MQ, -MD, and
+      // -MMD.
+      if (!UsingClDriver && Arg.startswith("-M"))
+        continue;
+      // Under MSVC's cl driver mode, dependency file generation is controlled
+      // using /showIncludes
+      if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
+        continue;
+
+      AdjustedArgs.push_back(Args[i]);
     }
     return AdjustedArgs;
   };
 }
 
 ArgumentsAdjuster getInsertArgumentAdjuster(const CommandLineArguments &Extra,
                                             ArgumentInsertPosition Pos) {
   return [Extra, Pos](const CommandLineArguments &Args, StringRef /*unused*/) {

clang/unittests/Tooling/ToolingTest.cpp

TEST(ClangToolTest, StripDependencyFileAdjusterShowIncludes) {
   };
   EXPECT_FALSE(HasFlag("/showIncludes"));
   EXPECT_FALSE(HasFlag("/showIncludes:user"));
   EXPECT_FALSE(HasFlag("-showIncludes"));
   EXPECT_FALSE(HasFlag("-showIncludes:user"));
   EXPECT_TRUE(HasFlag("-c"));
 }
 
+// Check getClangStripDependencyFileAdjuster doesn't strip args when using the
+// MSVC cl.exe driver
+TEST(ClangToolTest, StripDependencyFileAdjusterMsvc) {
+  FixedCompilationDatabase Compilations(
+      "/", {"--driver-mode=cl", "-MD", "-MDd", "-MT", "-O1", "-MTd", "-MP"});
+
+  ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr<FrontendActionFactory> Action(
+      newFrontendActionFactory<SyntaxOnlyAction>());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+      [&FinalArgs](const CommandLineArguments &Args, StringRef /*unused*/) {
+        FinalArgs = Args;
+        return Args;
+      };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [&FinalArgs](const std::string &Flag) {
+    return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_TRUE(HasFlag("-MD"));
+  EXPECT_TRUE(HasFlag("-MDd"));
+  EXPECT_TRUE(HasFlag("-MT"));
+  EXPECT_TRUE(HasFlag("-O1"));
+  EXPECT_TRUE(HasFlag("-MTd"));
+  EXPECT_TRUE(HasFlag("-MP"));
+}
+
 // Check getClangStripPluginsAdjuster strips plugin related args.
 TEST(ClangToolTest, StripPluginsAdjuster) {
   FixedCompilationDatabase Compilations(
       "/", {"-Xclang", "-add-plugin", "-Xclang", "random-plugin"});
 
   ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
   Tool.mapVirtualFile("/a.cc", "void a() {}");