Closed Bug 557566 (FAIL_ON_WARNINGS) Opened 14 years ago Closed 13 years ago

Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 4 obsolete files)

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.
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.
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)
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).
I'm pretty sure we used to use the former (CXXFLAGS += $(WARNINGS_AS_ERRORS)) in some directories, but it's not currently used anywhere.
netwerk/cookie/src/ still does in CVS, as does camino/, though they don't count ;)
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?)
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).
Depends on: 557758
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #439956 - Flags: review?
Attachment #439956 - Flags: review? → review?(ted.mielczarek)
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
Attachment #439968 - Flags: review?(ted.mielczarek)
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!)
(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)
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...
(In reply to comment #12)
> needs an additional check for "if [username is non-ascii-chars]"

er, s/is/contains/
(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.
That would be fine with me. You could ask around and see if anyone still uses that.
(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#
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.
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.
Depends on: 561674
Attachment #439956 - Attachment description: patch A v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG → part 1 v1: Add FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG
Attachment #439968 - Attachment description: patch B v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles → part 2 v1: Use FAIL_ON_WARNINGS/FAIL_ON_WARNINGS_DEBUG in various Makefiles
(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.
Attachment #443190 - Flags: review?(ted.mielczarek)
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
Attachment #443193 - Flags: review?(ted.mielczarek)
Attachment #439968 - Attachment is obsolete: true
Attachment #439968 - Flags: review?(ted.mielczarek)
Depends on: 563849
Depends on: 560393
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.
Attachment #439956 - Flags: review?(ted.mielczarek) → review+
Attachment #443190 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #443193 - Flags: review?(ted.mielczarek) → review+
(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.
Here's part 1 again, with ted's review feedback from comment 21 addressed.  Carrying forward r=ted.
Attachment #439956 - Attachment is obsolete: true
Attachment #443716 - Flags: review+
sorry, that last version had an accidental space character introduced at the top of the file. fixed here.
Attachment #443716 - Attachment is obsolete: true
Attachment #443717 - Flags: review+
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.
Attachment #443717 - Attachment is obsolete: true
Attachment #443776 - Flags: review+
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).
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
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
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
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.
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.
Blocks: 646702
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!)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: Build with WARNINGS_AS_ERRORS in currently-warning-free directories → FAIL_ON_WARNINGS: Allow building with WARNINGS_AS_ERRORS in specific directories
Alias: FAIL_ON_WARNINGS
Summary: FAIL_ON_WARNINGS: Allow building with WARNINGS_AS_ERRORS in specific directories → Allow building with WARNINGS_AS_ERRORS in specific directories
Blocks: 703121
Blocks: 750381
Depends on: 750445
No longer depends on: 750445
Summary: Allow building with WARNINGS_AS_ERRORS in specific directories → Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]
Depends on: 824247
Blocks: 829168
Blocks: 832545
Blocks: 833547
Blocks: 834054
Blocks: 835912
Blocks: 836150
Blocks: 836348
Blocks: 836355
Blocks: 836570
Blocks: 756397
Blocks: 837903
Blocks: 837956
Blocks: 838840
Blocks: 839269
Blocks: 840189
Blocks: 845117
Blocks: 851183
Blocks: 866983
Blocks: 885598
Blocks: 882859
Blocks: 882824
Blocks: 889099
Blocks: 891993
Blocks: 896292
Blocks: 898489
Blocks: 913250
Blocks: 802490
Blocks: 805607
Blocks: 837313
Blocks: 925243
Blocks: 932303
Blocks: 945060
Blocks: 945063
Blocks: 945059
Blocks: 945151
Depends on: 945613
Blocks: 945613
No longer depends on: 945613
Blocks: 824247
No longer depends on: 824247
Depends on: 956126
Blocks: 956449
Blocks: 964077
Blocks: 968363
Blocks: 968491
Blocks: 974300
Blocks: 997982
Blocks: 1003907
Depends on: 1007525
Depends on: 1007741
Depends on: 666646
Blocks: 1012858
Depends on: 1018270
Depends on: 1018554
Depends on: 1018680
Depends on: 1024318
Depends on: 1026336
Depends on: 1028420
Depends on: 1032644
Depends on: 1033188
Depends on: 1033192
No longer depends on: 1032644
Depends on: 1032641
Blocks: 1049738
Depends on: 1050103
No longer blocks: 1049738
Depends on: 1049738
Blocks: 1049738
No longer depends on: 1049738
Depends on: 1052033
Blocks: 1052033
No longer depends on: 1052033
Blocks: 1068113
Blocks: 1068977
No longer blocks: 1108932
No longer blocks: 1108938
Blocks: 1126808
Blocks: 1167250
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.