Closed Bug 903663 Opened 6 years ago Closed 6 years ago

Use MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING to exempt warnings from FAIL_ON_WARNINGS

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch fix v1 (obsolete) — Splinter Review
I think this should do it.
Attached patch fix v2 (obsolete) — Splinter Review
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.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2f907df238ba

I'll request review if that goes well.
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.
(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? :) )
(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.)
Attachment #788434 - Flags: review?(mh+mozilla)
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)
Attachment #788434 - Flags: review?(mh+mozilla) → review+
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.
Attached patch fix v3Splinter Review
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.
Attachment #788425 - Attachment is obsolete: true
Attachment #788434 - Attachment is obsolete: true
Attachment #789377 - Flags: review?(mh+mozilla)
Attachment #789377 - Flags: review?(mh+mozilla) → review+
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
https://hg.mozilla.org/mozilla-central/rev/b3299b797d8d
Status: ASSIGNED → RESOLVED
Closed: 6 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.