Last Comment Bug 557566 - (FAIL_ON_WARNINGS) Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]
(FAIL_ON_WARNINGS)
: Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNI...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 1032641 557758 560393 561674 563849 666646 956126 1007525 1007741 1018270 1018554 1018680 1024318 1026336 1028420 1033188 1033192 1050103 1117034
Blocks: 882824 646702 703121 750381 756397 802490 805607 824247 829168 832545 833547 834054 834611 835912 836150 836348 836355 836570 837313 837903 837956 838840 839269 840189 845117 851183 866983 882859 885598 889099 891993 896292 898489 908142 913250 925243 932303 945059 945060 945063 945151 945613 956449 964077 968363 968491 974300 997982 1003907 1012858 1049738 1052033 1068113 1068977 1090016 1092001 1092028 1092710 1092711 1095878 1095883 1095926 1098134 1107814 1108934 1110031 1113229 1118149 1120312 1125690 1125693 1126808 1167250 1190148 1194580 1194948 1198334
  Show dependency treegraph
 
Reported: 2010-04-06 10:57 PDT by Daniel Holbert [:dholbert]
Modified: 2015-08-25 09:25 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG (1.89 KB, patch)
2010-04-19 10:30 PDT, Daniel Holbert [:dholbert]
ted: review+
Details | Diff | Splinter Review
part 2 v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles (7.59 KB, patch)
2010-04-19 11:03 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
part 0: add mozconfig option "--disable-warnings-as-errors" (1.80 KB, patch)
2010-05-03 14:29 PDT, Daniel Holbert [:dholbert]
ted: review+
Details | Diff | Splinter Review
part 2 v2: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles (6.65 KB, patch)
2010-05-03 14:36 PDT, Daniel Holbert [:dholbert]
ted: review+
Details | Diff | Splinter Review
part 1 v2: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted] (1.95 KB, patch)
2010-05-05 13:45 PDT, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
part 1 v2a: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted] (1.62 KB, patch)
2010-05-05 13:47 PDT, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review
part 1 v2b: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted] (3.15 KB, patch)
2010-05-05 17:59 PDT, Daniel Holbert [:dholbert]
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2010-04-06 10:57:47 PDT
In the quest to reduce compiler warnings, it would be very nice to mark currently-warning-free files or directories as "clean" in a way that lets us notice if someone suddenly introduces warning-spammy code there.

I think the easiest way to do this is with the $(WARNINGS_AS_ERRORS) build flag.  Filing this bug to add $(WARNINGS_AS_ERRORS) to a smattering of directories that are known to be warning-free right now.
Comment 1 Daniel Holbert [:dholbert] 2010-04-06 11:02:05 PDT
Note that this would mean tinderbox would turn red if anyone added warning-spammy code to a previously-clean directory. (local builds would fail too, depending on your compiler)

This redness is a minor annoyance to a developer introducing any new warning-spammy code in a previously marked-as-clean directory, but I think it's a reasonable & acceptable burden.  The alternative is that the committer would completely miss those build-warnings (which sometimes are indicative of real bugs), and they'll just pile on to warning-creep.
Comment 2 Daniel Holbert [:dholbert] 2010-04-06 11:21:39 PDT
The "obvious"/brute-force fix for this would be to just add
> CXXFLAGS += $(WARNINGS_AS_ERRORS)
to Makefiles in directories that are known-clean.

One perhaps slightly-cleaner alternative would be to define a variable in those Makefiles, e.g.
> DISALLOW_NEW_WARNINGS=1
and then check that variable wherever CXXFLAGS is set up -- in config.mk or rules.mk -- and append WARNINGS_AS_ERRORS to CXXfFLAGS if the variable is set.

This latter strategy would allow us to easily add a build flag for configuring this behavior, too, if we want (since there would only be one line where we'd actually be applying WARNINGS_AS_ERRORS, and we could wrap that one line in an #ifdef of some sort) (but maybe we don't want a build flag anyway, since I think you could disable this by setting WARNINGS_AS_ERRORS to be empty)
Comment 3 Daniel Holbert [:dholbert] 2010-04-06 11:30:48 PDT
Ted, any thoughts/suggestions/vetoes on this?  I'd be happy to work on a patch along the lines of either of the strategies from comment 2 (or a different strategy, if you can suggest something better).
Comment 4 Ted Mielczarek [:ted.mielczarek] 2010-04-06 11:35:47 PDT
I'm pretty sure we used to use the former (CXXFLAGS += $(WARNINGS_AS_ERRORS)) in some directories, but it's not currently used anywhere.
Comment 5 Phil Ringnalda (:philor) 2010-04-06 12:01:10 PDT
netwerk/cookie/src/ still does in CVS, as does camino/, though they don't count ;)
Comment 6 Daniel Holbert [:dholbert] 2010-04-06 12:10:27 PDT
Looks like  WARNINGS_AS_ERRORS got removed from network/cookie/src as part of bug 437002 / changeset af9466a5c157, due to a hunch that it was causing weirdness in PGO on windows.  I don't understand the last few comments there, though, or whether the removal had any effect.  Ted, that bug's assigned to you -- do you know what was going on there?  (and is there anything in that bug that should make us hesitate to use WARNINGS_AS_ERRORS in more places?)
Comment 7 Ted Mielczarek [:ted.mielczarek] 2010-04-06 12:18:27 PDT
I don't think we ever determined anything more than you see on that bug. The PGO build is mysterious. Adding these back in without some sort of mitigation could certainly cause weird PGO build failures.

We could try adding a flag like you described, maybe "FAIL_ON_ERRORS = 1", and then have some smarts in config.mk to only add WARNINGS_ON_ERRORS if we're not a) on windows and b) also doing a PGO build (MOZ_PROFILE_GENERATE or MOZ_PROFILE_USE is set).
Comment 8 Daniel Holbert [:dholbert] 2010-04-19 10:30:33 PDT
Created attachment 439956 [details] [diff] [review]
part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG

This patch adds checks in config.mk for two new Makefile variables:
  FAIL_ON_WARNINGS
  FAIL_ON_WARNINGS_DEBUG
The latter is a debug-only alias for the former. It's necessary because a lot of directories are warning-free for debug builds are *not* warning-free in opt builds -- largely because of variables whose only use is in optimized-away assertions, and hence get flagged with "unused" warnings.  (Those warnings can & should be fixed eventually, but I don't think we want to try to fix them all before landing this.)
Comment 9 Daniel Holbert [:dholbert] 2010-04-19 11:03:40 PDT
Created attachment 439968 [details] [diff] [review]
part 2 v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles

This patch applies the FAIL_ON_WARNINGS & FAIL_ON_WARNINGS_DEBUG flags in 15 currently-warning-free directories.

This compiles successfully on my linux machine, using g++ 4.3, for both debug and opt builds.*

I'm currently running this through tryserver (which is opt-only, sigh), to see if it trips over any compile warnings on other platforms.

*Note: I'm building on top of these two simple warning-fixes, which should hopefully land soon:
http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/6903793d9902/accelerometer_warningfix.patch
http://hg.mozilla.org/users/dholbert_mozilla.com/simple-patches/file/6903793d9902/text_control_frame_warningfix.patch
Comment 10 Daniel Holbert [:dholbert] 2010-04-19 11:35:24 PDT
Note: This failed on TryServer maemo box, due to bug 557758, which has a patch that should fix the maemo issues once bug 441767 lands. (and vlad says he can land bug 441767 later today -- yay!)
Comment 11 Daniel Holbert [:dholbert] 2010-04-19 14:07:02 PDT
(In reply to comment #9)
> *Note: I'm building on top of these two simple warning-fixes, which should
> hopefully land soon:
(those warning-fixes are Bug 560271 and Bug 560274, and they've both landed)
Comment 12 Daniel Holbert [:dholbert] 2010-04-20 14:01:02 PDT
sdwilsh points out in IRC that anyone with non-ASCII characters in their username will get compile warnings everywhere, in MSVC, due to the use of
 -DDEBUG_[username]

So, Patch A probably needs an additional check for "if [username is non-ascii-chars]", inside the "ifeq (WINNT,$(OS_ARCH))" clause.  I don't know how to write that in Makefile-speak though...
Comment 13 Daniel Holbert [:dholbert] 2010-04-20 14:01:40 PDT
(In reply to comment #12)
> needs an additional check for "if [username is non-ascii-chars]"

er, s/is/contains/
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-04-25 07:39:26 PDT
(In reply to comment #12)
> sdwilsh points out in IRC that anyone with non-ASCII characters in their
> username will get compile warnings everywhere, in MSVC, due to the use of
>  -DDEBUG_[username]

Can we just stop defining that?  I went on a crusade eradicating all of the #ifdef DEBUG_<long gone netscape engineer> from widget/src/windows a few months back and would be more than happy to eradicate them from the rest of the tree.  IMO we should be using #ifdef DEBUG_<FEATURE> rather than #ifdef DEBUG_<PERSON> e.g. DEBUG_LAYOUT, DEBUG_CC, etc.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2010-04-25 15:53:44 PDT
That would be fine with me. You could ask around and see if anyone still uses that.
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-04-25 16:21:58 PDT
(In reply to comment #15)
> That would be fine with me. You could ask around and see if anyone still uses
> that.

Asking: http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/b0bedb1c7db30f5b#
Comment 17 Mounir Lamouri (:mounir) 2010-04-25 16:40:55 PDT
Wouldn't be interesting to add an option in configure.in to let the user disable this ? So, if an user doesn't care about warnings or is compiling on a unusual platform, he/she could prevent annoying errors.
I think 'strict' is commonly used for this kind of feature.
Comment 18 Daniel Holbert [:dholbert] 2010-04-26 16:12:26 PDT
Ok, so assuming bug 561674 lands (removing the auto-define of DEBUG_username), then we don't have to worry about comment 12 anymore.

(In reply to comment #17)
> Wouldn't be interesting to add an option in configure.in to let the user
> disable this ? 

Seems reasonable.  Perhaps --disable-warnings-as-errors, which could just unset WARNINGS_AS_ERRORS in configure.in.  I'll update Patch A to include this.
Comment 19 Daniel Holbert [:dholbert] 2010-05-03 14:29:51 PDT
Created attachment 443190 [details] [diff] [review]
part 0: add mozconfig option "--disable-warnings-as-errors"

(In reply to comment #18)
> Seems reasonable.  Perhaps --disable-warnings-as-errors, which could just unset
> WARNINGS_AS_ERRORS in configure.in.

Did this as a separate patch, for simplicity.  This lets people clear WARNINGS_AS_ERRORS by adding
>ac_add_options --disable-warnings-as-errors
to their mozconfig.
Comment 20 Daniel Holbert [:dholbert] 2010-05-03 14:36:49 PDT
Created attachment 443193 [details] [diff] [review]
part 2 v2: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles

Updated version of part 2.  Changes in this version:
 - No longer adds FAIL_ON_WARNINGS in widget/src/xpwidgets/Makefile.in or content/media/Makefile.in, due to some Windows- and Mac-only warnings in those directories that I didn't hit locally but uncovered on TryServer.
 - Fixes bitrot in content/media/ogg/Makefile.in
Comment 21 Ted Mielczarek [:ted.mielczarek] 2010-05-05 11:51:46 PDT
Comment on attachment 439956 [details] [diff] [review]
part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG

>diff --git a/config/config.mk b/config/config.mk
>--- a/config/config.mk
>+++ b/config/config.mk
>+# Check for FAIL_ON_WARNINGS & FAIL_ON_WARNINGS_DEBUG (Shorthand for Makefiles
>+# to request that we use the 'warnings as errors' compile flags)
>+
>+# NOTE: First, we clear FAIL_ON_WARNINGS[_DEBUG] if we're doing a Windows PGO
>+# build, since WARNINGS_AS_ERRORS has been suspected of causing isuses in that
>+# situation. (See bug 437002.)
>+ifeq (WINNT,$(OS_ARCH))
>+ifneq (,$(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE))

You could combine these two lines into:
ifeq (WINNT_1,$(OS_ARCH)_$(MOZ_PROFILE_GENERATE)$(MOZ_PROFILE_USE))

>+ifdef FAIL_ON_WARNINGS_DEBUG
>+FAIL_ON_WARNINGS_DEBUG = 0

You should just set this to an empty value. We don't conventionally use a value of zero to mean anything. Also, you can just set both of these to empty without checking their values first, it won't hurt anything.


>+# Now, check for debug version of flag; it turns on normal flag in debug builds.
>+ifdef FAIL_ON_WARNINGS_DEBUG
>+ifeq (1,$(FAIL_ON_WARNINGS_DEBUG))

Just the ifdef should be fine, our boolean makefile variables are "unset = false, any value = true".


>+# Check for normal version of flag, and add WARNINGS_AS_ERRORS if it's set to 1.
>+ifdef FAIL_ON_WARNINGS
>+ifeq (1,$(FAIL_ON_WARNINGS))

Same here.

>+CXXFLAGS += $(WARNINGS_AS_ERRORS)

Do you want to set CFLAGS too?

r=me with those changes.
Comment 22 Ted Mielczarek [:ted.mielczarek] 2010-05-05 11:54:05 PDT
Comment on attachment 443193 [details] [diff] [review]
part 2 v2: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles

I think you should file a followup on making the places where you need _DEBUG not warn in release builds. Do we have some sort of UNUSED macro? Having warnings in our release builds due to stray debug variables seems wrong.
Comment 23 Daniel Holbert [:dholbert] 2010-05-05 13:20:02 PDT
(In reply to comment #22)
> (From update of attachment 443193 [details] [diff] [review])
> I think you should file a followup on making the places where you need _DEBUG
> not warn in release builds.

Will do.

> Do we have some sort of UNUSED macro?

GCC's convention to suppress the warning is:
   |(void)variableName;|
I think our preferred solution is to wrap the variable declaration in |#ifdef DEBUG/#endif|, though.

> Having
> warnings in our release builds due to stray debug variables seems wrong.

Yup, though sadly it happens a lot, I think. :)

Also, a good number of the warnings are for "variable may be used uninitialized".  For some reason, these warnings only get triggered in optimized builds.  I've filed bugs on a number of those: bug 560266, bug 557742, bug 557690, bug 534171, bug 534162, bug 534156, bug 534150

Often this "may be used uninitialized" warning actually isn't a problem -- e.g. 
  int foo;
  if (myFlag) {
    foo = [something];
  }
  // other stuff, that doesn't touch myFlag or foo

  if (myFlag) {
    someFunction(foo);
  }
As long as that "// other stuff" clause doesn't touch myFlag, then this isn't a problem.  Still, it may be better to take the (probably-negligible) perf hit and initialize foo to some sentinel value up-front, for the sake of sanity & safety.
Comment 24 Daniel Holbert [:dholbert] 2010-05-05 13:45:49 PDT
Created attachment 443716 [details] [diff] [review]
part 1 v2: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted]

Here's part 1 again, with ted's review feedback from comment 21 addressed.  Carrying forward r=ted.
Comment 25 Daniel Holbert [:dholbert] 2010-05-05 13:47:35 PDT
Created attachment 443717 [details] [diff] [review]
part 1 v2a: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted]

sorry, that last version had an accidental space character introduced at the top of the file. fixed here.
Comment 26 Daniel Holbert [:dholbert] 2010-05-05 17:59:41 PDT
Created attachment 443776 [details] [diff] [review]
part 1 v2b: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG [r=ted]

One more update to part 1: this now applies the changes to "js/src/config/config.mk", too, so that "make check" doesn't complain about it being out of sync with config/config.mk.
Comment 27 Daniel Holbert [:dholbert] 2010-05-06 11:59:44 PDT
Landed part 0 & part 1:
http://hg.mozilla.org/mozilla-central/rev/a0b935912fa2
http://hg.mozilla.org/mozilla-central/rev/86af015da3fc

(This just means FAIL_ON_WARNINGS is now *available* for use in Makefiles, but it isn't actually actually used anywhere yet -- we need to wait on bug 561674 before we can do that, per comment 12 / comment 14).
Comment 28 Daniel Holbert [:dholbert] 2010-06-26 14:35:44 PDT
Landed a tweaked version of "part 2"
 - I removed the _DEBUG chunks -- I intend to land those in a separate patch, with a few new additions.
 - Added two new warning-free directories: uriloader/base and docshell/shistory/src/

http://hg.mozilla.org/mozilla-central/rev/5da9fc2be835
Comment 29 Daniel Holbert [:dholbert] 2010-06-26 15:18:45 PDT
Pushed a followup to disable FAIL_ON_WARNINGS[_DEBUG] on Android for now, since android adds some global-ish #included files that have build warnings, causing failures like this:
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1277588954.1277589437.28511.gz
Followup is: http://hg.mozilla.org/mozilla-central/rev/19f3fce93339
Comment 30 Daniel Holbert [:dholbert] 2010-06-26 18:09:02 PDT
Pushed another followup to remove FAIL_ON_WARNINGS from "canvas" & "wave" directories, since GCC 4.4 suffers from a version of bug 573895 in those directories. (being tracked on that bug)
http://hg.mozilla.org/mozilla-central/rev/a65d962718b0
Comment 31 Daniel Holbert [:dholbert] 2010-06-26 20:09:02 PDT
Backed out the FAIL_ON_WARNINGS additions for now, per m.d.platform thread:
https://groups.google.com/group/mozilla.dev.platform/msg/db86e555e7e40a9e

Backout is:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1934562edc1b

Hoping to change this to be opt-in instead of opt-out and get the tinderbox farm opted in, so we can still catch new warnings in previously-clean directories without causing pain in local builds for people with non-"standard" compilers.
Comment 32 Ted Mielczarek [:ted.mielczarek] 2010-06-27 04:53:56 PDT
I can think of two ways to do this:
1) Have a configure option to enable the behavior
2) Have a whitelist of supported compilers in configure, enable only for those compilers

The problem with #1 is that if developers don't enable it locally, but we have it enabled on tinderbox, they can wind up pushing something that built fine locally but errors on tinderbox.
Comment 33 Daniel Holbert [:dholbert] 2011-04-19 09:55:56 PDT
I'm not actively working on the remaining parts of this anymore, so I'm reshaping this into just Comment 27, and resolving as fixed.

If we end up wanting to build with warnings as errors in certain directories by default, that should probably be done in followup bugs (like Bug 646702 -- woohoo!)

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