713531 - Investigate C11 _Static_assert compiler support, and make MOZ_STATIC_ASSERT expand to it when possible to do so without warnings

Status: RESOLVED WONTFIX

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

C++11 includes static_assert(cond, reason) syntax to implement static assertions.  MOZ_STATIC_ASSERT expands to this in compilers that support it.  C11 (the next edition of C) includes functionally identical _Static_assert syntax.  Someone (not me) should investigate compiler support for _Static_assert -- particularly for when it can be used without triggering a warning (note that in my cursory investigation clang's docs may be wrong about it triggering a warning except in -std=c11 code, not sure if I'm misreading or either docs or code have a bug) -- and make MOZ_STATIC_ASSERT expand to it when possible.

Comment 1

[Arun [:arunsl]]

Would like to take this up

Comment 2

[Brian Barnes]

After a bit of research, it looks like GCC and Clang support the _Static_assert macro, but MSVC does not. I have yet to test it in any compiler, so this is simply the result of an hour or so of searching.

Comment 3

[Brian Barnes]

I've gotten _Static_assert to run on Clang, but as I suspected, it does not appear to work in MSVC. GCC quits on a "not defined in this scope" error for both _Static_assert and static_assert. I suspect that this is due to a faulty GCC setup on Windows. I'm nearly a total newbie, so that's entirely likely.

Additionally, could I be marked as the assignee of this bug?

Comment 4

[Brian Barnes]

Attachment: Patch to use _Static_assert in clang

I haven't used Mercurial before, so I'm not sure if I created this patch correctly. Please let me know if it's messed up in any way.

Comment 5

[Jeff Walden [:Waldo]]

I'll take a look at this shortly.

Comment 6

[Jeff Walden [:Waldo]]

Hmm, this patch is indeed a bit weird.  It looks like it applies to a file that has _Static_assert uses in it already.  And it's reverting __GNUC__-conditioned _Static_assert uses to the current system for that compiler, that doesn't use _Static_assert.  Maybe this is a diff against a patch you'd created, and not against an unmodified tree?  I'd suggest asking on irc.mozilla.org in the #introduction channel for Mercurial help, to see if you can get realtime help creating a fully-properly-generated patch.

Comment 7

[Josh Matthews [:jdm]]

Brian, could you confirm that you're still working on this?

Comment 8

[Brian Barnes]

Yes, I am still working on this. I haven't had a chance to look at it in a while due to school, but will upload a new patch as soon as I can get my Mercurial setup figured out.

Comment 9

[Mike Hoye [:mhoye]]

OK, I think that I need to throw this one back in the pool.

Comment 10

[Emma Benoit]

Attachment: Edited mtbf/Assertions.h to expand MOZ_STATIC_ASSERT to _Static_assert or static_assert with clang, gcc and MSC

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8443926 [details] [diff] [review]
Edited mtbf/Assertions.h to expand MOZ_STATIC_ASSERT to _Static_assert or static_assert with clang, gcc and MSC

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

::: mfbt/Assertions.h
@@ +99,5 @@
> +#    endif
> +#    if __has_extension(c_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    elif __has_extension(cxx_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)

Given that we're in a !__cplusplus block, I think these two lines can/should be removed.

@@ +108,5 @@
> +#  elif defined(__GNUC__)
> +#    if MOZ_GCC_VERSION_AT_LEAST(4, 6, 0)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    elif MOZ_GCC_VERSION_AT_LEAST(4, 3, 0)
> +#      define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)

This too.

@@ +114,5 @@
> +#      define MOZ_STATIC_ASSERT(cond, reason) \
> +          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE
> +#    endif
> +#  elif defined(_MSC_VER)
> +#      if MSC_VER >= 1600 /* MSVC 10 */

_MSC_VER typo

@@ +118,5 @@
> +#      if MSC_VER >= 1600 /* MSVC 10 */
> +#         define MOZ_STATIC_ASSERT(cond, reason) static_assert((cond), reason)
> +#      else
> +#          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE
> +#      endif

This bit was mis-indented -- should be

#  elif defined(_MSC_VER)
#    if _MSC_VER >= 1600 /* MSVC 10 */
#      define MOZ_STATIC_ASSERT(cond, reason), static_assert((cond), reason)
#    else
#      extern void...
#    endif
#  elif defined(__COUNTER__)

Comment 12

[Jeff Walden [:Waldo]]

Also: now that it's landed, please use MOZ_MSVC_VERSION_AT_LEAST() for the MSVC version check.  So much more readable!

Comment 13

[Masatoshi Kimura [:emk]]

Does MSVC10+ support static_assert in C-mode?

Comment 14

[Emma Benoit]

I will update the patch as soon as I can.

@emk: I found this http://msdn.microsoft.com/en-us/library/bb918086%28v=vs.90%29.aspx which seems to indicate that it does.

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Emma Benoit from comment #14)
> I will update the patch as soon as I can.
> 
> @emk: I found this
> http://msdn.microsoft.com/en-us/library/bb918086%28v=vs.90%29.aspx which
> seems to indicate that it does.

I couldn't find any explanation from that MSDN article about neither C++99 static_assert nor C11 _Static_assert. The _STATIC_ASSERT macro uses a trick to implement static assertion for older compilers.

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> Does MSVC10+ support static_assert in C-mode?

MSVC10+ doesn't even support C-mode.  :-)  The C support they have is, as a matter of policy, largely incident to their support for C++.  No reason to do anything for MSVC for this bug.

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> MSVC10+ doesn't even support C-mode.  :-)

It does. It will compile the source files with .c extension in C-mode. It also has the -TC option to force compile sources in C-mode.

> The C support they have is, as a
> matter of policy, largely incident to their support for C++.  No reason to
> do anything for MSVC for this bug.

MSVC doesn't recognize the inline keyword in C-mode. I suspect the static_keyword is the same.

Comment 18

[Masatoshi Kimura [:emk]]

s/static_keyword/static_assert keyword/;

Comment 19

[Emma Benoit]

Attachment: mfbt/Assertions.h: expand MOZ_STATIC_ASSERT with clang and gcc

Updated, I removed the MSVC part.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8446150 [details] [diff] [review]
mfbt/Assertions.h: expand MOZ_STATIC_ASSERT with clang and gcc

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

Looks reasonable.  Make the one change and post for review, and I can land it for you.

::: mfbt/Assertions.h
@@ +100,5 @@
> +#    if __has_extension(c_static_assert)
> +#      define MOZ_STATIC_ASSERT(cond, reason) _Static_assert((cond), reason)
> +#    else
> +#      define MOZ_STATIC_ASSERT(cond, reason) \
> +          extern void MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)(int arg[(cond) ? 1 : -1]) MOZ_STATIC_ASSERT_UNUSED_ATTRIBUTE

Rather than copy this version, please copy the __COUNTER__ version defined below.  Also use it in the __GNUC__ case as well.  Ideally we'd have this code fall through into the actual __COUNTER__ version below, but unfortunately this

#if  defined(TEST_MACRO) && MACRO_DEFINED_ONLY_IF_TEST_MACRO_DEFINED()

doesn't short-circuit, because of how preprocessing works, so nesting and duplication like this is the most practical solution.

Comment 21

[Jeff Walden [:Waldo]]

We removed MOZ_STATIC_ASSERT some time ago in favor of just using C++11 static_assert directly. And we don't really have that much pure C code at all any more (and where possible I try to push back on our adding any more), so there just isn't a need for this any more.

Comment 22

[Chris Peterson [:cpeterson]]

(In reply to Jeff Walden [:Waldo] from comment #21)

We removed MOZ_STATIC_ASSERT some time ago in favor of just using C++11 static_assert directly. And we don't really have that much pure C code at all any more (and where possible I try to push back on our adding any more), so there just isn't a need for this any more.

MOZ_STATIC_ASSERT is currently used only in libmar C code:

https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/modules/libmar/src/mar.h#27

https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/modules/libmar/src/mar_private.h#30-31,35-37

I have a patch to replace libmar's MOZ_STATIC_ASSERTs with static_assert (C11's macro #defined in assert.h as C11's _Static_assert keyword), but that requires compiling mozilla-central's C code as C11 instead of C99 (-std=gnu99). And that, unsurprisingly, opens more rat holes. I have a WIP on my back burner but no ETA.