1324315 - Allow running our static analysis checks through clang-tidy

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

I have refactored most of our static analysis checks so that they can be compiled either as part of our clang plugin or in clang-tidy.  There is a Python script in this patch which should be run on top of a clang checkout to include our checks in the clang-tidy's checks list.

With a clang-tidy which supports these checks, you can pass |-checks=-*,mozilla-*| on the command line to get the full set of Mozilla checks to run.

Comment 1

[(no longer active)]

Attachment: Refactor the clang plugin to use a framework similar to clang-tidy's

Comment 2

[(no longer active)]

Attachment: Add support for building clang-tidy with Mozilla static analysis checks to build-clang.py

Comment 3

[(no longer active)]

The basic approach I used is as follows:

1. Introduce a base class called MozCheck that provides enough of the functionality of ClangTidyCheck.
2. Provide a preprocessor macro, CLANG_TIDY, which if defined will build this code for clang-tidy.
3. Make all of the checks inherit from a BaseCheck class which maps to either MozCheck or ClangTidyCheck depending on the build mode.
4. Rewrite the checks classes to make them adhere to the clang-tidy API.  The biggest change here was emitting diagnoses through BaseCheck::diag().
5. In import_mozilla_checks.py, copy all of the files in the directory to clang/tools/extra/clang-tidy/mozilla and then create a CMakeLists.txt file for all of the files (including MozillaTidyModule.cpp which is the glue code for clang-tidy and is absent in our moz.build) and patch up a few other files to integrate this directory into clang-tidy.
6. Provides Checks.inc and ChecksIncludes.inc as the files that need to updated when adding new checks, in a way that would get integrated into both the clang plugin and clang-tidy.

Currently the checks in MozChecker (specifically the must-use and must-override checks) are excluded from the portable subset because they use an AST visitor and aren't based on matchers.  It should be possible to port those as well.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8819696 [details] [diff] [review]
Refactor the clang plugin to use a framework similar to clang-tidy's

Review of attachment 8819696 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/CustomTypeAnnotation.h
@@ +39,5 @@
>    bool hasEffectiveAnnotation(QualType T) {
>      return directAnnotationReason(T).valid();
>    }
>    void dumpAnnotationReason(DiagnosticsEngine &Diag, QualType T,
>                              SourceLocation Loc);

Is this branch of dumpAnnotationReason still used? I'm guessing it's still used in MozChecker.cpp for the analyses which aren't supported in the clang tidy check yet like MOZ_MUST_OVERRIDE and MOZ_MUST_USE_TYPE.

If/when those are fixed I'd like to remove this method.

::: build/clang-plugin/MozCheck.h
@@ +16,5 @@
> +  DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
> +                         DiagnosticIDs::Level Level = DiagnosticIDs::Warning) {
> +    DiagnosticsEngine &Diag = Context->getDiagnostics();
> +    unsigned ID = Diag.getDiagnosticIDs()->getCustomDiagID(Level, Description);
> +    // We treat all diagnostics as errors here.

Are you sure we're treating all diagnostics as errors here? This seems to me like by default we would be reporting a DiagnosticIDs::Warning.

I think that switching the default to DiagnosticIDs::Error seems like it might be a good idea, or at the very least we probably want to remove this comment (unless it means something other than what I think it does).

::: build/clang-plugin/plugin.h
@@ +58,5 @@
> +// compatible base check class called MozCheck, and we use the preprocessor to
> +// decide which base class to pick.
> +#ifdef CLANG_TIDY
> +#include "../ClangTidy.h"
> +#define BaseCheck clang::tidy::ClangTidyCheck

Why can't we use a typedef clang::tidy::ClangTidyCheck BaseCheck; here? It seems much nicer than using a preprocessor macro.

@@ +62,5 @@
> +#define BaseCheck clang::tidy::ClangTidyCheck
> +typedef clang::tidy::ClangTidyContext ContextType;
> +#else
> +#define BaseCheck MozCheck
> +#include "MozCheck.h"

Why not just call it `BaseCheck` directly, and avoid the typedef in this branch?

I'd also be OK with us defining a `class BaseCheck : public '
#ifdef CLANG_TIDY
  public clang::tidy::ClangTidyCheck
#else
  public MozCheck
#endif
{}`

which would allow us to add methods to the type which are present in both the clang-tidy and clang-plugin versions, but it may be unnecessary.

Comment 5

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #4)
> Comment on attachment 8819696 [details] [diff] [review]
> Refactor the clang plugin to use a framework similar to clang-tidy's
> 
> Review of attachment 8819696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/CustomTypeAnnotation.h
> @@ +39,5 @@
> >    bool hasEffectiveAnnotation(QualType T) {
> >      return directAnnotationReason(T).valid();
> >    }
> >    void dumpAnnotationReason(DiagnosticsEngine &Diag, QualType T,
> >                              SourceLocation Loc);
> 
> Is this branch of dumpAnnotationReason still used? I'm guessing it's still
> used in MozChecker.cpp for the analyses which aren't supported in the clang
> tidy check yet like MOZ_MUST_OVERRIDE and MOZ_MUST_USE_TYPE.

Yes, exactly.

> If/when those are fixed I'd like to remove this method.

It is already removed in bug 1324328.  :-)

> ::: build/clang-plugin/MozCheck.h
> @@ +16,5 @@
> > +  DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
> > +                         DiagnosticIDs::Level Level = DiagnosticIDs::Warning) {
> > +    DiagnosticsEngine &Diag = Context->getDiagnostics();
> > +    unsigned ID = Diag.getDiagnosticIDs()->getCustomDiagID(Level, Description);
> > +    // We treat all diagnostics as errors here.
> 
> Are you sure we're treating all diagnostics as errors here? This seems to me
> like by default we would be reporting a DiagnosticIDs::Warning.
> 
> I think that switching the default to DiagnosticIDs::Error seems like it
> might be a good idea, or at the very least we probably want to remove this
> comment (unless it means something other than what I think it does).

This comment is just left-over from a previous approach, where I tried to default all diagnostics to errors, but I realized that's a bad idea for clang-tidy compat since the default there would be warnings and we don't want to have separate behaviors if in the future we start to use both warnings and errors.  So instead I ended up passing the diagnostic kind explicitly everywhere.

> ::: build/clang-plugin/plugin.h
> @@ +58,5 @@
> > +// compatible base check class called MozCheck, and we use the preprocessor to
> > +// decide which base class to pick.
> > +#ifdef CLANG_TIDY
> > +#include "../ClangTidy.h"
> > +#define BaseCheck clang::tidy::ClangTidyCheck
> 
> Why can't we use a typedef clang::tidy::ClangTidyCheck BaseCheck; here? It
> seems much nicer than using a preprocessor macro.

Sure, good idea!

> @@ +62,5 @@
> > +#define BaseCheck clang::tidy::ClangTidyCheck
> > +typedef clang::tidy::ClangTidyContext ContextType;
> > +#else
> > +#define BaseCheck MozCheck
> > +#include "MozCheck.h"
> 
> Why not just call it `BaseCheck` directly, and avoid the typedef in this
> branch?

That didn't occur to me for some reason, but it's a great idea!

> I'd also be OK with us defining a `class BaseCheck : public '
> #ifdef CLANG_TIDY
>   public clang::tidy::ClangTidyCheck
> #else
>   public MozCheck
> #endif
> {}`
> 
> which would allow us to add methods to the type which are present in both
> the clang-tidy and clang-plugin versions, but it may be unnecessary.

We can always switch to that in the future when we have a candidate.  So far it seems the current interface is sufficient for all existing checks.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8222951c9369
Refactor the clang plugin to use a framework similar to clang-tidy's; r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/409bdaed6b9f
Add support for building clang-tidy with Mozilla static analysis checks to build-clang.py; r=mystor

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8222951c9369
https://hg.mozilla.org/mozilla-central/rev/409bdaed6b9f