Last Comment Bug 609532 - Stop checking in code with warnings
: Stop checking in code with warnings
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
: 639732 (view as bug list)
Depends on: 644486 643700 646336 646340 646389 1090016
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-03 20:39 PDT by Sean Stangl [:sstangl]
Modified: 2014-10-27 21:22 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Current MSVC warnings related to JS (3.36 KB, text/plain)
2010-11-05 14:18 PDT, Ryan VanderMeulen [:RyanVM]
no flags Details
patch for GCC (ie. Mac and Linux) (3.78 KB, patch)
2011-03-07 22:22 PST, Nicholas Nethercote [:njn]
dmandelin: review+
gal: feedback+
Details | Diff | Splinter Review
patch v2 (for Linux and Mac) (4.19 KB, patch)
2011-03-08 19:37 PST, Nicholas Nethercote [:njn]
dwitte: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2010-11-03 20:39:36 PDT
The JS codebase has a zero-warning policy. However, we keep checking in code that produces warnings, at which point one of us has to flag down the relevant author, and a follow-up fix has to be pushed.

We could circumvent this process by just changing the build process to pass GCC -Werror by default.
Comment 1 Ryan VanderMeulen [:RyanVM] 2010-11-04 17:10:31 PDT
Any chance of doing this on MSVC as well? WARNINGS_AS_ERRORS may help here.
Comment 2 David Mandelin [:dmandelin] 2010-11-04 17:54:47 PDT
I'm concerned that we'll burn down the tree all the time, because someone will check something in that is clean on GCC but generates warnings on MSVC or vice versa. It seems like too much alarm to generate for a minor problem.

*If* we could auto-backout or auto-reject patches that generate warnings, so the tree is not disrupted, that seems better.

Otherwise, I would incline more toward trying to up our current practices on this. Maybe every Tuesday is warning day, and an assigned person goes in and fixes all warnings.
Comment 3 Ryan VanderMeulen [:RyanVM] 2010-11-04 17:56:55 PDT
How many JS devs build with MSVC? My experience with reporting JS build warnings is that they go unnoticed for very long periods of time.
Comment 4 Luke Wagner [:luke] 2010-11-04 18:04:13 PDT
Agree with comment 2.

From my perspective, the goal is not to literally have "zero warnings" but rather not to allow warnings to accumulate since eventually you'll have so many that you ignore them and noone will even attempt to fix them and then the terrorists have won.  As far as I'm concerned, this goal is met; I rarely have to ignore more than one or two warnings.  Then again, I don't often build on MSVC (case in point for comment 3, I guess).
Comment 5 Paul Biggar 2010-11-05 07:28:27 PDT
(In reply to comment #2)
> *If* we could auto-backout or auto-reject patches that generate warnings, so
> the tree is not disrupted, that seems better.

I know RelEng does something when new warnings appear. What does it do? Making them appear in tbpl would be useful.
Comment 6 Ryan VanderMeulen [:RyanVM] 2010-11-05 14:18:54 PDT
Created attachment 488563 [details]
Current MSVC warnings related to JS

FYI, here's a list of all current JS-related warnings when building with MSVC
Comment 7 Nicholas Nethercote [:njn] 2010-11-08 21:39:55 PST
(In reply to comment #2)
> I'm concerned that we'll burn down the tree all the time, because someone will
> check something in that is clean on GCC but generates warnings on MSVC or vice
> versa. It seems like too much alarm to generate for a minor problem.

We have warnings-as-errors turned on for Nanojit and it actually triggers quite rarely.  Like, definitely less than once a month.  TM gets more commits than NJ, but I reckon it's less of a problem than you'd think.
Comment 8 Nicholas Nethercote [:njn] 2011-03-07 21:37:16 PST
*** Bug 639732 has been marked as a duplicate of this bug. ***
Comment 9 Nicholas Nethercote [:njn] 2011-03-07 22:22:06 PST
Created attachment 517659 [details] [diff] [review]
patch for GCC (ie. Mac and Linux)

This patch turns on -Werror for SpiderMonkey builds that are built with GCC, ie. Mac and Linux builds.  It also fixes the handful of warnings present, including the editline ones.

I haven't done the same for MSVC because I don't know much about it and don't have easy access to a Windows machine.  If anyone wants to supply a patch for MSVC that would be great.

I suggest we land this patch (or patches) and wait a week to see if people's fears eventuate.  I suspect they won't but I'm prepared to be proven wrong.  If we want to be cautious we could just stick with changes for GCC and worry about MSVC later.

I haven't sent this to the try server yet, if it gets r+ I'll certainly do so before landing it.
Comment 10 Andreas Gal :gal 2011-03-07 22:30:00 PST
Comment on attachment 517659 [details] [diff] [review]
patch for GCC (ie. Mac and Linux)

I think this is great.
Comment 11 David Mandelin [:dmandelin] 2011-03-08 10:22:36 PST
Comment on attachment 517659 [details] [diff] [review]
patch for GCC (ie. Mac and Linux)

If we decide to keep it, we should definitely do MSVC as well. It's only fair that you guys get to burn down the tree with warnings on other compilers, too. :-)
Comment 12 Nicholas Nethercote [:njn] 2011-03-08 19:37:15 PST
Created attachment 517964 [details] [diff] [review]
patch v2 (for Linux and Mac)

The old patch was no good.  For one, Android has warnings in system headers (siginfo.h).  philor told me about FAIL_ON_WARNINGS, so this patch sets that for GCC in js/src/Makefile.in.  For some reason it doesn't apply to ctypes/, but that's maybe not a bad thing as there were various annoying-to-fix warnings in it.
Comment 13 Phil Ringnalda (:philor) 2011-03-08 20:36:06 PST
Comment on attachment 517964 [details] [diff] [review]
patch v2 (for Linux and Mac)

I don't actually know the build system, I just know that I've seen some things before. The only two parts of this I know are that you don't need to set both - FAIL_ON_WARNINGS_DEBUG is just a way to set it once, say in a script you include in all your mozconfigs, and only have it active while building debug; and that this will only get you warnings-as-errors in the things listed in CPPSRCS in that Makefile, so you'll miss editline/, shell/, tests, and as you noticed, the libffi part of ctypes, since it's built from a $(call SUBMAKE).

Conveniently, though, dwitte doesn't just own ctypes, he also owns the last other thing in the tree (since Camino doesn't count as in the tree anymore) which used to build with warnings-as-errors, making him a perfect candidate for a reviewer.
Comment 14 Robert Sayre 2011-03-08 21:21:50 PST
(In reply to comment #9)
> 
> I suggest we land this patch (or patches) and wait a week to see if people's
> fears eventuate. 

Yeah, fair.
Comment 15 Nicholas Nethercote [:njn] 2011-03-20 18:55:28 PDT
dwitte: review ping!
Comment 16 dwitte@gmail.com 2011-03-20 19:02:04 PDT
Comment on attachment 517964 [details] [diff] [review]
patch v2 (for Linux and Mac)

For what it's worth, I once turn on -Werror on one of my modules, and spent the next month angsting over silly little bustages on obscure platforms. If it's a whitelist approach for specific platforms, that makes it more realistic.

Anecdotes aside, r=me on the ctypes bits, but: the ffi changes have to go into js/src/ctypes/libffi.patch, and reference this bug#; and preferably you'd push them upstream by mailing the patch to libffi-discuss@sourceware.org.
Comment 17 Nicholas Nethercote [:njn] 2011-03-20 19:12:00 PDT
(In reply to comment #16)
> 
> Anecdotes aside, r=me on the ctypes bits, but: the ffi changes have to go into
> js/src/ctypes/libffi.patch, and reference this bug#; and preferably you'd push
> them upstream by mailing the patch to libffi-discuss@sourceware.org.

Oh, interesting.  The FAIL_ON_WARNINGS doesn't cause libffi to be compiled with -Werror, those changes are from my earlier patch.  So it seems easiest to just take them out.
Comment 18 Nicholas Nethercote [:njn] 2011-03-21 23:21:35 PDT
Landed, with two follow-up fixes.

http://hg.mozilla.org/tracemonkey/rev/b4729e53ce45
http://hg.mozilla.org/tracemonkey/rev/5a2e69befff3
http://hg.mozilla.org/tracemonkey/rev/cdcef9d0f76a

Note:  if we decide to run warnings-as-errors off, we shouldn't back out these patches, because they contain fixes for several warnings which are worth keeping regardless.  We should instead just remove the FAIL_ON_WARNINGS lines in js/src/Makefile.in and js/src/shell/Makefile.in.
Comment 19 Nicholas Nethercote [:njn] 2011-03-22 21:19:07 PDT
A follow-up breakage fix that was in Nanojit:

http://hg.mozilla.org/projects/nanojit-central/rev/a581d0f455f1
http://hg.mozilla.org/tracemonkey/rev/2e3d8fb7c4ab
Comment 20 Nicholas Nethercote [:njn] 2011-03-27 15:23:32 PDT
One possibility is that this becomes a pain with obscure platforms and compilers, eg. AIX.  If that's the case, it might be worth only enabling it for the common platforms and compilers, eg. those covered by tinderbox testing.
Comment 21 Phil Ringnalda (:philor) 2011-03-27 21:44:44 PDT
Oh, did I forget to mention that sweet part of using FAIL_ON_WARNINGS? It comes with a built-in out for "my obscure unsupported platform warns too much" and "my brand new alpha version of GCC has warnings you've never seen before" - just build with --disable-warnings-as-errors. All three people building on AIX can just add that to their mozconfigs :)
Comment 22 cajbir (:cajbir) 2011-03-29 20:00:23 PDT
I get the following build error from trunk which seems to be from this landing:

js/src/nanojit/Containers.h:173: error: dereferencing pointer ‘data’ does break strict-aliasing rules

Previously that was a warning. Mint Linux 10 64-bit x86, gcc version:
  gcc (Ubuntu/Linaro 4.4.4-14ubuntu5) 4.4.5
Comment 23 Ted Mielczarek [:ted.mielczarek] 2011-03-30 04:59:37 PDT
Yeah, we've tried this before, and it's untenable. If we do this again it needs to be whitelisted to specific compiler version+platform combinations. (Like, say, the ones on Tinderbox, and anything that developers have personally tested with.) Even then I suspect you'll still get people complaining that "my distro GCC 4.hacked fails with -Werror".
Comment 24 Nicholas Nethercote [:njn] 2011-03-30 16:18:26 PDT
(In reply to comment #23)
> If we do this again it needs
> to be whitelisted to specific compiler version+platform combinations.

Can you describe roughly how that would be implemented?
Comment 25 Paul Biggar 2011-03-30 16:59:50 PDT
Can I suggest an alternate implementation?

The problem here is that it burns down the tree for things that are largely irrelevant, and are tough to check for in advance. What this needs is a place that's safe to burn, that can be fixed retrospectively. And we already have one of those, which is marked "SM(!m !t d s)" on tbpl.

So add it as a configure option, turned off by default, and add an extra configuration to the "SM" marking. In that case, we'll see when we've caused a new warning, but we won't burn down the tree with it.
Comment 26 Nicholas Nethercote [:njn] 2011-03-30 17:03:28 PDT
(In reply to comment #25)
> 
> So add it as a configure option, turned off by default, and add an extra
> configuration to the "SM" marking. In that case, we'll see when we've caused a
> new warning, but we won't burn down the tree with it.

Lovely!  Care to implement it?  Or explain how to?
Comment 27 Paul Biggar 2011-03-30 17:13:22 PDT
Step 0: Spidermonkey code changes, as described above.

Step 1: Add the configuration settings. Should be obvious how from looking at these commits from bug 609413:

https://hg.mozilla.org/build/buildbot-configs/rev/09d71e2e4167
https://hg.mozilla.org/build/tools/rev/87edf68bff3d

Step 2: Ask catlee for review (I think he'll need to commit for you too)

Step 3: Add a bug asking philor to add them to tbpl.
Comment 28 Nicholas Nethercote [:njn] 2011-03-30 18:01:14 PDT
I've disabled warnings-as-errors for now, and filed bug 646702 for the suggestion in comment 25 and comment 27:

http://hg.mozilla.org/tracemonkey/rev/11d72b25348d
Comment 29 Chris Leary [:cdleary] (not checking bugmail) 2011-04-01 12:04:44 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/11d72b25348d
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 14:46:36 PDT
Perhaps it would make sense for warnings-as-errors to be enabled on Tinderbox but not by default? That avoids penalizing contributors and gives us strong protection against introducing new warnings on Tinderbox configurations. It would mean increased dependence on tryserver but that may be worth it. We already have failures of the form "patch builds on my machine, but not on Tinderbox"; this would just make that more common.
Comment 31 Nicholas Nethercote [:njn] 2011-04-19 16:18:23 PDT
roc, you mean like pbiggar suggested in comment 25, and philor and catlee implemented in bug 646702 and its dependents?  See http://blog.mozilla.com/nnethercote/2011/04/19/warnings-as-errors-in-jssrc-take-2/ for a write-up.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 16:22:48 PDT
No ... there you've added an extra build type with warnings-as-errors.

I'm wondering if it's feasible to turn on warnings-as-errors for all Tinderbox builds (only for those modules that opt into that behavior, of course).
Comment 33 Nicholas Nethercote [:njn] 2011-04-19 16:37:11 PDT
Oh, so basically adding FAIL_ON_WARNINGS_ON_TINDERBOX?  Yeah, that'd be nice.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-19 17:00:29 PDT
Actually I'm suggesting
a) switch --disable-warnings-as-errors to --enable-warnings-as-errors (i.e. flip the default from enabled to disabled)
b) configure with --enable-warnings-as-errors on Tinderbox
c) add FAIL_ON_WARNINGS everywhere that currently works on Tinderbox
Comment 35 Paul Biggar 2011-04-20 05:36:00 PDT
(In reply to comment #34)
> Actually I'm suggesting
> a) switch --disable-warnings-as-errors to --enable-warnings-as-errors (i.e.
> flip the default from enabled to disabled)
> b) configure with --enable-warnings-as-errors on Tinderbox
> c) add FAIL_ON_WARNINGS everywhere that currently works on Tinderbox

This will be unwanted for the same reason as the original implementation. If you add a warning on Mac even though you use windows and don't test on mac, you'll burn down the tree. Then you need to be backed out cause the tree is burning.

The difference with my suggestion was that we burn down an irrelevant tree (a spidermonkey shell-on-build which _specifically_ is checking for warnings). The understanding we have with philor is that the SM trees are not important, should be fixed retrospectively, and dont require a backout.

I think the canonical fix is for tinderbox to have a warnings parser (does it? I heard it did but have never seen it), and to put the warnings in a different letter. Then we can have a "you must fix this, but we wont back you out because of it" policy.
Comment 36 Nicholas Nethercote [:njn] 2011-04-20 13:52:00 PDT
pbiggar, your proposal is the ideal but in practice I don't think it would make much difference for mozilla-central, as long as the warnings-as-errors is enabled in the try server.  My reasoning: (a) if you do a try server run, you'll be aware of any warnings-as-errors breakage and fix it;  (b) if you don't do a try server run, you could cause warnings-as-errors breakage on untested platforms but you could cause other kinds of breakage as well.

A warnings parser seems like unnecessary complication, and it also relegates warnings-as-errors to second class errors which I don't think is necessary.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-20 16:57:18 PDT
(In reply to comment #35)
> (In reply to comment #34)
> > Actually I'm suggesting
> > a) switch --disable-warnings-as-errors to --enable-warnings-as-errors (i.e.
> > flip the default from enabled to disabled)
> > b) configure with --enable-warnings-as-errors on Tinderbox
> > c) add FAIL_ON_WARNINGS everywhere that currently works on Tinderbox
> 
> This will be unwanted for the same reason as the original implementation. If
> you add a warning on Mac even though you use windows and don't test on mac,
> you'll burn down the tree. Then you need to be backed out cause the tree is
> burning.

The biggest problem with the original implementation (by which I assume you mean warnings-as-errors enabled by default for everyone) is that it means people not building with an exact Tinderbox configuration (e.g., fringe platform or just a different compiler version) *can't build at all* without adding --disable-warnings-as-errors to their configure. What I'm suggesting in comment #34 fixes that.
Comment 38 Paul Biggar 2011-04-20 17:41:25 PDT
(In reply to comment #36)
> pbiggar, your proposal is the ideal but in practice I don't think it would make
> much difference for mozilla-central, as long as the warnings-as-errors is
> enabled in the try server.  

The problem here is that a warnings-as-errors build will significantly increase the load on the build machines, and will therefore never happen (and I wouldn't want it to). A warnings parser will not (this doesnt matter for spidermonkey because that build takes < 2 minutes).

> My reasoning: (a) if you do a try server run,
> you'll be aware of any warnings-as-errors breakage and fix it;  (b) if you
> don't do a try server run, you could cause warnings-as-errors breakage on
> untested platforms but you could cause other kinds of breakage as well.

We shouldn't be requiring tryserver runs if the developer is sure it won't break things. Burning the tree over warnings is unnacceptable - we wont get tests run, and we'll have to back out.
Comment 39 Paul Biggar 2011-04-20 17:43:55 PDT
(In reply to comment #37)
> The biggest problem with the original implementation (by which I assume you
> mean warnings-as-errors enabled by default for everyone) is that it means
> people not building with an exact Tinderbox configuration (e.g., fringe
> platform or just a different compiler version) *can't build at all* without
> adding --disable-warnings-as-errors to their configure. What I'm suggesting in
> comment #34 fixes that.

There were two major problems with the original implementation, and we need to address both. We can't burn the tree for a warning.
Comment 40 Nicholas Nethercote [:njn] 2011-04-20 17:51:55 PDT
(In reply to comment #39)
>
> We can't burn the tree for a warning.

Why not?  If we decide that warnings are really important and we fix them, then we should treat them just like compile errors.  The three burns sometimes, it's unavoidable.
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-20 18:07:41 PDT
What Nick said.

We can at least try it and see if it's tolerable.
Comment 42 Paul Biggar 2011-04-21 03:12:57 PDT
(In reply to comment #41)
> We can at least try it and see if it's tolerable.

Not when there is a simple alternative. I'm not against this, I think it's great. I just think the cost is too high, compared to the simple alternative.

Doing it this way has one cost you haven't addressed (compiling the entire tree twice - which uses a resource that is already in short supply), and two costs we disagree about (in practice requiring tryserver runs, and burning the tree for something we can't test).

Doing it with a warnings parser (not that hard, it's a standard format, every editor and IDE does it) has none of those shortcomings.
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-21 03:32:32 PDT
(In reply to comment #42)
> Doing it this way has one cost you haven't addressed (compiling the entire tree
> twice - which uses a resource that is already in short supply),

Why would it require compiling the entire tree twice?

> and two costs
> we disagree about (in practice requiring tryserver runs, and burning the tree
> for something we can't test).

A developer could configure with --enable-warnings-as-errors if they have a configuration that matches Tinderbox. This is usually true on Windows for example.

> Doing it with a warnings parser (not that hard, it's a standard format, every
> editor and IDE does it) has none of those shortcomings.

I doubt "you must fix this, but we won't back you out" is better than "we'll back you out". Doing extra checkins to fix warnings doesn't seem very different from relanding with warnings fixed in terms of work for the developer. However, I suspect that letter will turn red or orange or purple pretty quickly, and from then on the people checking in will assume it's not their fault. But if you want to implement the warning parser etc, then I guess it's worth a try to see what happens. We will need it on tryserver and branches as well as central, though.

We did once have a warning parser which let us display warning counts on Tinderbox, but the fact that was ineffective is not relevant here since we back then we did not impose per-module "no warnings" policy.
Comment 44 Paul Biggar 2011-04-21 03:47:09 PDT
(In reply to comment #43)
> (In reply to comment #42)
> > Doing it this way has one cost you haven't addressed (compiling the entire tree
> > twice - which uses a resource that is already in short supply),
> 
> Why would it require compiling the entire tree twice?

Actually, I was confusing this with another implementation of it, where we do a second build with warnings-as-errors. Disregard and apologies.


> > and two costs
> > we disagree about (in practice requiring tryserver runs, and burning the tree
> > for something we can't test).
> 
> A developer could configure with --enable-warnings-as-errors if they have a
> configuration that matches Tinderbox. This is usually true on Windows for
> example.

This doesn't help. It's fine to expect developers to check for warning-free code on platforms they develop on. It's the platforms they don't have which are the problems. So a warning on windows when I have a Mac isn't something I can diagnose without asking tryserver (which I dont want to require) or checking in.


> I doubt "you must fix this, but we won't back you out" is better than "we'll
> back you out". Doing extra checkins to fix warnings doesn't seem very different
> from relanding with warnings fixed in terms of work for the developer. 

I disagree. Extra landings in between cause bitrot, and may require retesting. If the develop must now wait for tryserver to clear their patches, there may be many changes in between.


> We did once have a warning parser which let us display warning counts on
> Tinderbox, but the fact that was ineffective is not relevant here since we back
> then we did not impose per-module "no warnings" policy.

Using a counter rather than a binary flag would help avoid this problem:

> I suspect that letter will turn red or orange or purple pretty quickly, and
> from then on the people checking in will assume it's not their fault. But if
Comment 45 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-21 11:45:03 PDT
I don't think the problem of unexpected warnings is so large that we have to worry about requiring tryservering of everything (there wasn't even a tryserver a few years ago, and by and large people got along without compiling on every platform).  Nor do I think the burden of dealing with unforeseen warnings as they happen is so large as to nix the idea of doing tinderbox builds warning-free.

Note You need to log in before you can comment on or make changes to this bug.