Closed Bug 851753 Opened 12 years ago Closed 11 years ago

Deploy clang static analysis builders

Categories

(Release Engineering :: General, defect, P2)

Product:
Release Engineering
Component:
General
Platform:
x86_64
Linux
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jcranmer, Assigned: coop)

References

Details

Attachments

(2 files)

Add a static analysis clang builder for Linux64 on m-c only
(deleted), patch
catlee
: review+
coop
: checked-in+
Details | Diff | Splinter Review
Set --enable-stdcxx-compat
(deleted), patch
jcranmer
: review+
coop
: checked-in+
Details | Diff | Splinter Review 
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.
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.
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.
OS: Windows 7 → Linux
(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.
(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.
(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.
Okay, so what stage is this in now? Is it already possible to push to try with this clang plugin?
(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.
(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 :)
(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.
Depends on: 852745
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.
Component: Release Engineering: Platform Support → Release Engineering: Automation (General)
QA Contact: coop → catlee
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
(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.
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/6d1f6c17edf2

I'll try to dig in to adding the new build type this week during buildduty.
Assignee: nobody → coop
Status: NEW → ASSIGNED
Priority: -- → P2
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.
Attachment #738217 - Flags: review?(catlee)
(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.
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.
(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.
(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 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.
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.
(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.
Attachment #738217 - Flags: review?(catlee) → review+
(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.
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
The --enable-stdcxx-compat may be needed after all.
Attached patch Set --enable-stdcxx-compat (deleted) — — Splinter Review 
(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
Attachment #743615 - Flags: review?(Pidgeot18)
Attachment #743615 - Attachment is patch: true
Attachment #743615 - Attachment mime type: text/x-patch → text/plain
Attachment #743615 - Flags: review?(Pidgeot18) → review+
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.
The intent here is to build but run no tests, so the tests.zip should be unnecessary.
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
Attachment #738217 - Flags: checked-in+
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 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.
(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.
in production
Depends on: 868289
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?
(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 880434
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: