1028420 - Non-unified build fails when using --enable-warnings-as-errors

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Peterson [:cpeterson]]

When building with these mozconfig settings using clang on OS X:

ac_add_options --disable-unified-compilation
ac_add_options --enable-warnings-as-errors
ac_add_options --enable-sm-fail-on-warnings

I hit the following warnings-as-error build errors:

gfx/2d/FilterNodeSoftware.cpp:2216:1: error: unused function 'ColorAtPoint' [-Werror,-Wunused-function]
gfx/layers/composite/FPSCounter.cpp:368:20: error: unused variable 'FontStride' [-Werror,-Wunused-const-variable]
gfx/layers/opengl/CompositorOGL.cpp:69:23: error: unused function 'ns2gfxSize' [-Werror,-Wunused-function]
content/media/MP3FrameParser.cpp:309:23: error: unused variable 'SAMPLES_PER_FRAME' [-Werror,-Wunused-const-variable]
layout/generic/nsBlockFrame.cpp:58:23: error: unused variable 'kCircleCharacter' [-Werror,-Wunused-const-variable]
layout/generic/nsBlockFrame.cpp:59:23: error: unused variable 'kSquareCharacter' [-Werror,-Wunused-const-variable]
extensions/spellcheck/src/mozPersonalDictionary.cpp:24:11: warning: unused variable 'kMaxWordLen' [-Wunused-const-variable]
extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:18:22: warning: unused variable 'kUniversalDetectorCID' [-Wunused-const-variable]
extensions/universalchardet/src/xpcom/nsUdetXPCOMWrapper.cpp:19:22: warning: unused variable 'kUniversalStringDetectorCID' [-Wunused-const-variable]
toolkit/components/places/nsNavHistory.cpp:105:22: warning: unused variable 'USECS_PER_DAY' [-Wunused-const-variable]
toolkit/xre/nsSigHandlers.cpp:44:18: warning: unused variable 'kClientChannelFd' [-Wunused-const-variable]
content/base/src/ShadowRoot.cpp:57:1: error: unused variable 'kDOMClassInfo_ShadowRoot_interfaces' [-Werror,-Wunused-const-variable]
layout/generic/nsSelection.cpp:3151:1: error: unused variable 'kDOMClassInfo_Selection_interfaces' [-Werror,-Wunused-const-variable]

Comment 1

[Chris Peterson [:cpeterson]]

bjacob fixed some of these non-unified build errors:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4213538b0f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/97067b95835d

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ah, thanks for filing a bug. I have some more fixes coming up.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix-non-unified

I'll land that if it's green on try (some platform-specific #ifdef'd stuff that I could have gotten wrong)

https://tbpl.mozilla.org/?tree=Try&rev=581374eda010

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Windows actually uses those -> need another try:

https://tbpl.mozilla.org/?tree=Try&rev=c90273fb0d75

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/611283da02bf

Comment 6

[Daniel Holbert [:dholbert]]

># HG changeset patch
># Parent 97067b95835d08323de4e383bcde296ce64300f9
>Bug 1028420 - Non-unified build fails when using --enable-warnings-as-errors - bustage fix, no review

IMHO, this patch is stretching the definition of "bustage fix, no review"...

I don't think we should bypass review on patches, simply because they address a build failure with a special local compiler-version combined with an special mozconfig.   That is a large class of bugs (including e.g. all future bugs w/ build warnings introduced in new compiler versions, that trigger a Werror build failure w/ those compilers).

Having said that, removing unused code is generally uncontroversial, and I trust your skills/judgement, so I'm not especially concerned in this specific case.  Just commenting on the "bustage fix, no review" labeling.

(In reply to Benoit Jacob [:bjacob] from comment #4)
> Windows actually uses those -> need another try:

I assume this is referring to the static variables at the end -- kSysMemoryParameter, kTotalVirtualMemoryParameter, etc.  Are you sure Windows uses these?  (If so, where?)

A MXR search seems to indicate that they're unused, e.g.
 http://mxr.mozilla.org/mozilla-central/ident?i=kSysMemoryParameter
...and the patch that added them did include some usages (which no longer exist):
https://hg.mozilla.org/integration/mozilla-inbound/diff/b84f528e2e10/toolkit/crashreporter/nsExceptionHandler.cpp
...and the windows failures in your Try run look unrelated to those variables ("ImportError: No module named _curses"). Seems like that was just a sporadic infra issue (or maybe an issue that affected the specific trunk revision your Try run was based on). Though I may be missing something.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> ># HG changeset patch
> ># Parent 97067b95835d08323de4e383bcde296ce64300f9
> >Bug 1028420 - Non-unified build fails when using --enable-warnings-as-errors - bustage fix, no review
> 
> IMHO, this patch is stretching the definition of "bustage fix, no review"...

OK, point taken, I think you're right, I'll ask for review next time I want to land a similar patch.

> (In reply to Benoit Jacob [:bjacob] from comment #4)
> > Windows actually uses those -> need another try:
> 
> I assume this is referring to the static variables at the end --
> kSysMemoryParameter, kTotalVirtualMemoryParameter, etc.  Are you sure
> Windows uses these?  (If so, where?)

Those are actually used in a macro that generates identifiers with ##, which is why I initially missed those too and got windows bustage on my first try push above.

Here:
http://hg.mozilla.org/integration/mozilla-inbound/file/dd07ffb3af82/toolkit/crashreporter/nsExceptionHandler.cpp#l695

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

And by the way, DXR should have no issue with ## (IIUC) but here this is #ifdef XP_WIN32 so this is out of reach of DXR too.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Benoit Jacob [:bjacob] from comment #7)
> Those are actually used in a macro that generates identifiers with ##, which
> is why I initially missed those too and got windows bustage on my first try
> push above.
> 
> Here:
> http://hg.mozilla.org/integration/mozilla-inbound/file/dd07ffb3af82/toolkit/
> crashreporter/nsExceptionHandler.cpp#l695

Ah, sneaky. Thanks for the pointer.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> ...and the windows failures in your Try run look unrelated to those
> variables ("ImportError: No module named _curses").

(d'oh, I was looking at the wrong Try run here -- I clicked on the link in comment 4 instead of comment 3. If I'd been looking at the right one, I would've figured this out on my own. :))

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/611283da02bf