712129 - Various assertion improvements for MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Move all the assertion stuff to mfbt/Assertions.h

The assertion stuff is going to grow beyond just JS_Assert (ugh) and MOZ_ASSERT to include more assertion and static-assertion macros.  Util.h is pretty grab-baggy right now; let's not have it get worse.  :-)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Move the MOZ*INLINE macros to Attributes.h, since that's what they are

This is not strictly assertion-related, but it's necessary so that some of the other mfbt headers can have narrower include sets, which keeps build times down across changes to all these files.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Move various assertion flavors into mfbt from the JS engine: MOZ_NOT_REACHED, MOZ_ASSERT_IF, MOZ_ALWAYS_TRUE, MOZ_ALWAYS_FALSE

I recently learned about the __builtin_unreachable() GCC/clang builtin, and about MSVC's __assume() builtin.  It's tempting to apply these here, now.  But __assume(0) at least causes "dead" code to be folded away, and I know the decompiler's LOCAL_ASSERT would be sad if that happened.  So we can't just add it, yet.  More investigation needed.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Implement MOZ_STATIC_ASSERT and MOZ_STATIC_ASSERT_IF

These are mostly a copy from the JS engine, but not entirely.  I tried to fold in the discussion in bug 621201, first.  Also, keeping in mind that this header would like to be C and C++-compatible, there's more complexity in checking before using static_assert() and similar.

C++11's static_assert takes two arguments, the condition to test, and a message.  It may be worth switching to the two-argument form here at some point.  But we have existing code now, and it's a bit troublesome to change just now.  So this leaves things as-is.  Maybe in the future we can improve MOZ_STATIC_ASSERT to take one or two arguments, then let people slowly migrate, then remove the one-argument form.

Comment 5

[Jeff Walden [:Waldo]]

Oh, the MOZ_STATIC_ASSERT patch includes the relevant bits from bug 621201, since I'm going to the trouble of touching all this so much.

Comment 7

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

Comment on attachment 582958 [details] [diff] [review]
Move various assertion flavors into mfbt from the JS engine: MOZ_NOT_REACHED, MOZ_ASSERT_IF, MOZ_ALWAYS_TRUE, MOZ_ALWAYS_FALSE

>+/*
>+ * MOZ_ALWAYS_TRUE(expr) and MOZ_ALWAYS_FALSE(expr) always evaluate the provided
>+ * expression, in debug builds and in release builds both.  Then, in debug
>+ * builds only, the value of the expression is asserted either true or false

truthy/falsy, maybe?

Comment 8

[(not currently active) Ted Mielczarek]

Given the low number of consumers you changed in the MOZ_STATIC_ASSERT patch, don't you think it's worth just adding the second parameter now? Even if you don't propogate it up to JS_STATIC_ASSERT, it still seems like a win.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: MOZ_STATIC_ASSERT, v2

So, while the previous patch compiled locally for me with Clang, it seems to Have Issues Still.  Specifically:

  extern "C" {
  MOZ_STATIC_ASSERT('A' == 'A');
  }
  MOZ_STATIC_ASSERT('A' == 'A');

In compilers where that compiles to the extern, it's an error, because the two have different linkage (and extern "C" blocks don't introduce new namespaces).  I've been trying to hack around this, and it's pretty awful.  Basically I've given up on having non-bifurcated C/C++ definitions here.  Now it's #ifdef __cplusplus / define it for C++ / #else / define it for C / #endif.  Which is not necessarily so bad, but it multiplies the problem a bit in complexity.

Nonetheless, drawing from past precedents (even ones abandoned in bug 381236 because we were C at the time and C is Dumb), I think I have it all down.  Except for (seemingly) one last problem: js::HashTable::staticAsserts.  For reasons I sort of maybe kinda grok but not entirely, it seems replacing staticAsserts with this causes compile errors with both Clang and gcc -- "error: typedef redefinition with different types ('int [sInvMaxAlpha / sInvMaxAlpha]' vs 'int [sMaxInit / sMaxInit]')":

  void staticAsserts() {
    typedef int m[sInvMaxAlpha / sInvMaxAlpha];
    typedef int m[sMaxInit / sMaxInit];
  }

I could believe this is from something template-expansiony, but it's one expansion, and only in a constexpr, so this is funky.  Is there some actual reason why these typedefs should conflict in the standard?  And what can you think of as a workaround for this?  I think this might be the only place where this patch hits issues now...

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #8)
> Given the low number of consumers you changed in the MOZ_STATIC_ASSERT
> patch, don't you think it's worth just adding the second parameter now? Even
> if you don't propogate it up to JS_STATIC_ASSERT, it still seems like a win.

That seems fair, we can back-propagate into SpiderMonkey at however leisurely a pace we want.  I'll make that change once I have the fully-working part under control.

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

>   void staticAsserts() {
>     typedef int m[sInvMaxAlpha / sInvMaxAlpha];
>     typedef int m[sMaxInit / sMaxInit];
>   }

On the clang side this is http://llvm.org/bugs/show_bug.cgi?id=11630. I will try to find some time to work on it.

Comment 12

[Jeff Walden [:Waldo]]

Welp, looks like typedefs for C++ are out as an implementation technique for awhile, if both clang and gcc have problems with it.

The current trick that works is an extern function declaration, with __COUNTER__-izing if possible.  The __COUNTER__ bits cover up this error in gcc >=4.3, and 4.2 seems perfectly willing to allow extern "C" and non-extern "C" declarations to coexist, so maybe reintroducing the __COUNTER__ gunk is the right fix.  Unfortunately that doesn't fit well in the compiler casuistry in the latest patch; bother.  Some sort of patch will arrive tomorrow here.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: MOZ_STATIC_ASSERT, v3

Can we outrun gcc 4.2?  That is the question....  If worse comes to worst we can always just disable the macro there, although given the Sun guys' experience that's to be avoided at all costs.

To answer an implicit question, this was always a hazard of JS_STATIC_ASSERT's current definition.  It's just that it only mattered on gcc 4.2, and that plus the phase of the moon meant we never observed it.

C1X has _Static_assert which could be used here, but I don't know quite enough about the conditions where it can be used without triggering warnings or other such things to add a section attempting to use it.

Comment 14

[Jeff Walden [:Waldo]]

Oh, and it seems for there to be a conflict with gcc 4.2 the C++-linkage extern must precede the C-linkage extern.  That's even more narrowing of the conflict condition...

Comment 15

[Jeff Walden [:Waldo]]

Oh, this passed try.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 583581 [details] [diff] [review]
MOZ_STATIC_ASSERT, v3

It builds?  Ship it

Comment 17

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0df00b3b8846
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffea4a903ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7567cf0a67
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d4a9617fcd1

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0df00b3b8846
https://hg.mozilla.org/mozilla-central/rev/cffea4a903ac
https://hg.mozilla.org/mozilla-central/rev/0d7567cf0a67
https://hg.mozilla.org/mozilla-central/rev/8d4a9617fcd1

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> On the clang side this is http://llvm.org/bugs/show_bug.cgi?id=11630. I will
> try to find some time to work on it.

I fixed it in r147281. Thanks for reporting it!

Comment 20

[Jeff Walden [:Waldo]]

https://developer.mozilla.org/en/mfbt briefly mentions Assertions.h, and the API defined by the header is profusely commented in the header.

Also, a blog post and a newsgroup post:

http://whereswalden.com/2011/12/26/introducing-mozillaassertions-h-to-mfbt/
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/43b7181c25ee759d