Bug 557566 (FAIL_ON_WARNINGS)

Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]

RESOLVED FIXED

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
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)
(Assignee)

Comment 3

7 years ago
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 ;)
(Assignee)

Comment 6

7 years ago
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).
(Assignee)

Updated

7 years ago
Depends on: 557758
(Assignee)

Comment 8

7 years ago
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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #439956 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #439956 - Flags: review? → review?(ted.mielczarek)
(Assignee)

Comment 9

7 years ago
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
Attachment #439968 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 10

7 years ago
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!)
(Assignee)

Comment 11

7 years ago
(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)
(Assignee)

Comment 12

7 years ago
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...
(Assignee)

Comment 13

7 years ago
(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.
(Assignee)

Comment 18

7 years ago
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
(Assignee)

Updated

7 years ago
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
(Assignee)

Updated

7 years ago
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
(Assignee)

Comment 19

7 years ago
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.
Attachment #443190 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

7 years ago
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
Attachment #443193 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Attachment #439968 - Attachment is obsolete: true
Attachment #439968 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

7 years ago
Depends on: 563849
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 23

7 years ago
(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.
(Assignee)

Comment 24

7 years ago
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.
Attachment #439956 - Attachment is obsolete: true
Attachment #443716 - Flags: review+
(Assignee)

Comment 25

7 years ago
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.
Attachment #443716 - Attachment is obsolete: true
Attachment #443717 - Flags: review+
(Assignee)

Comment 26

7 years ago
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.
Attachment #443717 - Attachment is obsolete: true
Attachment #443776 - Flags: review+
(Assignee)

Comment 27

7 years ago
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).
(Assignee)

Comment 28

7 years ago
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
(Assignee)

Comment 29

7 years ago
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
(Assignee)

Comment 30

7 years ago
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
(Assignee)

Comment 31

7 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 646702
(Assignee)

Comment 33

6 years ago
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
Last Resolved: 6 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
(Assignee)

Updated

6 years ago
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

Updated

5 years ago
Summary: Allow building with WARNINGS_AS_ERRORS in specific directories → Allow building with WARNINGS_AS_ERRORS in specific directories [FAIL_ON_WARNINGS]
(Assignee)

Updated

5 years ago
Depends on: 824247
(Assignee)

Updated

5 years ago
Blocks: 829168
(Assignee)

Updated

5 years ago
Blocks: 832545
(Assignee)

Updated

5 years ago
Blocks: 833547
(Assignee)

Updated

5 years ago
Blocks: 834054
Blocks: 834611
(Assignee)

Updated

5 years ago
Blocks: 835912
(Assignee)

Updated

5 years ago
Blocks: 836150
(Assignee)

Updated

5 years ago
Blocks: 836348
(Assignee)

Updated

5 years ago
Blocks: 836355
(Assignee)

Updated

5 years ago
Blocks: 836570
(Assignee)

Updated

4 years ago
Blocks: 756397
(Assignee)

Updated

4 years ago
Blocks: 837903
(Assignee)

Updated

4 years ago
Blocks: 837956
(Assignee)

Updated

4 years ago
Blocks: 838840
(Assignee)

Updated

4 years ago
Blocks: 839269
(Assignee)

Updated

4 years ago
Blocks: 840189
(Assignee)

Updated

4 years ago
Blocks: 845117
(Assignee)

Updated

4 years ago
Blocks: 851183
(Assignee)

Updated

4 years ago
Blocks: 866983
(Assignee)

Updated

4 years ago
Blocks: 885598
(Assignee)

Updated

4 years ago
Blocks: 882859
(Assignee)

Updated

4 years ago
Blocks: 882824
(Assignee)

Updated

4 years ago
Blocks: 889099
(Assignee)

Updated

4 years ago
Blocks: 891993
(Assignee)

Updated

4 years ago
Blocks: 896292
(Assignee)

Updated

4 years ago
Blocks: 898489

Updated

4 years ago
Blocks: 908142
Blocks: 913250
(Assignee)

Updated

4 years ago
Blocks: 802490
(Assignee)

Updated

4 years ago
Blocks: 805607
(Assignee)

Updated

4 years ago
Blocks: 837313
(Assignee)

Updated

4 years ago
Blocks: 925243
(Assignee)

Updated

4 years ago
Blocks: 932303
(Assignee)

Updated

4 years ago
Blocks: 945060
(Assignee)

Updated

4 years ago
Blocks: 945063
(Assignee)

Updated

4 years ago
Blocks: 945059
(Assignee)

Updated

4 years ago
Blocks: 945151
Depends on: 945613
(Assignee)

Updated

4 years ago
Blocks: 945613
No longer depends on: 945613
(Assignee)

Updated

4 years ago
Blocks: 824247
No longer depends on: 824247
Depends on: 956126
(Assignee)

Updated

4 years ago
Blocks: 956449
(Assignee)

Updated

4 years ago
Blocks: 964077
(Assignee)

Updated

3 years ago
Blocks: 968363
(Assignee)

Updated

3 years ago
Blocks: 968491
(Assignee)

Updated

3 years ago
Blocks: 974300
(Assignee)

Updated

3 years ago
Blocks: 997982
(Assignee)

Updated

3 years ago
Blocks: 1003907
Depends on: 1007525
Depends on: 1007741
Depends on: 666646
(Assignee)

Updated

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1049738
Depends on: 1050103

Updated

3 years ago
No longer blocks: 1049738
Depends on: 1049738
(Assignee)

Updated

3 years ago
Blocks: 1049738
No longer depends on: 1049738
Depends on: 1052033
(Assignee)

Updated

3 years ago
Blocks: 1052033
No longer depends on: 1052033
(Assignee)

Updated

3 years ago
Blocks: 1068113
(Assignee)

Updated

3 years ago
Blocks: 1068977
Blocks: 1090016
Blocks: 1092001
Blocks: 1092028
Blocks: 1092710
Blocks: 1092711
Blocks: 1095878
Blocks: 1095883
Blocks: 1095926
Blocks: 1098134
Blocks: 1107814
Blocks: 1108932
Blocks: 1108934
Blocks: 1108938
No longer blocks: 1108932
No longer blocks: 1108938
Blocks: 1110031
Blocks: 1113229
Blocks: 1118149
Blocks: 1117034
No longer blocks: 1117034
Depends on: 1117034
Blocks: 1120312
Blocks: 1125690
Blocks: 1125693
(Assignee)

Updated

2 years ago
Blocks: 1126808
(Assignee)

Updated

2 years ago
Blocks: 1167250
Blocks: 1190148
Blocks: 1194580
Blocks: 1194948
Blocks: 1198334
You need to log in before you can comment on or make changes to this bug.