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)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla26
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(1 file, 2 obsolete files)
3.18 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
(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)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #788243 -
Flags: review?(gps)
Comment 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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-
Comment 6•11 years ago
|
||
(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)
Assignee | ||
Comment 7•11 years ago
|
||
Cool, thanks. :)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(dholbert)
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
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.)
Assignee | ||
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f907df238ba
I'll re-request review if that goes well.
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Green try run, for fix v3: https://tbpl.mozilla.org/?tree=Try&rev=d08d00f6da0d
Updated•11 years ago
|
Attachment #789385 -
Flags: review?(gps) → review+
Assignee | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Updated•11 years ago
|
Depends on: Wuninitialized
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•