703121 - Turn on WARNINGS_AS_ERRORS for TBPL builds

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

The relevant configs live in Hg now (e.g. browser/config/mozconfigs), so this would just be a matter of changing those.

Comment 2

[Daniel Holbert [:dholbert]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

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!

Comment 4

[Karl Tomlinson (:karlt)]

(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.

Comment 5

[Nicholas Nethercote [inactive]]

(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.

Comment 6

[Karl Tomlinson (:karlt)]

If it's a common practice, that's very unfortunate, but not a reason to advocate it.

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Daniel Holbert [:dholbert]]

(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.)

Comment 9

[Nicholas Nethercote [inactive]]

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?

Comment 10

[Karl Tomlinson (:karlt)]

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.

Comment 11

[Gregory Szorc [:gps]]

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.

Comment 12

[Justin Lebar (not reading bugmail)]

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.

Comment 13

[Nicholas Nethercote [inactive]]

(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.

Comment 14

[(not currently active) Ted Mielczarek]

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.

Comment 15

[Mounir Lamouri (:mounir)]

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.

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> 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.

Comment 17

[Gregory Szorc [:gps]]

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).

Comment 18

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

Not sure what releng needs to do here. Configs live in the tree now (see comment #1).

Comment 19

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Make warnings-as-errors opt-in instead of opt-out

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Remove the dirty hack that make warnings-as-errors not working on Android

Users should just not use --enable-warnings-as-errors on Android but we should not prevent them to do so.

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Make buildbots enable warnings-as-errors

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?).

Comment 22

[Mounir Lamouri (:mounir)]

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.

Comment 23

[Mike Hommey [:glandium]]

I wonder if we can't get rid of the warnings due to android system headers with some pragma in the system wrappers.

Comment 24

[Mounir Lamouri (:mounir)]

If/when those patches land, we could open a follow-up to handle Android.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 26

[Mounir Lamouri (:mounir)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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?

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Because the vs2010 mozconfig is designed to be included in the others, not used by itself.

Comment 28

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Make buildbots enable warnings-as-errors

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 30

[Daniel Holbert [:dholbert]]

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 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 32

[Nicholas Nethercote [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 33

[Daniel Holbert [:dholbert]]

(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.

Comment 34

[Ed Morley [:emorley]]

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/

Comment 35

[Daniel C]

Comment 30 wasn't addressed before this was checked in.

Comment 36

[Mounir Lamouri (:mounir)]

(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.

Comment 37

[Jason Duell]

This has landed on central--any reason why this bug is still open?

Comment 38

[Daniel Holbert [:dholbert]]

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

Comment 39

[Masatoshi Kimura [:emk]]

Why WARNINGS_AS_ERRORS wasn't enabled on Windows?

Comment 40

[Mounir Lamouri (:mounir)]

(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 :)

Comment 41

[Ed Morley [:emorley]]

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 :-)

Comment 42

[Masatoshi Kimura [:emk]]

OK, filed bug 824247.