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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Gregory Szorc [: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.

Comment 4

[Daniel Holbert [:dholbert]]

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.

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Gregory Szorc [:gps]]

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

Comment 7

[Daniel Holbert [:dholbert]]

Cool, thanks. :)

Comment 8

[Daniel Holbert [:dholbert]]

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.

Comment 9

[Gregory Szorc [:gps]]

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.

Comment 10

[Daniel Holbert [:dholbert]]

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. :)

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Daniel Holbert [:dholbert]]

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.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v2

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

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

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v3

Rebasing on top of reviewed patch from bug 903663, now using MOZ_C_SUPPORTS_WARNING() and MOZ_CXX_SUPPORTS_WARNING().

Comment 15

[Daniel Holbert [:dholbert]]

Green try run, for fix v3: https://tbpl.mozilla.org/?tree=Try&rev=d08d00f6da0d

Comment 16

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cef967999c4

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6cef967999c4