767563 - Build a clang static checker

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: First cut.

This bug builds the clang-plugin as part of the build system and uses it to test the build, at least for NS_MUST_OVERRIDE. That is all that could be feasibly done right now with the clang attribute system as limited as it is. It also turns out that layout/style won't build, since they violate this attribute (people don't implement GetSizeExcludingThis). It also relies on the new plugin architecture I'm adding, so I won't check this in until clang gets it in the first place.

Comment 1

[(no longer active)]

Comment on attachment 635896 [details] [diff] [review]
First cut.

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

This is awesome, thanks for working on this, Joshua!

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Add a plugin

Newer set of patches. Might not cleanly apply since this is built ontop of a mostly mechanical changeset (s/NS_MUST_OVERRIDE/MOZ_MUST_OVERRIDE)

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Tests

Because everything needs tests, and static analyses especially so.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 721903 [details] [diff] [review]
Add a plugin

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

I /think/ this is looking pretty good. I would like Ted, Mike, or someone more familiar with the compiler make gunk to chime in here (something I haven't touched too much yet).

Also, how do you feel about splitting this up into multiple parts? I propose:

1) clang-plugin.cpp
2) Build system integration to build just the plugin
3) Build system changes to build with said plugin

We'll probably want to get a non-build peer to r+ the plugin or at least get a rubber stamp from a core Gecko person (maybe bsmedberg or khuey). What's the scope of our Clang plugin knowledge?

Some other things to consider:

* How do we test that the Clang plugin is compatible with the Clang we are using?
* How do we force the plugin to be compiled at the top of the build? I don't believe the current patches handle this. Is this a fatal error to Clang? Does it silently ignore the missing plugin until build/clang-plugin is built?

::: build/clang-plugin/Makefile.in
@@ +17,5 @@
> +# include the plugin we're building now. Freeze the HOST_CXXFLAGS now, before we
> +# start messing with CXXFLAGS, and then add in the other options we need for the
> +# plugin.
> +HOST_CXXFLAGS := $(HOST_CXXFLAGS)
> +HOST_CXXFLAGS += $(shell $(LLVMCONFIG) --cxxflags)

We should ideally perform the llvm-config call in configure.

@@ +21,5 @@
> +HOST_CXXFLAGS += $(shell $(LLVMCONFIG) --cxxflags)
> +ifdef GNU_CC
> +# -fno-rtti doesn't get added to llvm-config --cxxflags for some reason, so add
> +# it here to make it work.
> +HOST_CXXFLAGS += -fno-rtti

We should probably also do this in configure.

@@ +32,5 @@
> +HOST_DSO_LDOPTS := $(filter-out $(wlzdefs),$(DSO_LDOPTS))
> +
> +# This is the equivalent statement for OS X undefined lookup issues.
> +ifeq ($(HOST_OS_ARCH),Darwin)
> +HOST_DSO_LDOPTS += -dynamiclib -Wl,-undefined,dynamic_lookup

And quite possibly all of this as well.

::: build/clang-plugin/clang-plugin.cpp
@@ +1,1 @@
> +#include "clang/AST/ASTConsumer.h"

I didn't really look at this file.

::: build/moz.build
@@ +3,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +if CONFIG['ENABLE_CLANG_PLUGIN']:
> +  DIRS += ['clang-plugin']

I'm almost tempted to treat this directory as an external directory (EXTERNAL_DIRS) and not do moz.build foo. It seems like this directory is already somewhat special in that it must be built at the top of the build. Maybe once we have moz.build primitives for compiling it will be a different story...

::: config/config.mk
@@ +419,5 @@
>  endif # MOZ_OPTIMIZE
>  
>  ifdef CROSS_COMPILE
>  HOST_CFLAGS	+= $(HOST_OPTIMIZE_FLAGS)
> +HOST_CXXFLAGS += $(HOST_OPTIMIZE_FLAGS)

Surely there is a reason HOST_CXXFLAGS isn't here already. I'm not an expert on the compiler variable magic.

Ted, Mike?

::: config/rules.mk
@@ +925,5 @@
> +ifeq ($(HOST_CPP_PROG_LINK),1)
> +	$(HOST_CXX) $(HOST_DSO_LDOPTS) -o $@ $(HOST_CXXFLAGS) $(HOST_LDFLAGS) $(HOST_OBJS) $(HOST_LIBS) $(HOST_EXTRA_LIBS)
> +else
> +	$(HOST_CC) $(HOST_DSO_LDOPTS) -o $@ $(HOST_CFLAGS) $(HOST_LDFLAGS) $(HOST_OBJS) $(HOST_LIBS) $(HOST_EXTRA_LIBS)
> +endif

Do we really not have an equivalent rule already?!

::: mfbt/Attributes.h
@@ +368,5 @@
>   *   and the subclass does not provide an equivalent definition, the compiler
>   *   will emit an error.
>   */
>  #ifdef MOZ_CLANG_PLUGIN
> +#define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))

What actually changed here? Is this intentional?

Comment 5

[(no longer active)]

Feel free to ask me for review on the clang plugin itself.  I have some experience with clang plugins.

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #4)
> I /think/ this is looking pretty good. I would like Ted, Mike, or someone
> more familiar with the compiler make gunk to chime in here (something I
> haven't touched too much yet).

CC'ing them

> Also, how do you feel about splitting this up into multiple parts? I propose:
> 
> 1) clang-plugin.cpp
> 2) Build system integration to build just the plugin
> 3) Build system changes to build with said plugin
> 
> We'll probably want to get a non-build peer to r+ the plugin or at least get
> a rubber stamp from a core Gecko person (maybe bsmedberg or khuey). What's
> the scope of our Clang plugin knowledge?

I think I am the most knowledgeable on Clang plugins; I was planning on getting review from bsmedberg or taras for the actual semantic correctness of analysis.

> * How do we test that the Clang plugin is compatible with the Clang we are
> using?

I don't have a good answer, but we can use CLANG_VERSION* conditional-compilation macros here.

> * How do we force the plugin to be compiled at the top of the build? I don't
> believe the current patches handle this. Is this a fatal error to Clang?
> Does it silently ignore the missing plugin until build/clang-plugin is built?

I forced it by looking at the directory output and stuffing it before any other directory. It's kind of like stdc++compat in this regard (which also needs to go before every C++ library is loaded).

> We should probably also do this in configure.

I am not a big fan of doing things in configure since it's a $#@!$@#ing pain in the butt. Most of this logic was originally ported from DXR's Makefile logic, where it doesn't use a configure script.

> ::: config/config.mk
> @@ +419,5 @@
> >  endif # MOZ_OPTIMIZE
> >  
> >  ifdef CROSS_COMPILE
> >  HOST_CFLAGS	+= $(HOST_OPTIMIZE_FLAGS)
> > +HOST_CXXFLAGS += $(HOST_OPTIMIZE_FLAGS)
> 
> Surely there is a reason HOST_CXXFLAGS isn't here already. I'm not an expert
> on the compiler variable magic.

I suspect that I'm the first person to attempt to build a C++ library that should reside on the host in a cross-compiled setup.

> ::: mfbt/Attributes.h
> @@ +368,5 @@
> >   *   and the subclass does not provide an equivalent definition, the compiler
> >   *   will emit an error.
> >   */
> >  #ifdef MOZ_CLANG_PLUGIN
> > +#define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
> 
> What actually changed here? Is this intentional?

This applies on top of another patch which did some framework for enabling this analysis; since it wasn't interesting from a build perspective for feedback (it's rote NS_MUST_OVERRIDE->MOZ_MUST_OVERRIDE conversion), I didn't include it here.

> I'm almost tempted to treat this directory as an external directory
> (EXTERNAL_DIRS) and not do moz.build foo. It seems like this directory is
> already somewhat special in that it must be built at the top of the build.
> Maybe once we have moz.build primitives for compiling it will be a different
> story...

An alternative idea presented itself to me. Basically, from a build perspective, what I really need is "give me the original variables passed into configure with no other modifications." We don't have that right now--we pick up things like -fno-rtti, or -Wl,-z,nodefs which range from harmless to build-breaking (in particular, I had a problem running on tinderboxes since it caught itself trying to link stdc++compat when building the plugin).

One option is to completely thumb our nose at cross-compiling, which would require much fewer build-system changes (we'd use regular CXXFLAGS, etc.). Another option is to not try to build it with the m-c build system at all and fall-back on manual $(CXX) make rules and the like.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] from comment #6)
> > Surely there is a reason HOST_CXXFLAGS isn't here already. I'm not an expert
> > on the compiler variable magic.
> 
> I suspect that I'm the first person to attempt to build a C++ library that
> should reside on the host in a cross-compiled setup.

You're not, and our way to deal with host compiler flags is awful and broken.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Newer version of plugin integration

As discussed briefly yesterday, another idea for this patch is to screw trying to integrate with the mozilla build system for now and treat it as an external build system. This isn't the best long-term scenario, but I think it's probably better to defer this issue until we get a less-broken build system.

This patch gets it working for me locally; I can also make it kind of work on Try if I revert to an old version of the linux clang binaries, but it fails in the crash reporter with what appears to be a libstdc++-in-clang issue.

My basic plan for landing looks like as follows:
1. Add the clang plugin as a shell (i.e., no analyses implemented)
2. Port individual analyses from the old dehydra stuff over one-by-one
3. Kill the old dehydra analysis integration from our build system.
4. Coordinate with releng to get static-analysis checking working on TBPL.
5. Get it working on OS X.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 723107 [details] [diff] [review]
Newer version of plugin integration

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

Lookin' good!

Please ask Mike Hommey for final review because he has the best pair of eyes for reviewing configure goo.

::: build/clang-plugin/clang-plugin.cpp
@@ +1,1 @@
> +#include "clang/AST/ASTConsumer.h"

License needed.

@@ +106,5 @@
> +};
> +}
> +
> +static FrontendPluginRegistry::Add<MozCheckAction>
> +X("moz-check", "check moz action");

I didn't look at this file in any detail.

::: build/clang-plugin/configure
@@ +35,5 @@
> +  -e "s%@CXX@%$CXX%" \
> +  -e "s%@CXXFLAGS@%$CXXFLAGS%" \
> +  -e "s%@LDFLAGS@%$LDFLAGS%" \
> +  -e "s%@srcdir@%$srcdir%" \
> +  > Makefile

It's so hacky and simple - I love it!

::: configure.in
@@ +9342,5 @@
>  RC=
>  
> +if test -n "$ENABLE_CLANG_PLUGIN"; then
> +    ac_configure_args="$_SUBDIR_CONFIG_ARGS"
> +    AC_OUTPUT_SUBDIRS(build/clang-plugin)

Perhaps we could pass $LLVMCONFIG into the subconfigure? Does this get done automatically with MOZ_PATH_PROGS?

::: js/src/configure.in
@@ +3779,5 @@
> +    elif test ! -x "$LLVMCONFIG"; then
> +        AC_MSG_ERROR([Cannot execute $LLVMCONFIG])
> +    fi
> +    AC_DEFINE(MOZ_CLANG_PLUGIN)
> +fi

I wonder if it's worth moving this boilerplate into an m4 file?

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Add the plugin shell

This adds a plugin shell, complete with the test framework, but we still check nothing here.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Add must-override analysis

And this patch adds the NS_MUST_OVERRIDE analysis. Removing the corresponding analysis tests from xpcom/tests/static-checker wasn't strictly necessary, but it makes a simple way to mark how many more static analyses need to be ported.

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Move the annotations to MFBT instead of XPCOM.

nscore.h feels like a bad place for these kinds of annotations, so the location moves to a block in mfbt/Attributes.h. I'm also keeping the documentation of semantics for the macros in mfbt instead of the clang plugin, since this feels more natural.

Most of this patch is a mechanical porting of the NS_MUST_OVERRIDE to MOZ_MUST_OVERRIDE. There is one exception: I removed the annotation from GroupRule.h, since it appears that none of the subclasses override this.

I'm going to leave porting of the NS_STACK_CLASS and the static-initializer warnings to a later bug, as well as the total removal of the old Dehydra analysis.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 725692 [details] [diff] [review]
Part 1: Add the plugin shell

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

::: build/clang-plugin/Makefile.in
@@ +21,5 @@
> +TESTS := $(patsubst %.cpp,test-%,$(TESTSRCS))
> +
> +all: libclang-plugin.so $(TESTS)
> +
> +%.o: %.cpp Makefile

$(OBJS): %.o: %.cpp Makefile

@@ +22,5 @@
> +
> +all: libclang-plugin.so $(TESTS)
> +
> +%.o: %.cpp Makefile
> +	$(CXX) -o $@ -c $(CXXFLAGS) $(abspath $<)

I'm not sure $(abspath) makes much sense here, since $< is already going to do some magic with the VPATH.

@@ +24,5 @@
> +
> +%.o: %.cpp Makefile
> +	$(CXX) -o $@ -c $(CXXFLAGS) $(abspath $<)
> +
> +libclang-plugin.so: $(OBJS)

Can you use a variable for libclang-plugin.so?

@@ +29,5 @@
> +	rm -f $@
> +	$(CXX) -shared -o $@ $(CXXFLAGS) $(LDFLAGS) $(OBJS)
> +
> +TESTFLAGS := -fsyntax-only -Xclang -verify \
> +	-Xclang -load -Xclang $(shell pwd)/libclang-plugin.so \

$(CURDIR) instead of $(shell pwd)

@@ +32,5 @@
> +TESTFLAGS := -fsyntax-only -Xclang -verify \
> +	-Xclang -load -Xclang $(shell pwd)/libclang-plugin.so \
> +	-Xclang -add-plugin -Xclang moz-check
> +
> +test-%: tests/%.cpp libclang-plugin.so

$(TESTS): ...

::: configure.in
@@ +7434,5 @@
> +if test -n "$ENABLE_CLANG_PLUGIN"; then
> +    if test -z "$CLANG_CC"; then
> +        AC_MSG_ERROR([Can't use clang plugin without clang.])
> +    fi
> +    AC_DEFINE(MOZ_CLANG_PLUGIN)

You're not using the AC_DEFINE.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Mike Hommey [:glandium] from comment #13)
> ::: configure.in
> @@ +7434,5 @@
> > +if test -n "$ENABLE_CLANG_PLUGIN"; then
> > +    if test -z "$CLANG_CC"; then
> > +        AC_MSG_ERROR([Can't use clang plugin without clang.])
> > +    fi
> > +    AC_DEFINE(MOZ_CLANG_PLUGIN)
> 
> You're not using the AC_DEFINE.

It's unused in this patch, but it gets used a little later in part 3.

Comment 15

[(no longer active)]

Comment on attachment 725693 [details] [diff] [review]
Part 2: Add must-override analysis

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

::: build/clang-plugin/clang-plugin.cpp
@@ +45,5 @@
> +      // The base is either a class (CXXRecordDecl) or it's a templated class...
> +      CXXRecordDecl *parent = base->getType()
> +        .getDesugaredType(d->getASTContext())->getAsCXXRecordDecl();
> +      // XXX: !!!!
> +      if (!parent) {

???

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 725692 [details] [diff] [review]
Part 1: Add the plugin shell

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

I think Mike Hommey's review is sufficient.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 725694 [details] [diff] [review]
Part 3: Move the annotations to MFBT instead of XPCOM.

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

::: mfbt/Attributes.h
@@ +321,5 @@
>  
> +/*
> + * The following macros are attributes that support the static analysis plugin
> + * included with Mozilla, and are implemented (when static analysis is enabled)
> + * as C++11 attributes. Since such attributes are legal pretty much everywhere

...except they're not, right now.  Have you made them C++11 attributes with any compiler (even clang bleeding-edge-not-released-yet-fresh-from-the-oven) to be certain they're correctly positioned?  I know I read the relevant bits of C++11 attribute details, and I got very confused by all of it.  :-(  My memory had been that [[]] applied to the thing to the right, not the left, but I wouldn't trust that reading (or memory!) very far at all.  (And on a second look, maybe I was totally misreading anyway.)

Given you're changing every use's location, I tend to think you should have actual working use of C++11 attributes before you land this, at least as concerns the function-placement language  Ideally, even, you'd have it enabled for clang-super-bleeding-edge, in the patch, so others tracking clang tip closely can verify.  But it's not strictly a requirement to land.

@@ +323,5 @@
> + * The following macros are attributes that support the static analysis plugin
> + * included with Mozilla, and are implemented (when static analysis is enabled)
> + * as C++11 attributes. Since such attributes are legal pretty much everywhere
> + * and have subtly different semantics depending on their placement, the
> + * following is a guide on where to place the attributes.

That said, this guide (assuming it's correct) is totally awesome, and this is totally the right place to have it.

@@ +333,5 @@
> + *
> + * Attributes that apply to functions immediately follow the parentheses:
> + *
> + *   void DeclaredFunction() MOZ_FUNCTION_ATTRIBUTE;
> + *   void SomeFunction() MOZ_FUNCTION_ATTRIBUTE {}

Include some examples with const and = 0 here, please, and adjust the wording as appropriate.  (Note that MOZ_OVERRIDE and MOZ_FINAL clarify this explicitly, with examples.)

@@ +352,5 @@
> + *   MOZ_IF_ATTRIBUTE if (x == 0)
> + *   MOZ_DO_ATTRIBUTE do { } while(0);
> + *
> + * Attributes that apply to labels precede the label:
> + * 

A couple trailing whitespaces in this file need removing.

@@ +361,5 @@
> + *
> + * The static analyses that are performed by the plugin are as follows:
> + *
> + * MOZ_MUST_OVERRIDE: Applies to all C++ member functions. All immediate
> + *   subclasses must provide an override of this method; if a subclass does not

Must it be an *exact* override?  I'd hope that's how the semantics of this would work, but please be clear.

@@ +370,5 @@
> + */
> +#ifdef MOZ_CLANG_PLUGIN
> +#define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
> +#else
> +#define MOZ_MUST_OVERRIDE

#if ...
#  define ...
#else
#  define ...
#endif

Also put a /* nothing */ make more readable that a macro's being defined to nothing.

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> > + * The following macros are attributes that support the static analysis plugin
> > + * included with Mozilla, and are implemented (when static analysis is enabled)
> > + * as C++11 attributes. Since such attributes are legal pretty much everywhere
> 
> ...except they're not, right now.  Have you made them C++11 attributes with
> any compiler (even clang bleeding-edge-not-released-yet-fresh-from-the-oven)
> to be certain they're correctly positioned?  I know I read the relevant bits
> of C++11 attribute details, and I got very confused by all of it.  :-(  My
> memory had been that [[]] applied to the thing to the right, not the left,
> but I wouldn't trust that reading (or memory!) very far at all.  (And on a
> second look, maybe I was totally misreading anyway.)

When I originally wrote this patch, I used [[mozilla::must_override]]. This did require a patched version of clang to allow custom C++11 attributes, so I eventually replaced it with the current setup when it was clear that the patches weren't going to be accepted anytime soon.

Rereading the C++11 spec, which only vaguely describes how attributes bind, the basic rule of thumb appears to be that attributes at the beginning of the declaration apply to all declarators, and attributes bind to the token just to the left, be that a declarator id, type, etc.

> @@ +333,5 @@
> > + *
> > + * Attributes that apply to functions immediately follow the parentheses:
> > + *
> > + *   void DeclaredFunction() MOZ_FUNCTION_ATTRIBUTE;
> > + *   void SomeFunction() MOZ_FUNCTION_ATTRIBUTE {}
> 
> Include some examples with const and = 0 here, please, and adjust the
> wording as appropriate.  (Note that MOZ_OVERRIDE and MOZ_FINAL clarify this
> explicitly, with examples.)

I'm not 100% sure where the correct place is here with cv-qualified (this is one of the things that got editorialized between the last public draft and the actual spec, it appears), but it looks like the answer is "(almost) exactly where OVERRIDE and FINAL are supposed to go" (elaborated return types are sucky...).

> @@ +361,5 @@
> > + *
> > + * The static analyses that are performed by the plugin are as follows:
> > + *
> > + * MOZ_MUST_OVERRIDE: Applies to all C++ member functions. All immediate
> > + *   subclasses must provide an override of this method; if a subclass does not
> 
> Must it be an *exact* override?  I'd hope that's how the semantics of this
> would work, but please be clear.

The intent of "override" here is to equivalent to "override" in the sense of the C++11 attribute, except that the definition is meant to apply to both nonvirtual and virtual methods. The actual semantics the checker uses is "a method of the same name that would not overload", which is apparently how clang itself computes overriding.

Comment 19

[Joshua Cranmer [:jcranmer]]

Comment on attachment 725693 [details] [diff] [review]
Part 2: Add must-override analysis

Per discussion over IRC, bsmedberg said that ehsan's review was sufficient given also the presence of static checking tests.

Comment 20

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf75954e488f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfc8698edd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7684259549

Comment 21

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/cf75954e488f
https://hg.mozilla.org/mozilla-central/rev/9dfc8698edd0
https://hg.mozilla.org/mozilla-central/rev/4d7684259549