Lots of reviewers have asked me to substitute a closing MOZ_CRASH() with a MOZ_NOT_REACHED(), as though these two are equivalent. They're not; MOZ_NOT_REACHED is not an assertion. It's a flag which enables compiler optimizations. Given that there's widespread confusion about this, I propose modifying the comments on MOZ_NOT_REACHED and sending a public service announcement to dev.planning. I'll post a patch in a moment.

I know the example is contrived, and you don't like that. Suggestions for improvement are welcome; I'm kind of at a loss for other occasions when when it's appropriate to use this macro, to be honest.

Should we do away with MOZ_NOT_REACHED? Bug 824840 shows another example, where bz (of all people) was confused about it even after I pointed him at this bug. The fact that you can't come up with a good example of its use in a comment make me suspect that we don't need it at all. (On a side note: part of the confusion may be due to the fact that JS_NOT_REACHED used to be equivalent to JS_ASSERT(false). That's how everybody seems to be using it.) If you really believe we need this macro, can we rename it MOZ_OPTIMIZE_FOR_UNREACHABILITY or something equally descriptive and awkward, so that people think twice before using it?

Oh, and the existing not-quite-right uses should also be changed.

I suspect the confusion is increased by the existence of NS_NOTREACHED that is likely doing what people expect MOZ_NOT_REACHED to do (assert in debug, nothing in opt)

> Should we do away with MOZ_NOT_REACHED? I can fathom circumstances under which MOZ_NOT_REACHED would be appropriate. Particularly in SM, I imagine there are places where we really do want the compiler to assume a particular point cannot be reached. But perhaps, as mak suggests, it shouldn't be called "MOZ_NOT_REACHED". Maybe MOZ_ASSUME_NOT_REACHED() would be more clear. And I suspect that the majority of places where MOZ_NOT_REACHED is used it's not being used as an optimization.

I have partial comments I really need to get around to polishing up and posting. In the meantime, I'm not sure I agree with the premise of some of the comments here, although I'm certainly happy to adjust docs to make them more understandable or more precise or whatever regardless of any semantics issues. An assertion indicates that a condition is impossible to encounter. There should be no expectation about semantics when it is encountered and fails (however failure is defined -- by being reached, for MOZ_NOT_REACHED, for its condition failing with MOZ_ASSERT [or that it would have failed, if it were checked or could be checked in an opt build], or whatever). By this I mean the *concept* of an assertion, of course. (We could say we have our own thing which we call "assertion", but that just muddies the water.) MOZ_NOT_REACHED() has no semantics at all, because it's impossible for it to be hit. If you say it's impossible for this to happen, you can't very well say "...but if it does, do this". You're off the rails already. (It's like saying fluffy unicorns do not exist, but the fluffy unicorns that do exist are piiiiiiiiink. It's self-contradictory.) That it happens to let compilers optimize knowing a point is unreachable is gravy, not promised semantics. MOZ_CRASH() is totally different -- it just crashes the program, with no implication that something bad happened, or that something bad didn't happen. It's up to the user of it to decide what it implies, if anything.

> An assertion indicates that a condition is impossible to encounter. There > should be no expectation about semantics when it is encountered and fails I sure hope it aborts the process in a debug build. A concrete question that's been bugging me: what should I use in the default case of a switch that shouldn't be hit? My current practice is to put either MOZ_ASSERT(false) or MOZ_CRASH(), depending on where it occurs and how hard-assed I'm feeling at the time. Is that reasonable?

AIUI you're saying that MOZ_NOT_REACHED is in fact an "assertion", where an "assertion" is something that's impossible to reach. Is that right? If so, I contend that definition is not consistent with the usage of "assertion" in Mozilla code and indeed in code in general. MOZ_ASSERT has well-defined semantics when it fails -- we don't say that it's impossible for a MOZ_ASSERT condition to be false. But I hope you'll agree that MOZ_ASSERT is an "assertion". Similarly for NS_ASSERT, and NS_ABORT_IF_FALSE. C's assert() function is also well-defined. I hope you'll agree that's reasonably called an "assertion". By far the most common usage of "assertion" -- in both Mozilla code and in programming in general -- is a function which checks a condition and declares that there's an error if that condition fails. If there is an error, it then takes some defined action. I'd be happy to extend the notion of "assertion" to a function whose check may be trivial (always declare that there is an error). You're of course free to disagree with this usage and to define the word "assertion" however you like, but for the purposes of documentation, I don't think we should call a function which behaves differently than all other assertions an "assertion", because that would cause some people to use MOZ_NOT_REACHED incorrectly. Indeed it has. > That it happens to let compilers optimize knowing a point is unreachable is gravy, not > promised semantics. Understood. But we agree that the only reason to use MOZ_NOT_REACHED is to enable these optimizations, right? If you don't care about the speed of your code, you should never use MOZ_NOT_REACHED, as it is strictly less safe than MOZ_CRASH() or MOZ_ASSERT(false), right? It's a simple result that it's very unlikely that you're able to prove anything non-trivial about the Mozilla codebase, so in particular it's unlikely that you can prove that a particular line of code is not reached. As a programmer, you can "assert" without proof that a line is not reached (*), but that's all. And even if you can prove that a particular line is not reached, you certainly can't prove that there's no modification one can make to Gecko which will cause it to be reached. So since any line of code /may/ be reached -- either in this build or a future one -- MOZ_NOT_REACHED(), which triggers undefined behavior when it's reached, is less safe than the alternatives, which trigger defined behavior. The chance of undefined behavior is necessary sometimes. But you have to get something in return for the decrease in safety for it to be worthwhile. Certainly if you got an increase in speed, that would be something. Perhaps there's some other advantage we gain from MOZ_NOT_REACHED. But the point is, MOZ_NOT_REACHED is not an "assertion" how most of us understand that word, and MOZ_NOT_REACHED decreases the safety of our code, so should not be used where you don't care about its corresponding benefits. That's what I was trying to get across in this patch. > what should I use in the default case of a switch that shouldn't be hit? Because code can change, I think you should use something with defined behavior, unless you care about speed. Someone could add a new value to the enum, or someone could pass in a bad int value to the function. (*) Perhaps this is what you mean by "assertion", but note that this is different from asking the program to check a condition.

Wow, sorry, that was a lot longer than I intended it to be.

> MOZ_NOT_REACHED decreases the safety of our code This point is critical, and I didn't even understand it until just now.

Can we remove (the subtle and dangerous) MOZ_NOT_REACHED and fold its compiler-specific "not reached" optimization hint into the end of MOZ_CRASH, one of the few places in our code that we can guarantee is unreachable?

Comment on attachment 691220 [details] [diff] [review] Patch, v1 Jeff, should I take the lack of action here as an r-?

Here is an alternate patch: 1. Rename MOZ_NOT_REACHED_MARKER() to MOZ_ASSUME_UNREACHABLE() and remove fatal abort() calls. 2. Call MOZ_ASSUME_UNREACHABLE() at end of MOZ_CRASH() so MOZ_CRASH() can be used to provide optimization hints about unreachable code. 3. Temporarily call MOZ_CRASH() from MOZ_NOT_REACHED(), assuming MOZ_NOT_REACHED() calls could be replaced with MOZ_CRASH() or MOZ_ASSUME_UNREACHABLE(). Curiously, the try builds I ran are green except for "Win debug" failures. Perhaps "Win debug" tests are hitting a MOZ_NOT_REACHED(), which now crashes instead of asserting? https://tbpl.mozilla.org/?tree=Try&rev=e505408211db https://tbpl.mozilla.org/?tree=Try&rev=e0594e273609

Jeff, I don't think it's fair to pocket veto these patches. If you disagree with both approaches here, can you please say so?

Not a pocket veto, just a harder question/lower priority than other reviews I have to deal with. (Including a couple going back to October. :-( ) I'll try to look at this later today.

Can we perhaps add a peer to mfbt to take you off the critical path?

There are several peers already, if unlisted ones (just, um, like the owner is, in letter if not spirit. or something). Fourteen-month-old mail has this as the set: "cjones looks like the winner by hg annotate. Chris, are you willing to own? Luke, will you peer along with Jeff, mhommey, Ms2ger" which admittedly is probably 20% out of date. I'd just update the module ownership page with this, but for the initial change it feels a little weird doing it -- especially as, when I added the section to the page originally, it spawned the thread that led to that mail.

fwiw that list might be more than 20% out of date; here's the tail of $ git log --pretty='%an' mfbt | sort | uniq -c | sort -g 4 Aryeh Gregor 4 Boris Zbarsky 4 Jacek Caban 4 Jeff Muizelaar 4 Ms2ger 5 Chris Jones 5 Chris Leary 5 Christian Holler 7 Ed Morley 7 Terrence Cole 8 Nathan Froyd 10 Ehsan Akhgari 11 Chris Peterson 11 Mike Hommey 11 Rafael Ávila de Espíndola 12 Benoit Jacob 19 Justin Lebar 79 Jeff Walden

Comment on attachment 691220 [details] [diff] [review] Patch, v1 I'd be happy with glandium's review here, if that works for you, Jeff.

Comment on attachment 703592 [details] [diff] [review] MOZ_ASSUME_UNREACHABLE.patch Review of attachment 703592 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +132,5 @@ > + (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)) > + /* > + * __builtin_unreachable() was implemented in gcc 4.5. If we don't have > + * that, call a noreturn function; abort() will do nicely. Qualify the call > + * in C++ in case there's another abort() visible in local scope. You removed what is described here.

So, we have here two different approaches, none of which are entirely satisfactory imho. On one end, Justin's patch attempts to describe what MOZ_NOT_REACHED does, which is nice, but the way it currently works is not what is described: release builds will happily abort() with the current code depending on the compiler used. On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED crashes/aborts in all cases. I think I prefer Justin's approach, provided the code is modified to actually do what is advertised.

> On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED > crashes/aborts in all cases. Well, he essentially merges MOZ_CRASH and MOZ_NOT_REACHED. This actually seems perhaps better to me, tbh. It seems like this change removes an entire class of mistake / argument. But if we go with the comment-only change, perhaps we can also change the name of MOZ_NOT_REACHED to MOZ_ASSUME_UNREACHABLE? > provided the code is modified to actually do what is advertised. Do you want the comment to change from "will have undefined behavior" to "may have undefined behavior"? Or do you want to remove the existing abort()'s and replace them with a call to an empty noreturn function?

(In reply to Justin Lebar [:jlebar] from comment #22) > > On the other hand, Chris's patch changes the semantics entirely so that MOZ_NOT_REACHED > > crashes/aborts in all cases. > > Well, he essentially merges MOZ_CRASH and MOZ_NOT_REACHED. > > This actually seems perhaps better to me, tbh. It seems like this change > removes an entire class of mistake / argument. It will likely add an entire new class of problems, with the compiler trying to be smart and making crash reports useless because it throws away the return pointer value. (and that's not theoretical, that's why we removed noreturn from mozalloc_abort on arm) > But if we go with the comment-only change, perhaps we can also change the > name of MOZ_NOT_REACHED to MOZ_ASSUME_UNREACHABLE? > > > provided the code is modified to actually do what is advertised. > > Do you want the comment to change from "will have undefined behavior" to > "may have undefined behavior"? Or do you want to remove the existing > abort()'s and replace them with a call to an empty noreturn function? I don't know. The more I think about it, the more I think we may just want to remove that macro altogether, and add an optional reason to MOZ_CRASH, that would be displayed on debug builds. And leave __builtin_unreachable out of the equation, but I do understand some places may want it.

> It will likely add an entire new class of problems, with the compiler trying to be smart > and making crash reports useless because it throws away the return pointer value. Ah, excellent point. > The more I think about it, the more I think we may just want to remove that macro > altogether, and add an optional reason to MOZ_CRASH, that would be displayed on debug > builds. And leave __builtin_unreachable out of the equation, but I do understand some > places may want it. In the process of removing the macro, perhaps we'll get a sense for whether any codesites need the speed increase. I'd be in favor of converting most callsites to MOZ_CRASH(reason) and creating MOZ_UNSAFE_ASSUME_UNREACHABLE() if necessary.

MOZ_CRASH() has wanted a (non-optional) reason string for awhile -- see bug 763070. The issue has always been that we want MOZ_CRASH() to be guaranteed to crash, non-exploitably -- it should be usable even with an arbitrarily corrupted heap. That's easily enough done on Linux, but I'm not sure how to do it elsewhere. We could expediently ignore the string on "hard" platforms, if we thought printing a reason *sometimes* to be important enough to not delay for printing it *all* the time. Now that I state it that way, actually I think probably we should just do that, and let other platforms play catchup. :-) If it'll make people happy, I guess MOZ_ASSUME_UNREACHABLE() is okay with me. But I still think we should be confident in what we assert, and we should let the compiler know, in optimized builds. Worries about theoretical safety when we're wrong, given we're already in an incredibly unsafe language, continue to seem overblown to me.

Comment on attachment 691220 [details] [diff] [review] Patch, v1 Clearing wrt comment 25.

> The issue has always been that we want MOZ_CRASH() to be guaranteed to crash, > non-exploitably It's not clear to me how we can do this safely even on Linux. We could have MOZ_CRASH("foo") expand to write(1, "foo\n", 4); but there's no guarantee that fd 1 is actually stdout. Did you have a more clever way of doing this?

Jeff, ping re comment 27? I think we'd all love to resolve this bug.

Mostly the concern, I think, is about the process itself being exploited, through, say, fprintf's use of the malloc heap or similar (if fprintf were naively used). fd1 not being stdout wouldn't present an issue in this regard. I'm willing to accept that a re-bound fd1 will cause the MOZ_CRASH string to go nowhere visible. If we're truly paranoid we could try writing to /dev/tty, but given we're writing a constant text string we chose, it seems unlikely we'd compromise some other process or poison a file stream in an exploitable manner.

> Mostly the concern, I think, is about the process itself being exploited, through, say, > fprintf's use of the malloc heap or similar (if fprintf were naively used). fd1 not > being stdout wouldn't present an issue in this regard. What if fd1 were rebound to /proc/self/mem? /dev/tty seems pretty safe to me, naively.

Didn't know /proc/self/mem was a thing! (Although, really, I probably should have expected it.) And the exploit there is the write goes to an unmapped page, that triggers an exploit-induced page fault handler or similar, and then goes to town somehow. That does seem somewhat worrisome to us paranoiacs, although if you can install a custom #PF handler, it seems likely you'd have your vector already. Maybe /dev/tty is the reasonable thing to do, then, out of an abundance of caution if for no other reason.

> And the exploit there is the write goes to an unmapped page, that triggers an exploit- > induced page fault handler or similar, and then goes to town somehow. Or perhaps to modify the code we're running so the next instruction doesn't crash us!

Okay, we should take this discussion into bug 763070.

Most of the patches that I'm going to label "Part 1x" are uninteresting. They're not totally mechanical, however, so I'd like some eyes on them. In particular, if I've changed a MOZ_NOT_REACHED to a MOZ_CRASH in performance-critical code, please let me know. Most everything looked non-critical. I kicked off try runs with talos, but our infrastructure for meaningfully comparing talos results on try is so broken, I think it will probably be easier just to push to m-i and back it out if we see regressions.

I'm not 100% confident in these changes, so if you have any doubts, it may be worthwhile to try and poke glandium.

There were some parts here that I wasn't sure if the code was perf-sensitive or not. Probably doesn't make a difference.

I deal with JS_NOT_REACHED in a later patch.

Note that I'm relying on MOZ_CRASH() being noreturn, in all of these patches. This allows us to do int foo() { MOZ_CRASH(); } i.e., you don't have to put a return statement below a MOZ_CRASH().

Sorry, this is a big one.

This is the main event, where we try to clear up confusion about MOZ_NOT_REACHED.

These could probably be MOZ_CRASH()'s, if you like.

Comment on attachment 760695 [details] [diff] [review] Part 1j: s/MOZ_NOT_REACHED/MOZ_CRASH/ in js/. Note that I didn't get rid of all of the MOZ_NOT_REACHED()'s in this patch. I just got rid of some that seemed (to me) obviously not hot.

This was sed; nothing interesting here except the removal of the macro from Utils.h. This introduced a few whitespace errors (particularly in macros), but I think I covered them in the next patch. I don't know if you actually want to get rid of JS_NOT_REACHED, but given that both JS_NOT_REACHED and MOZ_NOT_REACHED were used in the JS engine, I figure now is a good time to get rid of the JS one in favor of the MOZ one.

I left in a bunch of MOZ_ASSUME_NOT_REACHED()'s, because I don't want to mess with the finely-tuned machine that is SM. But SM has a lot of code which looks like switch (foo) { ... default: MOZ_NOT_REACHED(); return false; } I get rid of these return statements (and other code) that sit after MOZ_NOT_REACHED. These give us a false sense of security and imply that MOZ_NOT_REACHED does something that it doesn't do.

In case there's any confusion, the approach here is basically: * Switch most users of MOZ_NOT_REACHED to MOZ_CRASH (I switched everything in Gecko, but left most of JS as-is). [per the end of comment 23] * Rename MOZ_NOT_REACHED to MOZ_ASSUME_NOT_REACHED. I'd be happy to change this to MOZ_ASSUME_UNREACHABLE if that's preferred. [per the last paragraph of comment 25] * Change the comment of MOZ_ASSUME_NOT_REACHED to be scarier. [per my patch from comment 1] * Remove dead code beneath MOZ_CRASH() and MOZ_ASSUME_NOT_REACHED.

Comment on attachment 760706 [details] [diff] [review] Part 1r: s/MOZ_NOT_REACHED/MOZ_CRASH/ in accessible/. I can't imagine any of these coming up in the wild somehow so I think I'd prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it probably doesn't really matter

(In reply to Trevor Saunders (:tbsaunde) from comment #62) > I can't imagine any of these coming up in the wild somehow so I think I'd > prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it > probably doesn't really matter Is this perf-sensitive code?

(In reply to Justin Lebar [:jlebar] from comment #63) > (In reply to Trevor Saunders (:tbsaunde) from comment #62) > > I can't imagine any of these coming up in the wild somehow so I think I'd > > prefer MOZ_ASSUME_UNREACHABLE() or whatever you're calling it, but it > > probably doesn't really matter > > Is this perf-sensitive code? I doubt it, but perhaps what I should have said is I'm comfortable enough that assert will never fire it being a exploitable bug if it does is ok with me.

Comment on attachment 760686 [details] [diff] [review] Part 1c: s/MOZ_NOT_REACHED/MOZ_CRASH/ in toolkit/. Review of attachment 760686 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/identity/IdentityCryptoService.cpp @@ +358,5 @@ > } > if (!*publicKey) { > SECKEY_DestroyPrivateKey(*privateKey); > *privateKey = NULL; > + MOZ_CRASH("PK11_GnerateKeyPair returned private key without public key"); There's no reason to keep what's above the MOZ_CRASH, no?

Comment on attachment 760703 [details] [diff] [review] Part 1p: s/MOZ_NOT_REACHED/MOZ_CRASH/ in dom/ and content/. Review of attachment 760703 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/PrimitiveConversions.h @@ +92,5 @@ > > private: > static inline bool converter(JSContext* cx, JS::Handle<JS::Value> v, > jstype* retval) { > + MOZ_CRASH("This should never be instantiated!"); Can't this be a static assertion?

> Can't this be a static assertion? How do you mean? I think a STATIC_ASSERT(false) will fail even if that inline function is never called -- it's not like instantiating a template.

Oh, hmm, I guess that function isn't templated even if the struct is. That's unfortunate.

Before I forget, yeah, UNREACHABLE is better than NOT_REACHED. Still looking at the rest of this, trying to get an idea about all the patches before proceeding to mark any particular one of them (except the small one, at least).

Comment on attachment 760711 [details] [diff] [review] Part 2d: Remove code after MOZ_ASSUME_NOT_REACHED() in js/. Review of attachment 760711 [details] [diff] [review]: ----------------------------------------------------------------- Removing the subsequent code's a good idea. Hopefully having apparent paths out the end of the function, or into other code, won't trigger warnings. Guess we'll find out if they do! ::: js/src/frontend/ParseNode.cpp @@ +403,1 @@ > } else { This was (is) effectively else-after-return. So we should move the else-block statements up a level and kill off the else. ::: js/src/jsreflect.cpp @@ +117,2 @@ > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE); \ > + MOZ_ASSUME_NOT_REACHED(expr); \ Make this MOZ_ASSERT + report + return false instead. Neither the old code nor what's in your patch really fixes the problem, but this will do as a quick hackaround.

Comment on attachment 760710 [details] [diff] [review] Part 2c: s/MOZ_NOT_REACHED|JS_NOT_REACHED/MOZ_ASSUME_NOT_REACHED/ in js/. Review of attachment 760710 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, we should only have the one.

Comment on attachment 760695 [details] [diff] [review] Part 1j: s/MOZ_NOT_REACHED/MOZ_CRASH/ in js/. Review of attachment 760695 [details] [diff] [review]: ----------------------------------------------------------------- As a practical matter, the only reason for JS_NOT_REACHED is that we hadn't taken the time to convert all of them to MOZ_NOT_REACHED yet. There was no semantic difference of intent between JS_NOT_REACHED uses and MOZ_NOT_REACHED uses. So these uses shouldn't be substituted. The couple following-statement removals here are worth taking, tho -- r=me on those changes.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #65) > Comment on attachment 760686 [details] [diff] [review] > Part 1c: s/MOZ_NOT_REACHED/MOZ_CRASH/ in toolkit/. > > Review of attachment 760686 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/identity/IdentityCryptoService.cpp > @@ +358,5 @@ > > } > > if (!*publicKey) { > > SECKEY_DestroyPrivateKey(*privateKey); > > *privateKey = NULL; > > + MOZ_CRASH("PK11_GnerateKeyPair returned private key without public key"); > > There's no reason to keep what's above the MOZ_CRASH, no? I'm not totally sure; it looks like we're trying to destroy the key to keep it from leaking in memory somehow. I think we should probably leave this.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #70) > ::: js/src/jsreflect.cpp > @@ +117,2 @@ > > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_PARSE_NODE); \ > > + MOZ_ASSUME_NOT_REACHED(expr); \ > > Make this MOZ_ASSERT + report + return false instead. Neither the old code > nor what's in your patch really fixes the problem, but this will do as a > quick hackaround. Would it be better to report + MOZ_ASSERT + return false instead? That way we'd still get the error report either way...

Comment on attachment 760708 [details] [diff] [review] Part 2a: Change MOZ_NOT_REACHED to MOZ_ASSUME_NOT_REACHED in mfbt/Assertions.h. Review of attachment 760708 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +401,5 @@ > + * This may allow the compiler to generate faster or smaller code, in some > + * circumstances. But outside performance- or size-critical code, you > + * probably shouldn't use this macro, because it's unsafe. Instead, consider > + * using MOZ_CRASH() to safely end execution when a particular line of code is > + * reached, or use MOZ_ASSERT() if you can recover from the failure. I think this comment has a little bit too much FUD about the consequences of a mistake. It's not like the consequences will always be arbitrary code execution. It might be fallthrough to invalid instructions, nonsense instructions that quickly crash, bad computations informed by incorrectly-assumed preconditions, etc. But the consequences aren't an immediate exploit, and I don't . This dials back the fear a little bit, and it's a little more informative about what the compiler can do with the info. How does this read? """ MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that the macro call can't be reached during execution and that any preconditions to it don't hold. This lets the compiler generate better-optimized code. If the macro call *is* reached, what happen depends on the compiler, its optimizer, surrounding code, etc. *Probably* you'll crash...but you might also fall through to code letting an attacker construct an exploit. Be careful using this macro when you're not completely confident the macro call won't be hit. For absolute safety, use MOZ_ASSERT or MOZ_CRASH instead. """

(In reply to Justin Lebar [:jlebar] from comment #74) > Would it be better to report + MOZ_ASSERT + return false instead? That way > we'd still get the error report either way... That would be exactly equivalent to the LOCAL_ASSERT (?) just above it. Gets the job done, I guess, but that'd be a bigger change, and it seems nice to minimize this if it's mostly mechanical. I'm still not particularly convinced this is better with the reason-strings for these being optional. Explanations, even when moderately obvious in the surrounding code, are *not* simply clutter. They incent people to better document these cases, and while it's certainly possible to specify nonsense strings, mostly that's not what people have done in the code I've looked at. (At least in the JS engine. Maybe people elsewhere are more of the hack-til-it-compiles mentality.) But I don't really feel like continuing to argue this, so I guess you win. :-|

> I think this comment has a little bit too much FUD about the consequences of a mistake. I don't think it's FUD; that's why I filed this bug in the first place. :) This is really the crux of our disagreement, I think. The comment as written reflects how this macro is used in Gecko after the patches here. The main reason for going through this exercise of changing everything in Gecko is to set a good example, so certainly I want the comment to match the code. I tried to make SM's usage of the macro more similar to Gecko's, but that was vetoed in comment 72, and I'm not going to fight that. So at this point it sounds like Gecko and SM are going to use the macro differently. That's not ideal, but if that's the best we can do, we shouldn't pretend otherwise in the comment. How about: > MOZ_ASSUME_UNREACHABLE([reason]) tells the compiler that it can assume that > the macro call cannot be reached during execution. This lets the compiler > generate better-optimized code under some circumstances, at the expense of > the program's behavior being undefined if control reaches the > MOZ_ASSUME_UNREACHABLE. > > In Gecko, you probably should not use this macro outside of performance- or > size-critical code, because it's unsafe. If you don't care about code size > or performance, you should probably use MOZ_ASSERT or MOZ_CRASH. > > SpiderMonkey is a different beast, and there it's acceptable to use > MOZ_ASSUME_UNREACHABLE more widely. We can say whatever you want after "SpiderMonkey".

That'll do.

> That would be exactly equivalent to the LOCAL_ASSERT (?) just above it. Gets the job done, I guess, > but that'd be a bigger change, and it seems nice to minimize this if it's mostly mechanical. Okay; I have ASSERT + REPORT + return false in my patch queue. Rebasing this is going to be fun. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/55c1f447549d https://hg.mozilla.org/integration/mozilla-inbound/rev/1735d098ea86 https://hg.mozilla.org/integration/mozilla-inbound/rev/5ecd26bc5274

s/MOZ_ASSUME_NOT_REACHED/MOZ_ASSUME_UNREACHABLE/. Sorry, I forgot we were doing this (but still put it in the mfbt comment and in my newsgroup post, for some reason). https://hg.mozilla.org/integration/mozilla-inbound/rev/01ad949468fb

https://hg.mozilla.org/mozilla-central/rev/55c1f447549d https://hg.mozilla.org/mozilla-central/rev/1735d098ea86 https://hg.mozilla.org/mozilla-central/rev/5ecd26bc5274 https://hg.mozilla.org/mozilla-central/rev/01ad949468fb