Closed
Bug 851753
Opened 12 years ago
Closed 12 years ago
Deploy clang static analysis builders
Categories
(Release Engineering :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jcranmer, Assigned: coop)
References
Details
Attachments
(2 files)
7.35 KB,
patch
|
catlee
:
review+
coop
:
checked-in+
|
Details | Diff | Splinter Review |
639 bytes,
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.
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
(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.
Reporter | ||
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
Okay, so what stage is this in now? Is it already possible to push to try with this clang plugin?
Reporter | ||
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
(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 :)
Reporter | ||
Comment 10•12 years ago
|
||
(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
Assignee | ||
Comment 11•12 years ago
|
||
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
Reporter | ||
Comment 12•12 years ago
|
||
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
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Reporter | ||
Comment 14•12 years ago
|
||
(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•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
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
Assignee | ||
Comment 17•12 years ago
|
||
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)
Reporter | ||
Comment 18•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 20•12 years ago
|
||
(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.
Reporter | ||
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #738217 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 25•12 years ago
|
||
(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.
Assignee | ||
Comment 26•12 years ago
|
||
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
Reporter | ||
Comment 27•12 years ago
|
||
The --enable-stdcxx-compat may be needed after all.
Assignee | ||
Comment 28•12 years ago
|
||
(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)
Reporter | ||
Updated•12 years ago
|
Attachment #743615 -
Attachment is patch: true
Attachment #743615 -
Attachment mime type: text/x-patch → text/plain
Reporter | ||
Updated•12 years ago
|
Attachment #743615 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Comment 29•12 years ago
|
||
Comment on attachment 743615 [details] [diff] [review] Set --enable-stdcxx-compat https://tbpl.mozilla.org/?rev=e113285ef790
Attachment #743615 -
Flags: checked-in+
Assignee | ||
Comment 30•12 years ago
|
||
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.
Reporter | ||
Comment 31•12 years ago
|
||
The intent here is to build but run no tests, so the tests.zip should be unnecessary.
Assignee | ||
Comment 32•12 years ago
|
||
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+
Assignee | ||
Comment 33•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 35•12 years ago
|
||
(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•12 years ago
|
||
in production
Comment 37•12 years ago
|
||
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
Assignee | ||
Comment 38•12 years ago
|
||
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?
Reporter | ||
Comment 39•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•6 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•