Closed
Bug 712129
Opened 12 years ago
Closed 12 years ago
Various assertion improvements for MFBT
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 2 obsolete files)
6.25 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.42 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
4.46 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
10.83 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 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 | ||
Comment 2•12 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•12 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•12 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•12 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•12 years ago
|
Attachment #582954 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #582957 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #582960 -
Flags: review?(luke) → review+
Updated•12 years ago
|
Attachment #582958 -
Flags: review?(jones.chris.g) → review+
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
Attachment #583217 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #582960 -
Attachment is obsolete: true
Assignee | ||
Comment 15•12 years ago
|
||
Oh, this passed try.
Comment 16•12 years ago
|
||
Comment on attachment 583581 [details] [diff] [review] MOZ_STATIC_ASSERT, v3 It builds? Ship it
Attachment #583581 -
Flags: review?(luke) → review+
Assignee | ||
Comment 17•12 years ago
|
||
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
Comment 18•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
> 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•12 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
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•