New "FAIL_ON_WARNINGS" Makefile-variable on mozilla-central
Daniel Holbert
just landed a patch that adds the new FAIL_ON_WARNINGS annotation to some
Makefiles on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/5da9fc2be835
* What is FAIL_ON_WARNINGS?
---------------------------
It's a Makefile variable (created in bug 557566) that allows us to cleanly
annotate a directory as "currently has no build warnings on any
tinderbox-tested platforms" (using WARNINGS_AS_ERRORS under the hood). If
this warning-free promise is violated, then the build will fail.
* Warnings shmornings! Why bother avoiding them?
------------------------------------------------
A few reasons:
- Build warnings often turn out to be real behavior-altering bugs.
- Usually they're quite easy to fix.
- Even if a particular warning is ignorable, it adds to our
already-overwhelming compiler-spam[1], which makes it harder to identify
new build-warnings caused by your local patches. (possibly real bugs in
your patches)
[1] From grepping a recent tinderbox clobber-build log, we've currently
got at least 2434 lines of build warnings on Linux, and at least 3079 on
Windows. (for 32-bit debug builds)
* Hey, this breaks my build! How do I fix it?
---------------------------------------------
If you're building with a patch that introduces new warnings to a
FAIL_ON_WARNINGS-blessed directory, then your build will fail. (That's
the idea. :)) There are a few ways to fix this:
(a) Fix the code you've added that the warning is pointing at. (Yay!)
(b) Suppress the warning, if you know it's useless -- either with a
local #pragma, or globally if it's known-to-be-useless-in-all-cases.
Or, if you just need a quick-and-dirty hack to get your build to run, you
can do one of these:
(c) Temporarily remove FAIL_ON_WARNINGS from the directory's Makefile.in
(d) Add this to your mozconfig to globally ignore this annotation:
ac_add_options --disable-warnings-as-errors
Options (c) and (d) are intended only as temporary hacks, though -- not as
actual solutions. (The mozconfig option is also handy if you're doing
builds on a platform/compiler/configuration that isn't tested on tinderbox
& that spams build-warnings that we don't know about.)
* I've got a warning-free directory to mark as FAIL_ON_WARNINGS.
----------------------------------------------------------------
Great! I'm sure there are other directories that already qualify for this
annotation -- I just picked a few that I noticed to be warning-free for
this first patch. Any FAIL_ON_WARNINGS patches should definitely go
through TryServer, though -- there are cases where we hit a particular
warning *only* on e.g. 64-bit Linux opt builds -- so even if a directory
is warning-free in your local build, it's entirely possible that it has
build warnings on other platforms.
Thanks! Comments/questions welcome.
~Daniel
Robert Kaiser
> * What is FAIL_ON_WARNINGS?
> ---------------------------
> It's a Makefile variable (created in bug 557566) that allows us to
> cleanly annotate a directory as "currently has no build warnings on any
> tinderbox-tested platforms" (using WARNINGS_AS_ERRORS under the hood).
> If this warning-free promise is violated, then the build will fail.
In many cases in the past, we had WARNINGS_AS_ERRORS break builds
completely when gcc updates to a new version and adds new warnings for
things that have not produced warnings in earlier versions - I hope we
don't make lives too difficult for people there... ;-)
Robert Kaiser
--
Note that any statements of mine - no matter how passionate - are never
meant to be offensive but very often as food for thought or possible
arguments that we as a community needs answers to. And most of the time,
I even appreciate irony and fun! :)
L. David Baron
> Daniel Holbert schrieb:
> >* What is FAIL_ON_WARNINGS?
> >---------------------------
> >It's a Makefile variable (created in bug 557566) that allows us to
> >cleanly annotate a directory as "currently has no build warnings on any
> >tinderbox-tested platforms" (using WARNINGS_AS_ERRORS under the hood).
> >If this warning-free promise is violated, then the build will fail.
>
> In many cases in the past, we had WARNINGS_AS_ERRORS break builds
> completely when gcc updates to a new version and adds new warnings
> for things that have not produced warnings in earlier versions - I
> hope we don't make lives too difficult for people there... ;-)
I think we shouldn't try doing warnings-as-errors by default; it's a
recipe for giving people lots of random build bustage, since
warnings vary significantly by compiler version, and we don't have
tinderbox coverage for anywhere near all the compiler versions
developers regularly use.
(Then again, it broke my build because I have -Wshadow turned on in
my tree. Getting -Wshadow turned back on is a whole other story.
We originally turned it off because of incorrect warnings related to
compiling JS in C. Those warnings went away when we switched JS to
C++, but now there's tons of intentional shadowing in the JS engine,
especially in nanojit, so it might be hard to turn back on.)
-David
--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/
Daniel Holbert
> In many cases in the past, we had WARNINGS_AS_ERRORS break builds
> completely when gcc updates to a new version and adds new warnings
Yeah -- turns out mats already caught one of those instances right out of
the gate with GCC 4.4, as noted in
https://bugzilla.mozilla.org/show_bug.cgi?id=557566#c30
I pushed a followup soon after, so as not to cause too much pain for
developers like him who are using GCC 4.4.
On 06/26/2010 05:12 PM, L. David Baron wrote:
> I think we shouldn't try doing warnings-as-errors by default; it's a
> recipe for giving people lots of random build bustage, since
> warnings vary significantly by compiler version, and we don't have
> tinderbox coverage for anywhere near all the compiler versions
> developers regularly use.
So, I'd envisioned that the solution for failed builds from non-"standard"
compiler versions would be the "--disable-warnings-as-errors" mozconfig
option (plus bugs filed on the warnings, ideally).
If is too much of a hassle, though, we could perhaps default-disable
warnings-as-errors, and then enable them via a mozconfig flag on Tinderbox
(so we'd still be able to catch the introductions of new build warnings in
warning-free directories).
~Daniel
L. David Baron
> On 06/26/2010 05:12 PM, L. David Baron wrote:
> >I think we shouldn't try doing warnings-as-errors by default; it's a
> >recipe for giving people lots of random build bustage, since
> >warnings vary significantly by compiler version, and we don't have
> >tinderbox coverage for anywhere near all the compiler versions
> >developers regularly use.
>
> So, I'd envisioned that the solution for failed builds from
> non-"standard" compiler versions would be the
> "--disable-warnings-as-errors" mozconfig option (plus bugs filed on
> the warnings, ideally).
>
> If is too much of a hassle, though, we could perhaps default-disable
> warnings-as-errors, and then enable them via a mozconfig flag on
> Tinderbox (so we'd still be able to catch the introductions of new
> build warnings in warning-free directories).
I think non-"standard" compilers are the norm, given that we're only
building on a very small number of configurations on tinderbox (only
one or two compiler version + library/SDK version + OS version
combinations per platform).
We don't want to scare off new contributors with build errors that
happen the first time they try to build Mozilla, and which require a
weird .mozconfig incantation to fix.
I think enabling them on tinderbox would probably be ok (assuming
enough people use try server), though.
And in the interim, I think it's better off than on; we don't want
everybody wasting time when they pull mozilla-central Monday morning
and have to fight with new build errors.
Daniel Holbert
> I think non-"standard" compilers are the norm
Agreed. (that was why I used quotes. :))
> We don't want to scare off new contributors with build errors that
> happen the first time they try to build Mozilla, and which require a
> weird .mozconfig incantation to fix.
Makes sense -- agreed.
> I think enabling them on tinderbox would probably be ok (assuming
> enough people use try server), though.
I hope so -- without some automated way to visibly catch new warnings &
flag the guilty changeset, it's harder / impossible to prevent
warning-bloat, because people won't realize it when their patch introduces
new build-warnings.
Note also that in bustage situations, there's always the option of pushing
a temporary fix to disable FAIL_ON_WARNINGS for the responsible
director[y|ies], while the developer works on a better bustage-fix in the
meantime.
> And in the interim, I think it's better off than on; we don't want
> everybody wasting time when they pull mozilla-central Monday morning
> and have to fight with new build errors.
Agreed -- I'm intending to disable this later tonight (just waiting for
another Win64 build, to see if my patch for bug 575014 had any greening
effect). After that, I intend to tweak this to be opt-in instead of
opt-out, I'll file a bug on hopefully opting the tinderboxen in.
Boris Zbarsky
> If this warning-free promise is violated, then the build will fail.
So this more or less necessitates that pretty much any change, no matter
how trivial, run on the try server to determine whether it builds, since
especially opt gcc has quite a number of weird edge-case warnings (many
of them spurious) last I checked....
That's probably ok if we have the try server bandwidth for that. Do we?
I know in my case it'd raise the number of try runs I do by a factor
of 2-3.
-Boris
Robert Kaiser
> If is too much of a hassle, though, we could perhaps default-disable
> warnings-as-errors, and then enable them via a mozconfig flag on
> Tinderbox (so we'd still be able to catch the introductions of new build
> warnings in warning-free directories).
That sounds saner to me. (If we want to go for eliminating them, it
might even be nice to get back the warnings stats we had many years ago,
that did analyze warnings and put blame on them.)
Robert Kaiser
> I hope so -- without some automated way to visibly catch new warnings &
> flag the guilty changeset, it's harder / impossible to prevent
> warning-bloat, because people won't realize it when their patch
> introduces new build-warnings.
Could we have, for example, a script that posts changes to produced
warnings to m.d.tree-management, just like we're doing right now for
perf numbers? That would make this more trackable, I guess.
Ted Mielczarek
> Daniel Holbert schrieb:
>>
>> I hope so -- without some automated way to visibly catch new warnings &
>> flag the guilty changeset, it's harder / impossible to prevent
>> warning-bloat, because people won't realize it when their patch
>> introduces new build-warnings.
>
> Could we have, for example, a script that posts changes to produced warnings
> to m.d.tree-management, just like we're doing right now for perf numbers?
> That would make this more trackable, I guess.
bsmedberg has a buildbot-hosted warnings tracker database[1] that does
essentially this: keeps track of warnings and assigns blame to them
based on the changeset author. It used to run on his static analysis
box, but AIUI that machine has been repurposed. He filed a bug[2] and
got IT to set up a VM to host it, but is now too busy with e10s to
finish this work. I think if someone else wanted to take on this work
that could easily be arranged.
-Ted
1. http://hg.mozilla.org/users/bsmedberg_mozilla.com/static-analysis-buildbot/
2. https://bugzilla.mozilla.org/show_bug.cgi?id=512750
Boris Zbarsky
> On 06/26/2010 09:27 PM, Boris Zbarsky wrote:
>> So this more or less necessitates that pretty much any change, no matter
>> how trivial, run on the try server to determine whether it builds
>
> trivial of changes.
That's the problem. Very trivial changes can introduce build warnings
in unexpected ways...
> If there are truly spurious warnings that we accidentally introduce
> frequently
I fully expect the "x may be used uninitialized" or whatever the wording
is warning to be such. The problem is that gcc's detection here seems
to be pretty broken. Here's a simple example:
void test(unsigned i) {
int j;
if (i == 0) j = 1;
else if (i == 1) j = 2;
printf("This is a test\n");
if (i == 0) printf("One? %d\n", j);
else if (i == 1) printf("Two? %d\n", j);
}
Note that removing that printf in the middle shuts up the warning
(presumably because then the cases get commoned together or something).
This _can_ be worked around by adding a useless initialization of 'j',
but that could cover up real bugs (e.g. if in that function I take out
the printf and change the last else if to check against 2 I get a "x is
used uninitialized", instead of "may be used" warning) and costs cycles
so may be avoided in some hot loops.
This is a really useful thing to detect if the detection is working
well, but gcc's just isn't. :(
> There's also a debug-build-only variant of the Makefile variable, too --
> FAIL_ON_WARNINGS_DEBUG -- that can be used in cases where directories
> are known to have multiple opt-build warnings but 0 debug-build warnings.
That sounds excellent to me.
> Once we have the ability to request *only* a TryServer *build*, with no
> unit-tests at all, I don't think the added load of builds would present
> much of an additional burden. (I think joduinn said that ability is
> coming, in a Tuesday platform meeting a month back or so. Not sure how
> soon it will be here.)
Ah, indeed. Perfect!
-Boris
Daniel Holbert
> I fully expect the "x may be used uninitialized" or whatever the wording
> is warning to be such. The problem is that gcc's detection here seems to
> be pretty broken.
Yeah - we get that warning all over the place, in opt builds. :( That
warning was one of the main stumbling-blocks for finding directories were
suitable for adding FAIL_ON_WARNINGS. On the bright side, though,
FAIL_ON_WARNINGS_DEBUG should work around that one.
Nicholas Nethercote
>
> (Then again, it broke my build because I have -Wshadow turned on in
> my tree. Getting -Wshadow turned back on is a whole other story.
> We originally turned it off because of incorrect warnings related to
> compiling JS in C. Those warnings went away when we switched JS to
> C++, but now there's tons of intentional shadowing in the JS engine,
> especially in nanojit, so it might be hard to turn back on.)
I'd be happy to get rid of that shadowing. The lack of -Wshadow has
bitten me more than once.
Nick
Taras Glek
> On Sunday 2010-06-27 01:33 +0200, Robert Kaiser wrote:
>> Daniel Holbert schrieb:
> (Then again, it broke my build because I have -Wshadow turned on in
> my tree. Getting -Wshadow turned back on is a whole other story.
> We originally turned it off because of incorrect warnings related to
> compiling JS in C. Those warnings went away when we switched JS to
> C++, but now there's tons of intentional shadowing in the JS engine,
> especially in nanojit, so it might be hard to turn back on.)
This sounds like a gcc bug. JS is pretty shadow-free if you allow
constructor parameters that match class members.
https://bugzilla.mozilla.org/show_bug.cgi?id=522776
We'll probably land this once static analysis stuff is ported to 4.5.
Taras