Make clang-tidy work with clang-cl

[zturner]

Summary

Attempting to run clang-tidy with the clang-cl driver results in a number of warnings and errors that make the tool unusable. The two main issues, and the solutions implemented here are:

1. Command lines are tokenized differently on Windows and non-windows platforms. Using standard tokenization on Windows leads to -- among other things -- all path separators being stripped from paths. Obviously this won't work, so the fix implemented here is to use the correct windows command line tokenizer when clang-tidy is running on Windows.
2. The underlying clang driver does not attempt to auto-detect driver mode (CL, GCC, etc) from the program name. It relies only on the existence of a --driver-mode=<mode> option. We already have a function to detect the driver mode based on the program name, so the solution implemented here is to use the program name as a default, which will be overridden by the existence of a --driver-mode option.

With this patch, clang-tidy runs with clang-cl.

D:\src\llvm\tools\lldb>d:\src\llvmbuild\ninja\bin\clang-tidy.exe source\lldb.cpp
1448 warnings generated.
warning: argument unused during compilation: '-mincremental-linker-compatible' [clang-diagnostic-unused-command-line-argument]
warning: unknown argument ignored in clang-cl: '-resource-dir=d:\src\llvmbuild\ninja\bin\..\lib\clang\4.0.0' [clang-diagnostic-unk
nown-argument]
D:\src\llvm\tools\lldb\source\lldb.cpp:71:24: warning: invalid case style for variable 'g_version_str' [readability-identifier-nam
ing]
    static std::string g_version_str;
                       ^
D:\src\llvm\tools\lldb\source\lldb.cpp:76:22: warning: invalid case style for variable 'lldb_repo' [readability-identifier-naming]

        const char * lldb_repo = GetLLDBRepository();
                     ^
D:\src\llvm\tools\lldb\source\lldb.cpp:83:21: warning: invalid case style for variable 'lldb_rev' [readability-identifier-naming]
        const char *lldb_rev = GetLLDBRevision();
                    ^
D:\src\llvm\tools\lldb\source\lldb.cpp:89:21: warning: invalid case style for variable 'clang_rev' [readability-identifier-naming]

        std::string clang_rev (clang::getClangRevision());
                    ^
D:\src\llvm\tools\lldb\source\lldb.cpp:95:21: warning: invalid case style for variable 'llvm_rev' [readability-identifier-naming]
        std::string llvm_rev (clang::getLLVMRevision());
                    ^
Suppressed 1441 warnings (1441 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as
well.

[aaron.ballman]

Patch generally LGTM, though I wonder if there's a way we can add test coverage for this.

[rnk]

lib/Driver/Driver.cpp

93

Why not change ToolInvocation::run() to behave more like clang's main? I'd rather not do this twice, mostly for consistency with the regular driver, not because it's inefficient.

lib/Tooling/JSONCompilationDatabase.cpp

119

It would be nice if the JSON file just told us which quoting mechanism it was using. You can imagine building the compilation database on one system and sending it off to another for indexing.

127

I bet we could replace this with TokenizeGNUCommandLine some other day.

[zturner]

In D23409#512720, @aaron.ballman wrote:

Patch generally LGTM, though I wonder if there's a way we can add test coverage for this.

I'd imagine we can just enable the clang-tidy test suite on Windows (I'm assuming it's currently disabled). I'll look into it.

[zturner]

lib/Tooling/JSONCompilationDatabase.cpp

119

Correct me if I've got this wrong but:

1. If you had a compile command database generated on non-Windows, and you used it on Windows, then it would necessarily be referring to something other than clang-cl right? In which case, we don't support running clang in non-cl mode on Windows.

2. If you had a compile command database generated on Windows, then the only supported configuration would involve clang-cl, in which case you couldn't use it on non-Windows.

Is there a supported use case where a non-Windows compile database would be used on Windows or vice versa?

[zturner]

lib/Driver/Driver.cpp

93

I am very inexperienced with clang's driver model, but it seems to me like putting it here is actually the *correct* way, and what clang is doing is sort of a hackish workaround for the fact that it needs to parse response files the way it does.

If someone else comes along and builds another tool based off of clang, we don't want them to have to remember to do all this every single time. Seems like the Driver should "just work", and if a particular tool (such as clang) needs to do something funky, that's on the tool.

Thoughts?

[zturner]

In D23409#512745, @zturner wrote:

In D23409#512720, @aaron.ballman wrote:

Patch generally LGTM, though I wonder if there's a way we can add test coverage for this.

I'd imagine we can just enable the clang-tidy test suite on Windows (I'm assuming it's currently disabled). I'll look into it.

Actually yea I'm not sure how to go about doing this. All the existing tests are written to use GCC style command line, and it seems unreasonable to add a cl style command line variant of every single test. maybe alexfh@ or someone has a suggestion.

[alexfh]

In D23409#512819, @zturner wrote:

In D23409#512745, @zturner wrote:

In D23409#512720, @aaron.ballman wrote:

Patch generally LGTM, though I wonder if there's a way we can add test coverage for this.

I'd imagine we can just enable the clang-tidy test suite on Windows (I'm assuming it's currently disabled). I'll look into it.

Actually yea I'm not sure how to go about doing this. All the existing tests are written to use GCC style command line, and it seems unreasonable to add a cl style command line variant of every single test. maybe alexfh@ or someone has a suggestion.

I think a single test for cl-style command line variant should be enough, if it verifies different aspects of the transformation of the command line (/ prefix for flags, \ path separators, different quoting, what else?).

[alexfh]

May I ask you to upload patches with full diff context next time? I know, it's not directly supported by TortoiseGit, but there are at least two other reasonable ways of generating full diffs for Phabricator: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface and http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.

Wrt testing, I think, apart from a new clang-tidy test, the changes to Driver and Tooling need separate tests as well.

lib/Tooling/JSONCompilationDatabase.cpp

119

we don't support running clang in non-cl mode on Windows.

I think, clang in non-cl mode is functional on Windows to a certain degree (mingw-compatibility or something like this). Artificially preventing it from working doesn't seem to be right.

[zturner]

In D23409#513017, @alexfh wrote:

May I ask you to upload patches with full diff context next time? I know, it's not directly supported by TortoiseGit, but there are at least two other reasonable ways of generating full diffs for Phabricator: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface and http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.

Wrt testing, I think, apart from a new clang-tidy test, the changes to Driver and Tooling need separate tests as well.

I'm unsure what to actually test in the Driver and Tooling tests. For example, in Tooling we just change from using one function to using another function. And that function is already tested in the LLVM test suite. It seems redundant to add another test that is for all intents and purposes equivalent to "Does LLVM's windows command line parser work correctly?", which is already tested.

How necessary is this?

For the clang-tidy test, I will need to make it only enabled on Windows, since other platforms are not going to be building clang-cl. I think I should be able to figure that out.

[zturner]

I added a test for the clang Driver changes.

It seems I'm still having some trouble writing the clang-cl test for clang-tidy. It depends on having a different compilation database that uses clang-cl instead of clang. I found one set of tests called clang-tidy-run-with-database.cpp but it requires shell, which will make it not work on Windows, which is the main place it should be tested in the first place.

[alexfh]

The changes in Driver LGTM (though I'd prefer someone who knows Driver to look).

Could you move lib/Tooling/JSONCompilationDatabase.cpp to a separate patch? It seems like an orthogonal change.

As for a clang-cl-mode test for clang-tidy, clang-tidy-run-with-database.cpp is only tangentially related. We probably need two tests: one with a JSON compilation database and the other with a fixed compilation database (passing compiler arguments after --). In both cases I'd use flags like /D to verify correct parsing of the command line. Something along the lines of:

// RUN: clang-tidy -checks=-*,modernize-use-nullptr %s -- clang-cl /DTEST1 /DFOO=foo /DBAR#bar | FileCheck -implicit-check-not="{{warning|error}}:" %s
int *a = 0;
// CHECK: :[[@LINE-1]]:10: warning: use nullptr
#ifdef TEST1
int *b = 0;
// CHECK: :[[@LINE-1]]:10: warning: use nullptr
#endif
#define foo 1
#define bar 1
#if FOO
int *c = 0;
// CHECK: :[[@LINE-1]]:10: warning: use nullptr
#endif
#if BAR
int *d = 0;
// CHECK: :[[@LINE-1]]:10: warning: use nullptr
#endif

Maybe some other aspects of clang-cl mode options parsing can be tested this way as well.

[zturner]

Once I split the JSONCompilationDatabase patch out, then the rest is strictly a driver-only patch, and the other one is strictly a tooling patch. So I will abandon this one and upload two new patches with a more targeted set of reviewers.

include/clang/Driver/Driver.h

   /// @{
 
   /// ParseDriverMode - Look for and handle the driver mode option in Args.
-  void ParseDriverMode(ArrayRef<const char *> Args);
+  void ParseDriverMode(StringRef ProgramName, ArrayRef<const char *> Args);
 
   /// ParseArgStrings - Parse the given list of strings into an
   /// ArgList.

   LTOKind getLTOMode() const { return LTOMode; }
 
 private:
+  /// Set the driver mode (cl, gcc, etc) from an option string of the form
+  /// --driver-mode=<mode>.
+  void setDriverModeFromOption(StringRef Opt);
+
   /// Parse the \p Args list for LTO options and record the type of LTO
   /// compilation based on which -f(no-)?lto(=.*)? option occurs last.
   void setLTOMode(const llvm::opt::ArgList &Args);

lib/Driver/Driver.cpp

   llvm::DeleteContainerSeconds(ToolChains);
 }
 
-void Driver::ParseDriverMode(ArrayRef<const char *> Args) {
-  const std::string OptName =
-      getOpts().getOption(options::OPT_driver_mode).getPrefixedName();
+void Driver::ParseDriverMode(StringRef ProgramName,
+                             ArrayRef<const char *> Args) {
+  auto Default = ToolChain::getTargetAndModeFromProgramName(ProgramName);
+  StringRef DefaultMode(Default.second);
+  setDriverModeFromOption(DefaultMode);
 
   for (const char *ArgPtr : Args) {
     // Ingore nullptrs, they are response file's EOL markers
     if (ArgPtr == nullptr)
       continue;
     const StringRef Arg = ArgPtr;
-    if (!Arg.startswith(OptName))
-      continue;
+    setDriverModeFromOption(Arg);
+  }
+}
+
+void Driver::setDriverModeFromOption(StringRef Opt) {
+  const std::string OptName =
+      getOpts().getOption(options::OPT_driver_mode).getPrefixedName();
+  if (!Opt.startswith(OptName))
+    return;
+  StringRef Value = Opt.drop_front(OptName.size());
 
-    const StringRef Value = Arg.drop_front(OptName.size());
-    const unsigned M = llvm::StringSwitch<unsigned>(Value)
-                           .Case("gcc", GCCMode)
-                           .Case("g++", GXXMode)
-                           .Case("cpp", CPPMode)
-                           .Case("cl", CLMode)
-                           .Default(~0U);
+  const unsigned M = llvm::StringSwitch<unsigned>(Value)
+                         .Case("gcc", GCCMode)
+                         .Case("g++", GXXMode)
+                         .Case("cpp", CPPMode)
+                         .Case("cl", CLMode)
+                         .Default(~0U);
 
-    if (M != ~0U)
-      Mode = static_cast<DriverMode>(M);
-    else
-      Diag(diag::err_drv_unsupported_option_argument) << OptName << Value;
-  }
 }
 
 InputArgList Driver::ParseArgStrings(ArrayRef<const char *> ArgStrings) {

 
   // We look for the driver mode option early, because the mode can affect
   // how other options are parsed.
-  ParseDriverMode(ArgList.slice(1));
+  ParseDriverMode(ArgList[0], ArgList.slice(1));
 
   // FIXME: What are we going to do with -V and -b?
 

lib/Tooling/JSONCompilationDatabase.cpp

 #include "clang/Tooling/CompilationDatabasePluginRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/StringSaver.h"
 #include <system_error>
 
 namespace clang {

 
 std::vector<std::string> unescapeCommandLine(
     StringRef EscapedCommandLine) {
+#if defined(LLVM_ON_WIN32)
+  llvm::BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  llvm::SmallVector<const char *, 64> T;
+  llvm::cl::TokenizeWindowsCommandLine(EscapedCommandLine, Saver, T);
+  std::vector<std::string> Result(T.begin(), T.end());
+  return Result;
+#else
   CommandLineArgumentParser parser(EscapedCommandLine);
   return parser.parse();
+#endif
 }
 
 class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {