Various assertion improvements for MFBT

RESOLVED FIXED in mozilla12

Status

()

defect
--
minor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

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

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

8 years ago
No description provided.
Assignee

Comment 1

8 years ago
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)
Assignee

Comment 2

8 years ago
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)
Assignee

Comment 3

8 years ago
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)
Assignee

Comment 4

8 years ago
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)
Assignee

Comment 5

8 years ago
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

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

Updated

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

Updated

8 years ago
Attachment #582960 - Flags: review?(luke) → review+
Attachment #582958 - Flags: review?(jones.chris.g) → review+
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.
Assignee

Comment 9

8 years ago
Posted patch MOZ_STATIC_ASSERT, v2 (obsolete) — Splinter Review
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...
Assignee

Comment 10

8 years ago
(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.
Assignee

Comment 12

8 years ago
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.
Assignee

Comment 13

8 years ago
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)
Assignee

Comment 14

8 years ago
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...
Assignee

Updated

8 years ago
Attachment #583217 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #582960 - Attachment is obsolete: true
Assignee

Comment 15

8 years ago
Oh, this passed try.
Comment on attachment 583581 [details] [diff] [review]
MOZ_STATIC_ASSERT, v3

It builds?  Ship it
Attachment #583581 - Flags: review?(luke) → review+
Assignee

Updated

8 years ago
Blocks: 712936
Assignee

Updated

8 years ago
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!
Assignee

Comment 20

8 years ago
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

Updated

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