851237 - Address warnings-as-errors build bustage from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8)

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Building mozilla-central with GCC 4.8 (from the "gcc-snapshot" package in Ubuntu) triggers a ton of -Wunused-local-typedefs build warnings, from static asserts like MOZ_STATIC_ASSERT and "js::tl::StaticAssert".

These ultimately make my --enable-warnings-as-errors build fail, but they're also really spammy even if I don't build with that mozconfig line.

The warnings look like:
{
 5:57.61 Warning: -Wunused-local-typedefs in /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h: typedef 'moz_static_assert91' locally defined but not used
 5:57.62 /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h:1081:21: warning: typedef 'moz_static_assert91' locally defined but not used [-Wunused-local-typedefs]
 5:57.62          MOZ_STATIC_ASSERT(offsetof(ObjectImpl, type_) == offsetof(shadow::Object, type),
 5:57.62                      ^
 5:57.62 Warning: -Wunused-local-typedefs in /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h: typedef 'moz_static_assert92' locally defined but not used
 5:57.62 /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h:1083:21: warning: typedef 'moz_static_assert92' locally defined but not used [-Wunused-local-typedefs]
 5:57.62          MOZ_STATIC_ASSERT(offsetof(ObjectImpl, slots) == offsetof(shadow::Object, slots),
 5:57.62                      ^
 5:57.62 Warning: -Wunused-local-typedefs in /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h: typedef 'moz_static_assert93' locally defined but not used
 5:57.62 /scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/mozilla/js/src/vm/ObjectImpl.h:1085:21: warning: typedef 'moz_static_assert93' locally defined but not used [-Wunused-local-typedefs]
 5:57.62          MOZ_STATIC_ASSERT(offsetof(ObjectImpl, elements) == offsetof(shadow::Object, _1),
}

and
{
12:18.13 ../../dist/include/js/RootingAPI.h:174:85: error: typedef '_' locally defined but not used [-Werror=unused-local-typedefs]
12:18.13          typedef typename js::tl::StaticAssert<mozilla::IsPointer<T>::value>::result _;
}

Filing this bug on either turning off this build warning, or adding no-op "usages" of these typedefs so that there's nothing to warn about.  Anything that'll stop the buildspam. :)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: strawman patch v1: Disable warning

Not sure if we want to disable the warning[1], but here's a patch to disable it, as one possibility (and a workaround for warnings-as-errors builds, for the moment).

[1] Waldo suggested in IRC that this warning might be useful to leave turned on, particularly if most of its current instances are from macros & could be silenced by adding a "usage" of the typedef to the macro.

Comment 2

[Daniel Holbert [:dholbert]]

After a week of building w/ GCC 4.8 and having to qpush my attached patch every time: I think we should take the attached patch to turn off this warning, and track actual warning-fixes in followups, because:

 a) We haven't built with this warning since forever, so we spam tons of instances of it (and it breaks "--enable-warnings-as-errors" builds as a result)

 b) It's a fairly harmless warning, and in any "real" cases where it's fired (i.e. not for static assertions), it probably just indicates dead code -- it's very unlikely to point to an issue that actually affects behavior.

If either of (a) or (b) were false, then I'd be more in favor of keeping this warning on. But as it stands, I think it's not really useful to have it on, and we should just turn it off and file a followup on fixing our static assert implementations etc. to not run afoul of it.

Comment 3

[Jeff Walden [:Waldo]]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33255 was the gcc bug adding this, and pointed out a case where an unused local typedef was a bug for them.  (Someone used the wrong typename elsewhere, leaving the actually-meant-to-be-used typedef unused.)

Comment 4

[Jeff Walden [:Waldo]]

Attachment: 1 - __attribute__((unused)) up the static-assert macros

There's also a Sun Studio version here, but I can't figure out whether they support __attribute__((unused)) easily, so whatever.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 2 - Remove the StaticAssert duplicate-functionality, replace it with MOZ_STATIC_ASSERT

One way to do it.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 3 - Use MOZ_STATIC_ASSERT in an ipc/chromium header that was using a typedef to static-assert

We've forked ipc/chromium, so this should be okay.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 727971 [details] [diff] [review]
1 - __attribute__((unused)) up the static-assert macros

># HG changeset patch
># Parent 9dfb537207e5e0f280d5b4b7a2cadde3eebcada2
># User Jeff Walden <[e-mail]>
>Bug 851237 - Mark the static-assert typedef with an unused attribute so it doesn't trigger compiler warnings.  NOT REVIEWED YET
>
>diff --git a/mfbt/Assertions.h b/mfbt/Assertions.h
>--- a/mfbt/Assertions.h
>+++ b/mfbt/Assertions.h
>@@ -76,6 +76,11 @@
> #  endif
> #endif
> #ifndef MOZ_STATIC_ASSERT
>+#  if defined(__GNUC__)
>+#    define MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE __attribute__((unused))

This would benefit from a comment. There are several layers of macros here, with their definitions in different places, so it's not clear from casual inspection why __attribute__(unused) might be useful here.

r=me with a brief explanatory comment there

Comment 8

[Daniel Holbert [:dholbert]]

Also, the commit messages might benefit from "part 1:" "part 2:" etc. when this eventually lands, to make it clear that it's a multi-patch bug.  (and to make it easier to trace the eventual hg changeset back to an actual patch file attached to this bug page)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 727974 [details] [diff] [review]
2 - Remove the StaticAssert duplicate-functionality, replace it with MOZ_STATIC_ASSERT

>diff --git a/js/public/TemplateLib.h b/js/public/TemplateLib.h
[...]
> /*
>  * Produce an N-bit mask, where N <= BitSize<size_t>::result.  Handle the
>  * language-undefined edge case when N = BitSize<size_t>::result.
>  */
> template <size_t N> struct NBitMask {
>-    typedef typename StaticAssert<N < BitSize<size_t>::result>::result _;
>+    static const size_t checkPrecondition = 0 / (N < BitSize<size_t>::result);
>     static const size_t result = (size_t(1) << N) - 1;

Add a comment above the "assertion" here, explaining that this is an assertion and how we're asserting (triggering a divide-by-0 compile error if the condition fails).

Also, as a sanity-check, could you tweak a client of this class to have a too-large value of "N", and verify that it your faux-static-assert correctly fires? (if you haven't already)

r=me with that.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 727975 [details] [diff] [review]
3 - Use MOZ_STATIC_ASSERT in an ipc/chromium header that was using a typedef to static-assert

> We've forked ipc/chromium, so this should be okay.

r=me, given that

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9455756963a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/af13119deb47
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f162f54f29d
https://hg.mozilla.org/integration/mozilla-inbound/rev/073556b91f46

dholbert tells me there's one more of these in Breakpad code, which has been fixed upstream.  We need to update Breakpad for asm.js, so presumably we'll pick up that fix then.  Keeping this open til that lands...

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 727975 [details] [diff] [review]
3 - Use MOZ_STATIC_ASSERT in an ipc/chromium header that was using a typedef to static-assert

Review of attachment 727975 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/basictypes.h
@@ +294,3 @@
>  inline Dest bit_cast(const Source& source) {
>    // Compile time assertion: sizeof(Dest) == sizeof(Source)
>    // A compile error here means your Dest and Source have different sizes.

(Could have removed this comment.)

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9455756963a4
https://hg.mozilla.org/mozilla-central/rev/af13119deb47
https://hg.mozilla.org/mozilla-central/rev/8f162f54f29d
https://hg.mozilla.org/mozilla-central/rev/073556b91f46

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> dholbert tells me there's one more of these in Breakpad code, which has been
> fixed upstream.  We need to update Breakpad for asm.js, so presumably we'll
> pick up that fix then.  Keeping this open til that lands...

Correct -- breakpad header issue breaks warnings-as-errors builds because we #include that header in tools/profiler, which is a FAIL_ON_WARNINGS directory.

Depending on when the breakpad update comes, we might want to just make that FAIL_ON_WARNINGS annotation conditional on GCC version, or turn it off, pending the breakpad upgrade.

In other news, I filed bug 853874 to turn on this build warning for all GCC versions, so that people using old-gcc won't accidentally introduce more instances of this warning that break builds for people with new-gcc.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> dholbert tells me there's one more of these in Breakpad code,

I spun this off into its own bug: bug 855010.

> which has been fixed upstream.

To be clear, I have no idea whether it's been fixed upstream, but I recall someone saying in IRC that it's fixed upstream.

In any case, I think we can fix this by just avoiding the warning-spammy breakpad header -- I posted a patch to do that over on bug 855010.  Once that's resolved, we can close both that bug & this bug, and we can move forward on bug 853874 to prevent more of these warnings from cropping up.

Comment 16

[Daniel Holbert [:dholbert]]

Bug 855010 is now fixed, which was the last instance of this warning that was busting builds with GCC 4.8 and --enable-warnings-as-errors.

So, this bug here can be resolved.