Closed Bug 851237 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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. :)
Component: Build Config → General
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.
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.
Attachment #725114 - Flags: review?(khuey)
Summary: Suppress or disable warnings from -Wunused-local-typedefs (which is included in -Wall in gcc 4.8) → Suppress or disable warnings from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8)
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.)
There's also a Sun Studio version here, but I can't figure out whether they support __attribute__((unused)) easily, so whatever.
Attachment #727971 - Flags: review?(dholbert)
Attachment #725114 - Flags: review?(khuey)
We've forked ipc/chromium, so this should be okay.
Attachment #727975 - Flags: review?(dholbert)
Comment on attachment 727971 [details] [diff] [review]
1 - __attribute__((unused)) up the static-assert macros

># HG changeset patch
># Parent 9dfb537207e5e0f280d5b4b7a2cadde3eebcada2
># User Jeff Walden <jwalden@mit.edu>
>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
Attachment #727971 - Flags: review?(dholbert) → review+
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 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.
Attachment #727974 - Flags: review?(dholbert) → review+
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
Attachment #727975 - Flags: review?(dholbert) → review+
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...
Whiteboard: [leave open]
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.)
Summary: Suppress or disable warnings from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8) → Address warnings from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8)
(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.
Summary: Address warnings from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8) → Address warnings-as-errors build bustage from -Wunused-local-typedefs (which is now included in -Wall in gcc 4.8)
Depends on: 855010
(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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Assignee: nobody → jwalden+bmo
You need to log in before you can comment on or make changes to this bug.