1008083 - In disable-unified warnings-as-errors build: "HangMonitor.cpp:39:12: error: unused variable 'kTelemetryPrefName' [-Werror,-Wunused-const-variable]"

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
Build with this mozconfig:
ac_add_options --enable-debug --disable-optimize
ac_add_options --enable-warnings-as-errors
ac_add_options --disable-unified-compilation

ACTUAL RESULTS: Build error:
xpcom/threads/HangMonitor.cpp:39:12: error: unused variable 'kTelemetryPrefName' [-Werror,-Wunused-const-variable]
 0:53.87 const char kTelemetryPrefName[] = "toolkit.telemetry.enabled";


There might be another mozconfig option involved; I'll poke more at this tomorrow.  However, the upshot is that this variable is only used inside of an #ifdef REPORT_CHROME_HANGS clause, so it should be defined inside of a #ifdef REPORT_CHROME_HANGS clause, too. (Perhaps even as a static local variable alongside its usage, to minimize #ifdef blocks)

Comment 1

[Daniel Holbert [:dholbert]]

This is with clang 3.5, on 64-bit linux, BTW.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> There might be another mozconfig option involved

(Verified that the options in comment 0 are sufficient to trigger this for me. Though FWIW to build successfully I'm also combining them with --with-system-libvpx to work around bug 982693 and --without-intl-api to hack around some ICU build error that I'm hitting.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: fix v1: Add #ifdef wrappers

Comment 4

[Daniel Holbert [:dholbert]]

For reference, there are two usages of this variable, and they're both inside this same type of #ifdef, here:
 http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp?rev=e2c046c72d6c#74
and here:
 http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/HangMonitor.cpp?rev=e2c046c72d6c#258

(aside: I'm not sure why we don't hit this in unified builds; maybe because some straggling state from a previous file results in us turning on REPORT_CHROME_HANGS in unified builds, when it's off in disable-unified builds. I'll poke a bit more & file a separate bug on that, if that's ends up being the case.)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (aside: I'm not sure why we don't hit this in unified builds; maybe because
> some straggling state from a previous file results in us turning on
> REPORT_CHROME_HANGS in unified builds, when it's off in disable-unified
> builds. I'll poke a bit more & file a separate bug on that, if that's ends
> up being the case.)

(Fortunately, that's not the case -- REPORT_CHROME_HANGS is indeed consistently turned off in both unified & non-unified builds on my system.  And yet, we just only warn about the unused const variable in non-unified builds. I tried deleting the variable (in a unified build) to see if that would trigger a build error from some other usage that I was missing, but it still compiled fine.  So the variable is indeed unused in both configurations, and clang only warns in one of them. This is strange, but I'm not going to worry about it much more.)

Comment 6

[Daniel Holbert [:dholbert]]

(Oh, I'll bet the unified/non-unified distinction is that we're #including this source file instead of directly compiling it, and clang probably doesn't complain about unused const variables in #included files, since there could be usages in other #includers. Or something like that.)
</aside>

Comment 7

[(no longer active)]

Yeah, that seems plausible.  Wanna file a clang bug for that?  :-)

Comment 8

[(no longer active)]

(In reply to comment #7)
> Yeah, that seems plausible.  Wanna file a clang bug for that?  :-)

Trevor mentioned on IRC that the compiler can't actually warn about these, and he's right on.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Carsten Book [:Tomcat]]

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