Closed Bug 1008083 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)
This is with clang 3.5, on 64-bit linux, BTW.
(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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8420174 - Flags: review?(ehsan)
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.)
(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.)
(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>
Yeah, that seems plausible.  Wanna file a clang bug for that?  :-)
Attachment #8420174 - Flags: review?(ehsan) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d823a184bfb2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.