Closed Bug 903513 Opened 11 years ago Closed 11 years ago

Do not fail build on -Wmaybe-uninitialized when treating warnings as errors

Categories

(Firefox Build System :: General, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

Newer version of gcc/g++ (my 4.8.1 build, at least) have a warning "Wmaybe-uninitialized", which has false positives (I hit one today, causing local build bustage, in bug 862115 comment 15). I hit additional build bustage from this warning when I try to do a local opt build (since apparently their rules for what constitutes maybe-uninitialized-usage depends on the optimizations that are enabled). HISTORICAL NOTE: Per http://gcc.gnu.org/ml/gcc/2013-07/msg00177.html , it sounds like Wuninitialized was actually split into Wuninitialized and Wmaybe-uninitialized (with the latter sounding like the fuzzier more-false-positive-ish version) (though I haven't been able to find additional documentation of this). Anyway: since we exempt Wuninitialized from FAIL_ON_WARNINGS, we should presumably exempt Wmaybe-uninitialized, too.
(In reply to Daniel Holbert [:dholbert] from comment #0) > Anyway: since we exempt Wuninitialized from FAIL_ON_WARNINGS, we should > presumably exempt Wmaybe-uninitialized, too. (For reference - that change happened in bug 716541)
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #788243 - Flags: review?(gps)
Comment on attachment 788243 [details] [diff] [review] fix v1 Review of attachment 788243 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the detailed history lesson: it made granting r+ much easier! It'd be nice if configure.in contained a similar message saying it should likely be treated the same as -Wuninitialized.
Attachment #788243 - Flags: review?(gps) → review+
Thanks for the quick review! One clarification question: I don't understand what "similar message" you're asking for. Do you mean I should add a message saying that Wuninitialized and Wmaybe-uninitialized should have the same fate (so if we eventually turn one back on, we should turn the other back on too?) 'cause I'm not sure that's the case -- from my reading, it sounds like (in newer GCC versions) Wuninitialized has become more robust (and might be worth eventually re-enabling for FAIL_ON_WARNINGS), and the false-positive-ish cases have been pushed to Wmaybe-uninitialized.
In any case, I landed the patch to keep m-i buildable for folks like me on GCC 4.8 w/ --enable-warnings-as-errors: https://hg.mozilla.org/integration/mozilla-inbound/rev/b707277861fc (If you'd still like, I can add a comment as a DONTBUILD followup, once I understand what sort of comment you were looking for, per comment 4.)
Flags: needinfo?(gps)
Flags: in-testsuite-
(In reply to Daniel Holbert [:dholbert] from comment #4) > Thanks for the quick review! > > One clarification question: I don't understand what "similar message" you're > asking for. Do you mean I should add a message saying that Wuninitialized > and Wmaybe-uninitialized should have the same fate (so if we eventually turn > one back on, we should turn the other back on too?) 'cause I'm not sure > that's the case -- from my reading, it sounds like (in newer GCC versions) > Wuninitialized has become more robust (and might be worth eventually > re-enabling for FAIL_ON_WARNINGS), and the false-positive-ish cases have > been pushed to Wmaybe-uninitialized. You are talking a lot more sense than me today. Don't worry about adding a comment.
Flags: needinfo?(gps)
Cool, thanks. :)
Darn, apparently our OS X compiler (clang version whatever) doesn't have a -Wmaybe-uninitialized option, so it explodes with the current patch. { error: unknown warning option '-Werror=maybe-uninitialized'; did you mean '-Werror=uninitialized'? [-Werror,-Wunknown-warning-option] } https://tbpl.mozilla.org/php/getParsedLog.php?id=26362790&tree=Mozilla-Inbound Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/5ba1323b3806 This probably just needs the appropriate set of conditional wrappers.
Flags: needinfo?(dholbert)
Either we upgrade all the compilers or we add a configure check to only add the flag if the compiler supports it. The latter is preferred, otherwise we break a lot of local developer builds still running an old version.
I'd definitely prefer a configure check, too. The point of this is to *fix* local bustage; I don't want to introduce other bustage as collateral damage. :)
Flags: needinfo?(dholbert)
We already have the MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING macros in build/autoconf/compiler-opts.m4 for conditionally adding flags (if supported) to _WARNINGS_CFLAGS and _WARNINGS_CXXFLAGS. You'll probably need to add variants of those that do the same with WARNINGS_AS_ERRORS. Or generalize the existing macros to work with different variables, if you are an m4 hacker :) (And don't forget to update js/src/build/autoconf/compiler-opts.m4 as well.)
Depends on: 903663
Awesome! We can use those macros (no need to generify/copy them) to handle all of these exemptions. I filed bug 903663 to switch over the existing exemptions, and I'll attach a patch for this bug's exemption.
Attached patch fix v2 (obsolete) — Splinter Review
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f907df238ba I'll re-request review if that goes well.
Attached patch fix v3Splinter Review
Rebasing on top of reviewed patch from bug 903663, now using MOZ_C_SUPPORTS_WARNING() and MOZ_CXX_SUPPORTS_WARNING().
Attachment #788243 - Attachment is obsolete: true
Attachment #788439 - Attachment is obsolete: true
Attachment #789385 - Flags: review?(gps)
Attachment #789385 - Flags: review?(gps) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: