The default bug view has changed. See this FAQ.

Various assertion improvements for MFBT

RESOLVED FIXED in mozilla12

Status

()

Core
MFBT
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
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.  :-)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #582954 - Flags: review?(luke)
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.
Attachment #582957 - Flags: review?(luke)
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.
Attachment #582958 - Flags: review?(jones.chris.g)
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.
Attachment #582960 - Flags: review?(luke)
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.

Updated

5 years ago
Attachment #582954 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #582957 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #582960 - Flags: review?(luke) → review+
Attachment #582958 - Flags: review?(jones.chris.g) → review+

Updated

5 years ago
Duplicate of this bug: 712246
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?
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.
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...
(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.
>   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.
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.
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.
Attachment #583581 - Flags: review?(luke)
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...
Attachment #583217 - Attachment is obsolete: true
Attachment #582960 - Attachment is obsolete: true
Oh, this passed try.

Comment 16

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

It builds?  Ship it
Attachment #583581 - Flags: review?(luke) → review+
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
Target Milestone: --- → mozilla12
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 712936
Blocks: 712939
> 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!
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
Keywords: dev-doc-complete

Updated

5 years ago
Depends on: 714580
You need to log in before you can comment on or make changes to this bug.