903663 - Use MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING to exempt warnings from FAIL_ON_WARNINGS

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Daniel Holbert [:dholbert]]

We currently exempt gcc/clang warnings from FAIL_ON_WARNINGS by directly passing "-Wno-error=whatever".  (See patches from bug 716541, bug 833405 for example)

This is bad in cases where the warning only exists in clang, or only exists in GCC, or only exists in certain versions.  If it's unsupported, then the compiler will crap out if we pass "-Wno-error=something-unsupported", as I discovered in bug 903513 comment 8.

GOOD NEWS: as njn hinted at, we can use MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING to test if the warning is supported before exempting it.

Filing this bug on switching our current exemptions to use that strategy.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

I think this should do it.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Actually, this is marginally more straightforward. My previous patch had:
  MOZ_CXX_SUPPORTS_WARNING(-Wno-, error=
and this new patch has: 
  MOZ_CXX_SUPPORTS_WARNING(-W, no-error=

During configuration, we do a test compile with "-W{the second arg}", and if it that succeeds, we go ahead and stick {first arg}{second arg} into _WARNINGS_CXXFLAGS.  A few existing clients of this macro try to be cute and stick the "-Wno-" as the first argument, and I cargo-culted that pattern, but I don't think it buys us anything here. We might as well try to actually build with e.g. "-Wno-error=uninitialized" (instead of with "-Werror=uninitialized") in our trial build, as a more-accurate test of whether it's going to succeed in our real build.

Comment 3

[Daniel Holbert [:dholbert]]

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

I'll request review if that goes well.

Comment 4

[Nicholas Nethercote [inactive]]

Perhaps I'm missing something...  MOZ_C_SUPPORTS_WARNING directly modifies _WARNINGS_CFLAGS.  So for the GNUC case you're no longer using WARNINGS_AS_ERRORS.  But WARNINGS_AS_ERRORS is still used for other compilers.  The inconsistency is unfortunate.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> So for the GNUC case you're no longer using
> WARNINGS_AS_ERRORS.

Partly correct. You're right that I'm no longer using it *for the purpose of exempting specific warnings* from being treated as errors anymore.

But I *am* using it for its original purpose -- to store the compiler-specific "warnings-as-errors" flag ("-Werror" in this case).

> But WARNINGS_AS_ERRORS is still used for other
> compilers.  The inconsistency is unfortunate.

It's still used to store the compiler-specific "warnings-as-errors" flag, yes, for GCC and for other compilers (MSVC in particular).

Are you saying it's also still used for *exempting* warnings? I don't think it is... (or at least, I'm not seeing that from some quick MXR searching.

Anyway -- I'm missing the inconsistency you're referring to. (or perhaps you were right that you were missing something, and now I've cleared things up? :) )

Comment 6

[Daniel Holbert [:dholbert]]

(One slight self-criticism of my current patch: it'll make us build with these "-Wno-error=uninitialized" etc. compile flags even in non-FAIL_ON_WARNINGS directories, and in non-warnings-as-errors builds, because the patch ends up directly modifying _WARNINGS_{CFLAGS|CXXFLAGS} which get used everywhere.

I think the only way to get a more targeted usage of these flags would be to append them to WARNINGS_AS_ERRORS, as we currently do.  So we'd need a modified version of MOZ_C_SUPPORTS_WARNING that targets that variable instead of _WARNINGS_CFLAGS.

Having said that, I'm not particularly worried about this; these flags should have zero effect in non-warnings-as-errors builds/directories, so it's not harmful to have them there.  And it makes some sense to conceptually group them with the other warnings that we disable in _WARNINGS_CFLAGS.)

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 788434 [details] [diff] [review]
fix v2

Review of attachment 788434 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +1335,5 @@
> +    # -Wdeprecated-declarations - we don't want our builds held hostage when a
> +    #   platform-specific API becomes deprecated.
> +    #
> +    MOZ_C_SUPPORTS_WARNING(-W, no-error=uninitialized, ac_c_has_noerror_uninitialized)
> +    MOZ_C_SUPPORTS_WARNING(-W, no-error=deprecated-declarations, ac_c_has_noerror_deprecated_declarations)

Maybe only do that when MOZ_ENABLE_WARNINGS_AS_ERRORS is set? (which requires moving the block defining it)

Comment 8

[Daniel Holbert [:dholbert]]

Good call; that basically addresses my self-criticism in comment 6.  I'll fix that, give it a sanity Try run, and land this if all looks good.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v3

OK, here's an updated patch with the MOZ_C*_SUPPORTS_WARNING() statements in an "elif" clause after we check for the mozconfig flag.

Requesting review again, to be on the safe side.

Comment 10

[Daniel Holbert [:dholbert]]

Thanks!

I verified locally that FAIL_ON_WARNINGS still gets enforced, and that these targeted "no-error" requests are honored. 

Linux/Mac/Win try run: https://tbpl.mozilla.org/?tree=Try&rev=c36d9ae78bf1

Comment 11

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3299b797d8d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3299b797d8d