Page MenuHomePhabricator

Make clang-tidy work with clang-cl
AbandonedPublic

Authored by zturner on Aug 11 2016, 10:02 AM.

Details

Reviewers
djasper
alexfh
rnk
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.

Diff Detail

Event Timeline

zturner updated this revision to Diff 67698.Aug 11 2016, 10:02 AM
zturner retitled this revision from to Make clang-tidy work with clang-cl.
zturner updated this object.
zturner added reviewers: alexfh, djasper, rnk.
zturner added a subscriber: cfe-commits.

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

rnk added inline comments.Aug 11 2016, 10:23 AM
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.

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 added inline comments.Aug 11 2016, 10:33 AM
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.
  1. 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 added inline comments.Aug 11 2016, 10:44 AM
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?

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 edited edge metadata.Aug 11 2016, 1:31 PM

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

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.

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 updated this revision to Diff 67751.Aug 11 2016, 3:01 PM
zturner edited edge metadata.

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.

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.

alexfh requested changes to this revision.Aug 12 2016, 10:16 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Aug 12 2016, 10:16 AM
zturner abandoned this revision.Aug 12 2016, 10:29 AM

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.