Closed Bug 1081208 Opened 10 years ago Closed 10 years ago

No longer possible to build on Mac while using ccache without CCACHE_CPP2

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox34 unaffected, firefox35 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- unaffected
firefox35 --- fixed

People

(Reporter: bzbarsky, Assigned: cpeterson)

References

Details

(Keywords: regression)

Attachments

(1 file)

I have clang symlinked to ccache, and have been building in that configuration, without CCACHE_CPP2 (for slightly faster builds) for a while now.

Bug 1076698 makes this configuration no longer work, since it forces errors on warnings that clang emits only when called on already-preprocessed source.  Specifically:

 0:31.44 In file included from Unified_cpp_xpcom_glue1.cpp:184:
 0:31.44 /Users/bzbarsky/mozilla/vanilla/mozilla/xpcom/glue/pldhash.cpp:612:31: error: equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
 0:31.44         if (((entry)->keyHash == 1)) {

It would have been nice to at least have a heads-up about the change in behavior if it was intended, so I didn't have to waste a while bisecting this and tracking this down.  If it wasn't intended, then we should fix this, no?
In particular, as a documentation matter, building on Mac with ccache is no longer supported unless --with-ccache is used in the mozconfig, which forces on CCACHE_CPP2 when building with clang.  The tinderbox builders do use this configure option, which is why things were green there.
Sorry, bz. I had not tested that configuration.

Do you build with `ac_add_options --enable-warnings-as-errors`? -Wparentheses is part of -Wall, so FAIL_ON_WARNINGS directories would already be susceptible to that macro expansion problem even before bug 1076698.

As a quick fix, I can remove the -Werror=parentheses flag. "Extraneous" parentheses will be common for preprocessed source. I can then fix configure.in's tree-wide -Werror flags honor --enable-warnings-as-errors.
Assignee: nobody → cpeterson
> Do you build with `ac_add_options --enable-warnings-as-errors`?

No, precisely because it didn't build without enabling CCACHE_CPP2.

> I can then fix configure.in's tree-wide -Werror flags honor --enable-warnings-as-errors.

That would be awesome, thanks!
I've recall noticing this warning & briefly investigating it (& discovering that it's an innocuous result of preprocessing), at some point in the past few months. The mozconfig --enable-warnings-as-errors option doesn't make it fatal, because xpcom/glue isn't tagged as FAIL_ON_WARNINGS.

(I've never symlinked ccache, so I believe I just hit this in a --with-ccache build.)

I'd support dropping the global "Werror=parenthesis" usage, since extra parenthesis are innocuous & can be caused by preprocessing (as is the case here), generated code, & similar.
There's a similar error in ICU, which I assume derives from macros but haven't checked:

/home/jwalden/moz/slots/intl/icu/source/common/ucnv_u16.c:1358:38: error: equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
                if(((cnv)->sharedData==&_UTF16LEData_52)) {
                    ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
/home/jwalden/moz/slots/intl/icu/source/common/ucnv_u16.c:1358:38: note: remove extraneous parentheses around the comparison to silence this warning
                if(((cnv)->sharedData==&_UTF16LEData_52)) {
                   ~                 ^                 ~
/home/jwalden/moz/slots/intl/icu/source/common/ucnv_u16.c:1358:38: note: use '=' to turn this equality comparison into an assignment
                if(((cnv)->sharedData==&_UTF16LEData_52)) {
                                     ^~
                                     =

I'd personally prefer we fixed these by not adding warning flags for third-party code in the tree.  It seems a bit crazy that in the process of mixing in our CFLAGS/CXXFLAGS, we also mix in a bunch of warning flags (and ones triggering errors, even) that may or may not correspond with those projects' policies.  *This* case is probably because of ccache boiling away some preprocessor macros, but there's no reason plain old ICU source code (or any other third-party code in the tree) might not over-parenthesize on its own.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> I'd personally prefer we fixed these by not adding warning flags for
> third-party code in the tree.  It seems a bit crazy that in the process of
> mixing in our CFLAGS/CXXFLAGS, we also mix in a bunch of warning flags (and
> ones triggering errors, even) that may or may not correspond with those
> projects' policies.

I am working on a patch to mark directories as third-party code so we can avoid mixing CFLAGS/CXXFLAGS.
Disable -Wparentheses-equality warnings because extra parens are expected in preprocessed files.
Attachment #8503347 - Flags: review?(bzbarsky)
Comment on attachment 8503347 [details] [diff] [review]
Wno-parentheses-equality.patch

r=me insofar as I can review build system stuff
Attachment #8503347 - Flags: review?(bzbarsky) → review+
hmm. I need to rework this fix because Try's clang and gcc claim to not understand the -Wno-parentheses-equality flag, even though my local clang and (Android) gcc do.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a84d040eb188
The MOZ_C_SUPPORTS_WARNING / MOZ_CXX_SUPPORTS_WARNING macros should be able to help with that. (They check whether the compiler supports a particular warning-related command-line-arg, by running a quick test, and they'll it to your build flags IFF it does.)
Also, I don't think we want -Wno-parentheses-equality -- we really want -Wno-error=parentheses-equality.  (I think we *do* want to let the compiler warn about it, because it's useful to notice if you happen to trigger it in your patches, and there's hardly any background noise of spurious failures.  We just don't want to treat it as an error, because of that (small, nonzero) background noise.)
Bluntly, I don't trust people to "notice if you happen to trigger it in your patches".  If I introduced such in a file outside the JS engine (inside JS I keep a closer eye on compiler output), there's a solid possibility I'd miss it simply because I'm rarely watching the terminal when compiling.  I don't have much hope that anyone else would do much better.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> Bluntly, I don't trust people to "notice if you happen to trigger it in your
> patches"

(Nor do I, but if we suppress the warning entirely with -Wno, as in the current patch, then nobody will have a chance to see that there's a problem. Per IRC, it sounds like you agree that this warning is useful; all I'm saying in comment 11 is that it'd be better to keep reporting it, non-fatally (with -Wno-error=), than to completely silence it (with -Wno-).)
I removed -Werror=parentheses for now. I will revisit questions of reenabling with -Wno- this or -Wno-error that in another bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/22e19b0597e6
Sorry again for the wild goose chase. I hadn't seen any problems when I tested on Try's clang and gcc and bleeding-edge clang and Android gcc.
https://hg.mozilla.org/mozilla-central/rev/22e19b0597e6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1090088
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.