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

RESOLVED FIXED in mozilla26

Status

RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla26
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

5 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

5 years ago
Created attachment 788243 [details] [diff] [review]
fix v1
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+
(Assignee)

Comment 4

5 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

5 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-
(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

5 years ago
Cool, thanks. :)
(Assignee)

Comment 8

5 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

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

Comment 10

5 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)
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)

Updated

5 years ago
Depends on: 903663
(Assignee)

Comment 12

5 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

5 years ago
Created attachment 788439 [details] [diff] [review]
fix v2

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f907df238ba

I'll re-request review if that goes well.
(Assignee)

Comment 14

5 years ago
Created attachment 789385 [details] [diff] [review]
fix v3

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+
https://hg.mozilla.org/mozilla-central/rev/6cef967999c4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 1001975

Updated

10 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.