851753 - Deploy clang static analysis builders

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Joshua Cranmer [:jcranmer]]

Once bug 767563 lands, we need a builder to ensure that people don't add new static analysis violations. This will probably require modifications to the scripts we use to build clang.

Basically, we need a builder that builds the codebase on Linux32 or Linux64 (I recommend Linux64 debug) with --enable-clang-plugin and clang as its compiler but is otherwise the same as a regular build. Ideally this would run on every project that feeds into mozilla-central (e.g., try, mozilla-inbound, build-system, etc.). We don't need to run any tests on the built builds, just the building--the static checker just adds extra warnings/errors to a normal compile.

An extra bonus would be if we added a feature to track the warning count.

Comment 2

[Christian Holler (:decoder)]

How do you want to find out if people add new warnings? Just going by the count? If so, I'd also suggest to differentiate between the different types of static analysis warnings that Clang supports. 

Also, which of these types do you want to check, only your own?

I think it's a great idea (and we've been wanting something like this for a long time). That said, it's unlikely you can get the builders you requested. Already getting ASan on Linux64 debug/opt *only* for mozilla-central wasn't easy. I suggest you aim for inbound then.

Comment 3

[Gregory Szorc [:gps]]

Baby steps. Let's just get a minimal plugin / buildbot job deployed and we can worry about things like warnings count later.

I'm pretty sure technical leaders in Gecko land will happily champion this bug. After all, static analysis should lead to higher code quality and fewer bugs. If we integrate it into our development workflow, it should also make reviewing easier since reviewers won't need to be burdened by things the static analyzer will flag. It's a no-brainer to set up this job if you ask me.

Comment 4

[Christian Holler (:decoder)]

(In reply to Gregory Szorc [:gps] from comment #3)
> Baby steps. Let's just get a minimal plugin / buildbot job deployed and we
> can worry about things like warnings count later.
> [...] It's a no-brainer to set up this job if you ask me.

It's not only about brain but also about resources. It's not that easy to get additional build jobs as they are tied to resources that are currently at their limits already. It already required a significant effort to get ASan a build job, and only after we already had our "manual" build jobs for months and their value was clear by the reports that came in.

I suggest you start with getting this to work using a try push (which is required anyway to find out if it works), and then we see how a releng build job can help do better.

In general I agree with you regarding code quality and fewer bugs, but we already worked with Clang's static analysis and in practice, most of it isn't very helpful if you don't have a mechanism to track all the existing issues (which are a lot), that won't be fixed. And in the case of Clang's static analysis, not even all reported bugs are actually bugs which increases the number of reports that won't get fixes.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Christian Holler (:decoder) from comment #4)
> In general I agree with you regarding code quality and fewer bugs, but we
> already worked with Clang's static analysis and in practice, most of it
> isn't very helpful if you don't have a mechanism to track all the existing
> issues (which are a lot), that won't be fixed. And in the case of Clang's
> static analysis, not even all reported bugs are actually bugs which
> increases the number of reports that won't get fixes.

Oh, I think we're all a little mixed up. I /think/ this bug tracks getting builds with Mozilla's Clang plugin (from bug 767563) not Clang's built-in static analyzer. Although, I suppose it could be both.

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Oh, I think we're all a little mixed up. I /think/ this bug tracks getting
> builds with Mozilla's Clang plugin (from bug 767563) not Clang's built-in
> static analyzer. Although, I suppose it could be both.

This bug is specifically for the clang plugin, not clang static analysis tracking.

Comment 7

[Christian Holler (:decoder)]

Okay, so what stage is this in now? Is it already possible to push to try with this clang plugin?

Comment 8

[Joshua Cranmer [:jcranmer]]

(In reply to Christian Holler (:decoder) from comment #7)
> Okay, so what stage is this in now? Is it already possible to push to try
> with this clang plugin?

If you add the patches in the blocking bug, change the builds of clang you use, and run --disable-crashreporter (since clang-on-linux currently fails due to bad libstdc++ headers), you should be able to get results on try.

Comment 9

[Christian Holler (:decoder)]

(In reply to Joshua Cranmer [:jcranmer] from comment #8)
> If you add the patches in the blocking bug, change the builds of clang you
> use, and run --disable-crashreporter (since clang-on-linux currently fails
> due to bad libstdc++ headers), you should be able to get results on try.

That sounds like try doesn't have the right builds yet? In that case, that should be the first goal (and it's a requirement anyway before requesting build jobs). From that, we can move on to the next step :)

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to Christian Holler (:decoder) from comment #9)
> That sounds like try doesn't have the right builds yet? In that case, that
> should be the first goal (and it's a requirement anyway before requesting
> build jobs). From that, we can move on to the next step :)

Bug 852745 would create a version of clang that has the right features we need for this bug.

Comment 11

[Chris Cooper [:coop] (he/him)]

Sounds like we're ready to have someone in releng add this build type.

Can I ask someone (jcranmer?) to provide a new mozconfig for this that includes all the relevant compile flags? You can use https://hg.mozilla.org/mozilla-central/file/c232bec6974d/browser/config/mozconfigs/linux64/debug-asan as a template.

Comment 12

[Joshua Cranmer [:jcranmer]]

The following should work:

. "$topsrcdir/build/mozconfig.common"

# Use Clang as specified in manifest
export CC="$topsrcdir/clang/bin/clang"
export CXX="$topsrcdir/clang/bin/clang++"

# Add the static checker
ac_add_options --enable-clang-plugin

Comment 13

[Chris Cooper [:coop] (he/him)]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> The following should work:
> 
> . "$topsrcdir/build/mozconfig.common"
> 
> # Use Clang as specified in manifest
> export CC="$topsrcdir/clang/bin/clang"
> export CXX="$topsrcdir/clang/bin/clang++"
> 
> # Add the static checker
> ac_add_options --enable-clang-plugin

Did you want ac_add_options --enable-debug in there also (based on comment #0)? 

Or, more broadly, if you're meaning to test something closer to what we ship, have you consciously chosen to *not* include the other options/exports as used in one of the 2 standard mozconfigs?

https://hg.mozilla.org/mozilla-central/file/2aff2d574a1e/browser/config/mozconfigs/linux64/nightly
https://hg.mozilla.org/mozilla-central/file/2aff2d574a1e/browser/config/mozconfigs/linux64/debug

I'm happy to land the above on m-c (or act as an r+ for same) if that's what you want. Fine to land with DONTBUILD. The file should probably be named debug-static-analysis-clang or similar.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Chris Cooper [:coop] from comment #13)
> (In reply to Joshua Cranmer [:jcranmer] from comment #12)
> > The following should work:
> > 
> > . "$topsrcdir/build/mozconfig.common"
> > 
> > # Use Clang as specified in manifest
> > export CC="$topsrcdir/clang/bin/clang"
> > export CXX="$topsrcdir/clang/bin/clang++"
> > 
> > # Add the static checker
> > ac_add_options --enable-clang-plugin
> 
> Did you want ac_add_options --enable-debug in there also (based on comment
> #0)?

I based this off of https://hg.mozilla.org/mozilla-central/file/e818c908825a/build/unix/mozconfig.asan, not the files in browser/config/mozconfigs/linux64/*.

> Or, more broadly, if you're meaning to test something closer to what we
> ship, have you consciously chosen to *not* include the other options/exports
> as used in one of the 2 standard mozconfigs?
> 
> https://hg.mozilla.org/mozilla-central/file/2aff2d574a1e/browser/config/
> mozconfigs/linux64/nightly
> https://hg.mozilla.org/mozilla-central/file/2aff2d574a1e/browser/config/
> mozconfigs/linux64/debug

Most of the other options seem "releasy" to me; judging from the -debug-asan file, I'd also need:
ac_add_options --enable-debug

# Avoid dependency on libstdc++ 4.5
ac_add_options --enable-stdcxx-compat

# Package js shell.
export MOZ_PACKAGE_JSSHELL=1

> I'm happy to land the above on m-c (or act as an r+ for same) if that's what
> you want. Fine to land with DONTBUILD. The file should probably be named
> debug-static-analysis-clang or similar.

I'd be happy if you checked it in, since I'm unsure of the process for adding this kind of stuff.

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to Joshua Cranmer [:jcranmer] from comment #14)
> # Avoid dependency on libstdc++ 4.5
> ac_add_options --enable-stdcxx-compat

You don't need this.

> # Package js shell.
> export MOZ_PACKAGE_JSSHELL=1

You don't need this.

Comment 16

[Chris Cooper [:coop] (he/him)]

https://hg.mozilla.org/mozilla-central/rev/6d1f6c17edf2

I'll try to dig in to adding the new build type this week during buildduty.

Comment 17

[Chris Cooper [:coop] (he/him)]

Attachment: Add a static analysis clang builder for Linux64 on m-c only

catlee: I think you went through this most recently with the ASAN builds, so pinging you for review.

Of note, is there follow-up work to get static analysis builds to show up in TBPL? I don't see the ASAN builds there, so I'm lacking a template if we want them displayed there.

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Chris Cooper [:coop] from comment #17)
> Of note, is there follow-up work to get static analysis builds to show up in
> TBPL? I don't see the ASAN builds there, so I'm lacking a template if we
> want them displayed there.

They appear to show up if you don't hide ignored (https://tbpl.mozilla.org/?showall=1)...

CC'ing edmorley.

Comment 19

[Phil Ringnalda (:philor)]

https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#3.29_Runs_on_all_trees_that_merge_into_mozilla-central is the template for "I want to run it on mozilla-central only," - it'll be visible to interested parties at https://tbpl.mozilla.org/?showall=1&jobname=static%20analysis until tbpl2 brings us rock candy mountains and lemonade springs and a way to have "visible" things that can be busted without backing out over it. For now, visible by default === tier 1 you must back out for bustage, and tier 1 === must run on all trunk trees.

Comment 20

[Chris Cooper [:coop] (he/him)]

(In reply to Phil Ringnalda (:philor) from comment #19)
> https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#3.
> 29_Runs_on_all_trees_that_merge_into_mozilla-central is the template for "I
> want to run it on mozilla-central only," - it'll be visible to interested
> parties at https://tbpl.mozilla.org/?showall=1&jobname=static%20analysis
> until tbpl2 brings us rock candy mountains and lemonade springs and a way to
> have "visible" things that can be busted without backing out over it. For
> now, visible by default === tier 1 you must back out for bustage, and tier 1
> === must run on all trunk trees.

Fair enough, although I do believe jcranmer wants this on all derivative trees eventually. Whether static analysis is something we're hoping to promote to tier 1, I can't speak to.

Comment 21

[Joshua Cranmer [:jcranmer]]

(In reply to Chris Cooper [:coop] from comment #20)
> Fair enough, although I do believe jcranmer wants this on all derivative
> trees eventually. Whether static analysis is something we're hoping to
> promote to tier 1, I can't speak to.

I do want to run this on all the trees that merge to mozilla-central eventually.

Comment 22

[(no longer active)]

(In reply to comment #21)
> (In reply to Chris Cooper [:coop] from comment #20)
> > Fair enough, although I do believe jcranmer wants this on all derivative
> > trees eventually. Whether static analysis is something we're hoping to
> > promote to tier 1, I can't speak to.
> 
> I do want to run this on all the trees that merge to mozilla-central
> eventually.

In fact, ideally we should do these builds by default on Mac, once the build system stuff on Mac is figured out, of course.

Comment 23

[Ed Morley [:emorley]]

Adding to comment 19, ASan builds don't yet meet the requirements for unhiding - last I heard they were too resource intensive to run on every tree on every push.

Comment 24

[Christian Holler (:decoder)]

(In reply to Ed Morley [:edmorley UTC+1] from comment #23)
> Adding to comment 19, ASan builds don't yet meet the requirements for
> unhiding - last I heard they were too resource intensive to run on every
> tree on every push.

It's not that ASan is too resource intensive, no. We have been told that in general, we do not have resources for adding more builds to other trees. If we can add clang static analysis to inbound and other trees, then ASan should get added as well.

Comment 25

[Chris Cooper [:coop] (he/him)]

(In reply to Christian Holler (:decoder) from comment #24)
> It's not that ASan is too resource intensive, no. We have been told that in
> general, we do not have resources for adding more builds to other trees. If
> we can add clang static analysis to inbound and other trees, then ASan
> should get added as well.

Not to derail this bug with further discussion of ASan builds, but ASan builds are an order of magnitude larger on disk than regular builds, not ~2x like debug builds. Storing lots of ASan builds for any length of time become prohibitive quickly.

Comment 26

[Chris Cooper [:coop] (he/him)]

The compile step is failing, but I haven't had a chance to dig much deeper. Here's a log if you want to have a look:

https://people.mozilla.com/~coop/bug851753/compile_v1.log

Comment 27

[Joshua Cranmer [:jcranmer]]

The --enable-stdcxx-compat may be needed after all.

Comment 28

[Chris Cooper [:coop] (he/him)]

Attachment: Set --enable-stdcxx-compat

(In reply to Joshua Cranmer [:jcranmer] from comment #27)
> The --enable-stdcxx-compat may be needed after all.

The build works with --enable-stdcxx-compat set. I've put the artifacts on people if you want to examine them before I land this:

https://people.mozilla.com/~coop/bug851753/

The build specifically:

https://people.mozilla.com/~coop/bug851753/firefox-23.0a1.en-US.linux-x86_64.tar.bz2

Comment 29

[Chris Cooper [:coop] (he/him)]

Comment on attachment 743615 [details] [diff] [review]
Set --enable-stdcxx-compat

https://tbpl.mozilla.org/?rev=e113285ef790

Comment 30

[Chris Cooper [:coop] (he/him)]

A question about how these builds will be used: will you require the corresponding tests.zip for each build? 

If we can turn those off, we'll only need 1/3 as much storage space per build.

Comment 31

[Joshua Cranmer [:jcranmer]]

The intent here is to build but run no tests, so the tests.zip should be unnecessary.

Comment 32

[Chris Cooper [:coop] (he/him)]

Comment on attachment 738217 [details] [diff] [review]
Add a static analysis clang builder for Linux64 on m-c only

https://hg.mozilla.org/build/buildbot-configs/rev/3a405fe9f6e8

Comment 33

[Chris Cooper [:coop] (he/him)]

Once this gets reconfig-ed into production, you'll be able to find builds here:

http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-dbg-st-an/

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 743615 [details] [diff] [review]
Set --enable-stdcxx-compat

Backed out and relanded as

https://hg.mozilla.org/mozilla-central/rev/c0e81c0222fc

because the bug number was wrong.

Comment 35

[Chris Cooper [:coop] (he/him)]

(In reply to :Ms2ger from comment #34)
> Backed out and relanded as
> 
> https://hg.mozilla.org/mozilla-central/rev/c0e81c0222fc
> 
> because the bug number was wrong.

My fat fingers thank you for this.

Comment 36

[Rail Aliiev [:rail]]

in production

Comment 37

[Ed Morley [:emorley]]

Dep build + nightly hidden on m-c since busted:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22540437&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=22546101&tree=Mozilla-Central

(They also do not yet meet https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy)

View with:
https://tbpl.mozilla.org/?showall=1&jobname=static

Comment 38

[Chris Cooper [:coop] (he/him)]

jcranmer: do you want to leave this bug open until you green up the build, or file a new bug to enable these builds on all project branches once you do?

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to Chris Cooper [:coop] from comment #38)
> jcranmer: do you want to leave this bug open until you green up the build,
> or file a new bug to enable these builds on all project branches once you do?

Bug 868285 has already been filed on the bustage. Once it goes green, I'll file a bug to enable on all branches.