Last Comment Bug 712129 - Various assertion improvements for MFBT
: Various assertion improvements for MFBT
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- minor (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 712246 (view as bug list)
Depends on: 714580
Blocks: 712936 712939
  Show dependency treegraph
 
Reported: 2011-12-19 14:14 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-01 23:18 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move all the assertion stuff to mfbt/Assertions.h (6.25 KB, patch)
2011-12-19 14:16 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Move the MOZ*INLINE macros to Attributes.h, since that's what they are (5.42 KB, patch)
2011-12-19 14:18 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Move various assertion flavors into mfbt from the JS engine: MOZ_NOT_REACHED, MOZ_ASSERT_IF, MOZ_ALWAYS_TRUE, MOZ_ALWAYS_FALSE (4.46 KB, patch)
2011-12-19 14:21 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review
Implement MOZ_STATIC_ASSERT and MOZ_STATIC_ASSERT_IF (9.22 KB, patch)
2011-12-19 14:25 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
MOZ_STATIC_ASSERT, v2 (9.94 KB, patch)
2011-12-20 10:46 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
MOZ_STATIC_ASSERT, v3 (10.83 KB, patch)
2011-12-21 12:21 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-19 14:14:24 PST

    
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-19 14:16:09 PST
Created attachment 582954 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-19 14:18:21 PST
Created attachment 582957 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-19 14:21:26 PST
Created 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

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] (remove +bmo to email) 2011-12-19 14:25:41 PST
Created attachment 582960 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-19 14:44:36 PST
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 6 :Ms2ger 2011-12-20 02:56:17 PST
*** Bug 712246 has been marked as a duplicate of this bug. ***
Comment 7 :Ms2ger 2011-12-20 03:51:54 PST
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 Ted Mielczarek [:ted.mielczarek] 2011-12-20 05:11:53 PST
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] (remove +bmo to email) 2011-12-20 10:46:30 PST
Created attachment 583217 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-20 10:49:35 PST
(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) 2011-12-20 16:55:57 PST
>   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] (remove +bmo to email) 2011-12-20 22:12:37 PST
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] (remove +bmo to email) 2011-12-21 12:21:06 PST
Created attachment 583581 [details] [diff] [review]
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] (remove +bmo to email) 2011-12-21 12:24:19 PST
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] (remove +bmo to email) 2011-12-21 15:19:16 PST
Oh, this passed try.
Comment 16 Luke Wagner [:luke] 2011-12-21 18:05:47 PST
Comment on attachment 583581 [details] [diff] [review]
MOZ_STATIC_ASSERT, v3

It builds?  Ship it
Comment 19 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-26 14:51:45 PST
> 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] (remove +bmo to email) 2011-12-27 00:44:50 PST
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

Note You need to log in before you can comment on or make changes to this bug.