Closed Bug 703121 Opened 10 years ago Closed 9 years ago
Turn on WARNINGS
_AS _ERRORS for TBPL builds
2.39 KB, patch
|Details | Diff | Splinter Review|
2.17 KB, patch
|Details | Diff | Splinter Review|
10.83 KB, patch
|Details | Diff | Splinter Review|
Browser builds cause *tons* of compiler warnings. So many that it's really easy to introduce new ones because they're hard to see among the old ones. The starting point for this bug is the assumption that there is general agreement that zero warnings is a worthwhile goal -- yes, sometimes warnings are bogus and you have to jump through some tiny hoops to fix them (e.g. initialize a variable) but they catch real bugs often enough that it's worth it. I'll use the existence of bug 187528 as evidence of this general agreement. I'll also use the existence of bug 187528 as evidence that a non-automated approaches won't suffice -- that bug is 8 years old and still not even close to finished. It'll never be finished if we don't build with WARNINGS_AS_ERRORS. The key thing here is that we should *only* build with WARNINGS_AS_ERRORS on TBPL builds. So the goal is to guarantee no compiler warnings on the TBPL configurations, which are the most important ones. It shouldn't be the default for local builds, because then you get frequent breakage for people building with uncommon compilers on uncommon platforms. (And who cares if the Sun compiler warns about something if GCC and MSVC don't? You have to draw the line somewhere.) The inevitable complaint about this idea is that it's easy to introduce warnings on platforms that you're not using yourself. Which is true, but that's what the try server is for -- you are sending your patches to try server before landing them, right? Besides, it's already possible to introduce compile *errors* that only show up on one platform, so this problem already exists and people can deal with it. My final argument to the naysayers: this process is already in place for the JS engine and has been working just fine for months. (It's slightly different in that WARNINGS_AS_ERRORS is only on for standalone JS engine builds, not for whole-browser builds, but that's just a technical detail.) At one point I tried turning it on for local JS engine builds too, but that's a bad idea because of the above-mentioned problems with strange platforms and compilers. Turning it on only for TBPL builds is the right compromise. Now, obviously we can't do this immediately, bug 187528 has to be resolved. It might even make sense to turn WARNINGS_AS_ERRORS on one platform at a time. But hopefully this bug will provide extra motivation for people to work on bug 187528, because once we get to zero warnings we'll have automation to ensure it stays at zero, rather than relying on a manual whack-a-mole game.
Component: Tinderboxpushlog → Release Engineering
Product: Webtools → mozilla.org
QA Contact: tinderboxpushlog → release
The relevant configs live in Hg now (e.g. browser/config/mozconfigs), so this would just be a matter of changing those.
For reference, see bug 557566 comment 28 where I tried to do a version of this about a year ago (without the "only on tbpl" restriction), and the subsequent comments where I ended up partially disabling it and then backing it out, mainly due to warnings in not-on-tbpl compilers/compiler-versions. I'd support making this opt-in and enabling it on TBPL, as njn suggests. One potential problem would be for patches that aren't tryserver-tested for whatever reason (and there are legit reasons for not using Try; e.g. followup-to-fix-an-orange, certain security fixes, etc.), but hopefully any issues from those would be rare and would generally occur on project branches like mozilla-inbound rather than on mozilla-central.
Bug 646702 was the one where this was turned on for SpiderMonkey, BTW. I'd be very happy if this was enabled on a per-module or per-directory basis. It's certainly easier to fix all the warnings in one module at a time, and it would show sceptics that enabling it won't cause the sky to fall in. > One potential problem would be for patches that aren't tryserver-tested for > whatever reason (and there are legit reasons for not using Try; e.g. > followup-to-fix-an-orange, certain security fixes, etc.), but hopefully any > issues from those would be rare and would generally occur on project > branches like mozilla-inbound rather than on mozilla-central. Yes. This is just an instance of a general problem, which is that basically any push can cause breakage if someone screws up. That's why mozilla-inbound exists!
(In reply to Nicholas Nethercote [:njn] from comment #0) > yes, sometimes warnings > are bogus and you have to jump through some tiny hoops to fix them (e.g. > initialize a variable) I hope you are not advocating initializing variables to bogus values to suppress gcc's "may be used uninitialized" warnings. Initializing such a variable with junk will suppress reporting of possible future errors from tools that are smart enough to tell when a variable is really used uninitialized. gcc is honest about the fact that it is not always smart enough about presenting these warnings, and makes the warning optional for that reason. Unfortunately gcc doesn't give us fine grain control to choose between "may be" and "is" warnings, so we'll have to use -Wno-error=uninitialized on these builds.
(In reply to Karl Tomlinson (:karlt) from comment #4) > > I hope you are not advocating initializing variables to bogus values to > suppress gcc's "may be used uninitialized" warnings. I am. It's a common practice. I've done it myself numerous times in Mozilla code for this exact purpose.
If it's a common practice, that's very unfortunate, but not a reason to advocate it.
For MSVC and newer versions of GCC, we can selectively disable warnings for chunks of code, using something like the commented-out #pragma here: http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGPathDataParser.cpp#993 As long as TBPL runs new enough versions of GCC (I'm not sure what version is reqiured), that should work.
(Ah, looks like the "selectively enabling and disabling warnings via #pragma GCC diagnostic" feature was added in GCC 4.6, as noted at http://gcc.gnu.org/gcc-4.6/changes.html . So once our builders are running GCC 4.6 or greater, we'll have the ability to selectively warnings that we know are ignorable. That GCC version may never be available on mac, but that's not a huge deal, because we don't necessarily need to opt-in our mac builders to building with warnings.)
If the warnings (errors, actually) on the TBPL builds don't match the warnings you get with a local build, then this bug won't serve it's purpose -- I want to see no warnings on my machine when I build the browser. I'm surprised by this argument, actually. With any imperfect automated checking I can see three sensible scenarios: (1) turn it off (2) turn it on, and work around the false positives after confirming they are false (3) turn it on, and put up with the false positives You seem to be advocating (3) for non-TBPL builds is that right?
Unfortunately this is a compiler issue that we can't work around without writing a filter (or paying a price in the code). We want to see the "is" uninitialized warnings but not the "may be" warnings that don't mean anything. (3) seemed the best compromise to me. There is already plenty of make spew to ignore. I agree in principle that warnings that are not meaningless should be errors.
Let's discuss Windows. Windows builds are used by an overwhelming majority of the users, so the data says we should care more about MSVC than GCC or any other compiler. Fortunately, MSVC has a #pragma to selectively enable and disable warnings: http://msdn.microsoft.com/en-us/library/2c8f766e%28v=VS.100%29.aspx. And, it's been around since at least VS2003. It looks like GCC borrowed the syntax. As far as the number of warnings go, I have a personal Jenkins install continuously building m-c with PyMake on Windows 7 using a vanilla .mozconfig. The "warnings" Jenkins plugin nicely parses compiler warnings into a dashboard viewable through the Jenkins web interface. It is reporting 1481 *unique* warnings. The breakdown by warning type is as follows: C4003 1 C4005 18 C4018 138 C4047 1 C4065 1 C4067 1 C4083 1 C4090 5 C4101 19 C4133 11 C4146 27 C4150 4 C4244 1009 C4273 3 C4275 2 C4305 36 C4309 2 C4334 2 C4345 1 C4355 44 C4373 2 C4390 5 C4482 2 C4506 2 C4509 69 C4522 1 C4530 2 C4533 1 C4554 10 C4799 3 C4804 2 C4805 14 C4806 2 C4995 6 C4996 29 LNK4098 1 LNK4221 2 Msg 2 You can reference the warning codes at http://msdn.microsoft.com/en-us/library/8x5x43k7.aspx 1481 is a lot of warnings to fix. Unfortunately, my Jenkins install is on my personal network. If people are interested, I can see about exporting the warnings data. Or, I could easily set up a similar builder on a publicly available host.
If you use pragmas to get us to warnings-as-errors, then it's important to scope the pragmas as tightly as possible. There are some files with |#pragma disable warnings foo bar baz| at the top. Who knows what legitimate warnings they're hiding, or if those pragmas are even needed anymore.
(In reply to Karl Tomlinson (:karlt) from comment #10) > There is already plenty of make spew to ignore. Which is a huge part of the problem -- it's too easy to miss new ones. But if both GCC and MSVC allow selective disablement of warnings, we can use that and both of us will be happy. (In reply to Gregory Szorc [:gps] from comment #11) > Let's discuss Windows. Windows builds are used by an overwhelming majority > of the users, so the data says we should care more about MSVC than GCC or > any other compiler. Most Mozilla code is cross-platform, in which case we should care about whichever compiler gives the best warnings. But I suspect everyone who agrees with the goal of this bug is happy to see it done for both MSVC and GCC. > 1481 is a lot of warnings to fix. Yep. Doing it one module/directory at a time is a good way to proceed.
I support this plan. Scoping it to tinderbox builds only is the only feasible way to make it work, and I think you're right that the tradeoffs are good for the value here. Having lots of warnings is harmful and makes it difficult to spot real problems. One thing that might be a PITA is Windows PGO builds. I know we hit some weird warnings that became fatal only during the final PGO link phase, which is why this is here: http://mxr.mozilla.org/mozilla-central/source/configure.in#2595 Justin is right in that we should probably also audit our codebase for spots where we've disabled warnings, whether via configure or pragmas, but that can be a follow-up after we've gotten to a stable state here.
As far as I understand it, WARNINGS_AS_ERRORS is true by default (see --disable-warnings-as-errors) and only applies to directories with FAIL_ON_WARNINGS or FAIL_ON_WARNINGS_DEBUG. According to bug 557566, it's currently opt-out but there were some issues so the FAIL_ON_WARNING changes have been removed. We should probably make this disabled by default, add --enable-<option> to buildbots' mozconfig and add FAIL_ON_WARNINGS options to makefiles slowly. It looks like WARNINGS_AS_ERRORS doesn't apply to PGO builds already.
Depends on: FAIL_ON_WARNINGS
> gcc is honest about the fact that it is not always smart enough about > presenting these warnings, and makes the warning optional for that reason. This particular warning was really bad. It also depends on things like which functions are inlined, which is very unstable. Gcc 4.7 (I think) has split this into *is* used uninitialized and "I can't prove it is not". I would suggest trying enabling -Werror, but having this particular warning disabled for gcc until we switch to 4.7.
The 1400+ warnings on Windows I was referring to earlier can be seen at http://jenkins.gregoryszorc.com:9000/job/mozilla-central/59/warningsResult/?. This is hitting a personal server, so I make no guarantees about SLA. On a related topic, the Jenkins warning pages are pretty darn nice. I'd love to see that dashboard exist somewhere in .mozilla.org. I don't think we even need to build with Jenkins either - we could have Jenkins merely pull TBPL logs and parse them into its database and dashboard. The main caveat is it gets confused for non-clobber builds (it thinks that warnings not encountered because the compilation didn't occur have been fixed).
Not sure what releng needs to do here. Configs live in the tree now (see comment #1).
Component: Release Engineering → Build Config
Product: mozilla.org → Core
QA Contact: release → build-config
Version: other → unspecified
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #583536 - Flags: review?(khuey)
Users should just not use --enable-warnings-as-errors on Android but we should not prevent them to do so.
Attachment #583537 - Flags: review?(khuey)
Basically, it's adding --enable-warnings-as-errors to all mozconfig except: - for mobile, because we know it's not a good idea right now; - for xulrunner, because, AFAIK, it doesn't show up in TBPL; - for l10n builds because at that point, build warnings doesn't really matter (do they show up on TBPL?).
So, with those patches, warnings-as-errors will not apply to anybody except if they ask for it and it will apply on all builds showing up in TBPL (except for mobile). Making a directory warning proof can be done by adding FAIL_ON_WARNINGS = 1 in Makefile.in. When done, any warning in the directory will make the build to fail in TBPL. The main downside I see is that when updating our build systems (like upgrading to a newer version of GCC/MSVC), we might hit new warnings on directories that fail on warnings. That might make build bot updates a bit trickier. However, it's hard to tell how bad that could be and if it's really annoying, [temporarily] removing FAIL_ON_WARNINGS for some directories can always be done.
I wonder if we can't get rid of the warnings due to android system headers with some pragma in the system wrappers.
If/when those patches land, we could open a follow-up to handle Android.
Attachment #583536 - Flags: review?(khuey) → review+
Attachment #583537 - Flags: review?(khuey) → review+
Comment on attachment 583538 [details] [diff] [review] Part 3 - Make buildbots enable warnings-as-errors Review of attachment 583538 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine (modulo the one comment) but needs wider discussion before r+ing. ::: browser/config/mozconfigs/win32/vs2010-mozconfig @@ +11,5 @@ > mk_add_options "export INCLUDE=$INCLUDE" > mk_add_options "export WIN32_REDIST_DIR=$WIN32_REDIST_DIR" > + > +# Treat warnings as errors in directories with FAIL_ON_WARNINGS. > +ac_add_options --enable-warnings-as-errors This is unnecessary.
(In reply to Kyle Huey [:khuey] (firstname.lastname@example.org) from comment #25) > ::: browser/config/mozconfigs/win32/vs2010-mozconfig > @@ +11,5 @@ > > mk_add_options "export INCLUDE=$INCLUDE" > > mk_add_options "export WIN32_REDIST_DIR=$WIN32_REDIST_DIR" > > + > > +# Treat warnings as errors in directories with FAIL_ON_WARNINGS. > > +ac_add_options --enable-warnings-as-errors > > This is unnecessary. Why?
Because the vs2010 mozconfig is designed to be included in the others, not used by itself.
Comment on attachment 584448 [details] [diff] [review] Part 3 - Make buildbots enable warnings-as-errors To be clear, I want you to get buy in from the project (reaching a consensus on the newsgroups, etc) before I r+ this.
Comment on attachment 583536 [details] [diff] [review] Part 1 - Make warnings-as-errors opt-in instead of opt-out A few notes on patch 1: >diff --git a/configure.in b/configure.in > dnl ======================================================== > dnl = Disable any treating of compile warnings as errors This wants to say "Enable" now. >diff --git a/js/src/configure.in b/js/src/configure.in > dnl ======================================================== > dnl = Disable any treating of compile warnings as errors Same here. (in js/src/) >- MOZ_DISABLE_WARNINGS_AS_ERRORS=1, >- MOZ_DISABLE_WARNINGS_AS_ERRORS= ) >+[ --enable-warnings-as-errors >+ Enable treating of warnings as errors], >+ MOZ_DISABLE_WARNINGS_AS_ERRORS=, >+ MOZ_DISABLE_WARNINGS_AS_ERRORS=1) It looks like you changed this (js/src/configure.in) in a different way than you changed the toplevel configure.in. In the toplevel one, you did s/DISABLE/ENABLE/, whereas here, you left the variable still being named MOZ_DISABLE_WARNINGS_AS_ERRORS. You should probably make the two configure.in files consistent on this, for sanity's sake. (The toplevel variant seems clearer, IMHO.)
Comment on attachment 584448 [details] [diff] [review] Part 3 - Make buildbots enable warnings-as-errors If you want to do this go ahead, as long as when they come for your head I get left out of it.
Attachment #584448 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (email@example.com) from comment #31) > Comment on attachment 584448 [details] [diff] [review] > Part 3 - Make buildbots enable warnings-as-errors > > If you want to do this go ahead, as long as when they come for your head I > get left out of it. Which directories use FAIL_ON_WARNINGS? AFAICT it's exactly zero.
(In reply to Nicholas Nethercote [:njn] from comment #32) > Which directories use FAIL_ON_WARNINGS? AFAICT it's exactly zero. Right -- that's because it's opt-out currently, so we can't (yet) have any FAIL_ON_WARNINGS without potentially breaking obscure compilers in default build configurations. Once it's opt-in, we can start re-adding FAIL_ON_WARNINGS and be secure in the fact that we're only affecting people with --enable-warnings-as-errors.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08d24f493dc1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b281d54daedd https://hg.mozilla.org/integration/mozilla-inbound/rev/701095bec704 \o/
Target Milestone: --- → mozilla12
Comment 30 wasn't addressed before this was checked in.
(In reply to Daniel Cater from comment #35) > Comment 30 wasn't addressed before this was checked in. I did address those comments before checking-in. I might have forget something. If that's the case, please let me know what.
This has landed on central--any reason why this bug is still open?
No reason it's still open, AFAICT -- looks like whoever did the m-i --> m-c merge just forgot to update this bug, that's all. m-c csets were: https://hg.mozilla.org/mozilla-central/rev/08d24f493dc1 https://hg.mozilla.org/mozilla-central/rev/b281d54daedd https://hg.mozilla.org/mozilla-central/rev/701095bec704
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Why WARNINGS_AS_ERRORS wasn't enabled on Windows?
(In reply to Masatoshi Kimura [:emk] from comment #39) > Why WARNINGS_AS_ERRORS wasn't enabled on Windows? MSVC was warning for a lot of stuff that other compilers were fine with. I didn't had time to consider if we should simply ignore some types of warnings. If you are interested in this, I think help would be appreciated :)
Also at the time, we were still using VS 2005 on our builders, whereas most win devs were using 2010. Now that the builders are using 2010 too, any transition should be less painful :-)
You need to log in before you can comment on or make changes to this bug.