See long thread on moz.dev.platform. Bug 723114 changed the semantics of MOZ_NOT_REACHED from "print an error message and crash, in all builds" to "assert and crash in debug builds, undefined behavior in release builds". This is not what those pre-existing call sites want. We can either change it back; or just remove it and replace all uses with MOZ_CRASH.

The optimization-adding feature of MOZ_ASSUME_UNREACHABLE() doesn't seem that valuable (if we really cared about perf, we'd could probably just rearrange the code). Rather, MOZ_ASSUME_UNREACHABLE is valuable (at least in code I see) for squelching annoying GCC warnings when performing case analysis with a switch. A classic example is something like: enum E { A, B }; int foo(E e) { switch (e) { case A: return 1; case B: return 2; } } // warning: control reaches end of non-void function Before MOZ_ASSUME_UNREACHABLE, we'd just add this to the end of the function: MOZ_ASSERT(false && "Bad enum value"); return -1; or something to shut up the warning. MOZ_ASSUME_UNREACHABLE() is a nice readability improvement since one avoids the need to come up with a bogus return value. See FrameIter::* in js/src/vm/Stack.cpp for 30 examples of this but I think there are a bunch more of the same flavor all over SM. Note: while I don't think the optimization of MOZ_ASSUME_UNREACHABLE() is a significant win, converting all MOZ_ASSUME_UNREACHABLE() to function calls in release builds (MOZ_CRASH) might actually impact perf or code size and at least should be measured.

If people will otherwise just decide they're never going to use this, okay, let's remove. But I honestly believe worries about the potential undefined behavior that manifest here are an excess of paranoia. If people are at all worried about a case potentially happening, they should be crashing or similar. It is entirely possible to use this primitive correctly, and plenty of uses are entirely correct, and you're imposing the costs of the misbehavers on the well-behavers. That's backwards. Fix buggy uses. Don't throw out the baby with the bathwater.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > If people will otherwise just decide they're never going to use this, okay, > let's remove. > > But I honestly believe worries about the potential undefined behavior that > manifest here are an excess of paranoia. If people are at all worried about > a case potentially happening, they should be crashing or similar. It is > entirely possible to use this primitive correctly, and plenty of uses are > entirely correct, and you're imposing the costs of the misbehavers on the > well-behavers. That's backwards. Fix buggy uses. Don't throw out the baby > with the bathwater. I think the issue is that it's not obvious that this macro can be used in this bad way if you don't understand what the compiler does when it sees it.

If you want to keep it, please at least change the name so people stop confusing it with an assertion macro. (That's how the thread started.)

Agreeing with jorendorff on irc, I think most important thing to fix here is that a bunch of places that used JS_NOT_REACHED() and assumed this crashed even in release builds were converted to using debug-only assertions. jcrammer was just pointed out on irc that MOZ_CRASH has a noreturn annotation that should have the same warning-squelching utility as described in comment 1. So that makes a switch to MOZ_CRASH seem like a fine replacement for MOZ_ASSUME_UNREACHABLE (as long as perf doesn't regress).

Benoit, feedback? After your __built_unreachable experiments, you may be the resident expert. :) 1. Add MOZ_ASSERT_UNREACHABLE convenience macro. 2. Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH or MOZ_OPTIMIZE_UNREACHABLE_UNDEFINED_BEHAVIOR?

Conversation here slowed down probably because, as comment 1 suggested, there is no automatic fix. Automatically changing MOZ_ASSUME_UNREACHABLE into debug-only assertions could be bad in non-debug builds if some supposedly unreachable spots are reached and execution just carries through. (Of course the current undefined behavior might give the same outcome, or even something worse, we don't know, and it depends on the compiler). Automatically changing MOZ_ASSUME_UNREACHABLE into MOZ_CRASH could be bad in two ways. First, it could harm performance; second, it could be a case of adding release-mode assertions when that's not needed. Generally, we try to stay conservative with release-mode assertions, typically only using them when something really bad would otherwise happen, because it is infuriating for people to lose real-world work to an assertion. So, for lack of an automatic way out of the present situation, we will probably have to manually audit all the current MOZ_ASSUME_UNREACHABLE usage and decide on a case-by-case basis. What we can already do today is: - add a MOZ_ASSERT_UNREACHABLE macro, as Chris' patch here does, and as we agreed on the dev-platform thread. - rename MOZ_ASSUME_UNREACHABLE, even if we might end up removing it in the near future. If anything, an explicit name will help remove confusion and obtain needed r+'s. I still think that MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE is a good name: it says exactly what it does. Names based on _OPTIMIZE_ are more descriptive in terms of what this should have been used for, but that does not match what it had been used for, and doesn't describe what it actually does.

Comment on attachment 8405194 [details] [diff] [review] MOZ_ASSERT_UNREACHABLE.patch See previous comment. +1 to MOZ_ASSERT_UNREACHABLE, +1 to renaming MOZ_ASSUME_UNREACHABLE, -1 to MOZ_OPTIMIZE_* names.

patch v2: * Rename MOZ_ASSUME_UNREACHABLE to MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. * But add a temporary #define MOZ_ASSUME_UNREACHABLE that preserves the current behavior so this patch won't be blocked waiting for my follow-up rename patches throughout the tree to get r+'d by multiple reviewers. * Add a MOZ_NIGHTLY_ASSERT that is defined for both debug and release builds on the Nightly channel, but only debug builds on Aurora, Beta, and Release. XPCOM uses similar macros for thread-safety assertions on Nightly. Defining a standard macro like this could encourage other developers to add Nightly assertions. * #define MOZ_ASSERT_UNREACHABLE using MOZ_NIGHTLY_ASSERT so we can get extra testing so we can feel confident that no existing MOZ_ASSUME_UNREACHABLE code paths are getting executed because that might cause exploitable undefined behavior. There should be no performance overhead because this code is supposedly never executed. :)

btw, I did not rename MOZ_ASSUME_UNREACHABLE_MARKER because it is just an implementation detail of MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.

(In reply to Chris Peterson (:cpeterson) from comment #9) > Created attachment 8409855 [details] [diff] [review] > part-1-v2-add-MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.patch > > patch v2: > > * Rename MOZ_ASSUME_UNREACHABLE to MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. > > * But add a temporary #define MOZ_ASSUME_UNREACHABLE that preserves the > current behavior so this patch won't be blocked waiting for my follow-up > rename patches throughout the tree to get r+'d by multiple reviewers. I don't think that that's needed. We should be able to get a single r+ to rename all occurences of MOZ_ASSUME_UNREACHABLE to MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. I've been thinking, maybe MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE would be a slight improvement, as it would be closer to the current name while having about the same meaning. If we don't mass-rename now, then, given that, as discussed above here and on dev-plaform there is likely no legitimate or even intentional use of MOZ_ASSUME_UNREACHABLE throughout gecko, there is not much of a point in introducing a new name for it, like MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. The point of MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE was to have a better name for it right now, without waiting for the longer process of fixing each case.

(In reply to Benoit Jacob [:bjacob] from comment #11) > If we don't mass-rename now, then, given that, as discussed above here and > on dev-plaform there is likely no legitimate or even intentional use of > MOZ_ASSUME_UNREACHABLE throughout gecko, there is not much of a point in > introducing a new name for it, like > MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. The point of > MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE was to have a better name for it > right now, without waiting for the longer process of fixing each case. FWIW, I have follow up patches to rename MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE (where a safe error handling is possible), MOZ_CRASH (where no obvious error handling is possible), or MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE (where the function seems like it could be optimizable) throughout the tree. But I wanted to get feedback on these macros first.

OK, in that case (i.e. as you say you 1) already have patches and 2) have identified cases that would legitimately use an unreachability marker) then that sounds OK.

Comment on attachment 8409855 [details] [diff] [review] part-1-v2-add-MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.patch Review of attachment 8409855 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +434,5 @@ > + * executed. (Right??) Take this extra testing precaution because hitting > + * MOZ_ASSUME_UNREACHABLE_MARKER could trigger exploitable undefined behavior. > + */ > +#define MOZ_ASSERT_UNREACHABLE(reason) \ > + MOZ_NIGHTLY_ASSERT(false, "MOZ_ASSERT_UNREACHABLE: " reason) Interesting compromise, I think I like it :-) Slightly scared of having Nightly further drift apart from other channels performance-wise, but this shouldn't be very big, and Nightly is already different from other channels performance-wise, in much bigger ways (Nightly has profiling enabled).

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in content/. MOZ_ASSUME_UNREACHABLE invokes possibly dangerous undefined behavior if actually hit. Note that my proposed (but not yet r+'d) implemention of MOZ_ASSERT_UNREACHABLE checks both debug and release builds on the Nightly channel (but only debug builds of Aurora, Beta, and Release) to help determine whether any MOZ_ASSUME_UNREACHABLE calls on older channels might be hit.

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in dom/. For code paths did not have an obvious way to safely handle the "unreachable" code path, I called MOZ_CRASH() instead (which is what MOZ_ASSUME_UNREACHABLE would do if hit, if we are lucky).

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in parser/htmlparser/. Note that my proposed (but not yet r+'d) implemention of MOZ_ASSERT_UNREACHABLE checks both debug and release builds on the Nightly channel (but only debug builds of Aurora, Beta, and Release) to help determine whether any MOZ_ASSUME_UNREACHABLE calls on older channels might be hit.

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in webrtc/signaling. MOZ_ASSUME_UNREACHABLE invokes possibly dangerous undefined behavior if actually hit.

HTML parser is more hsivonen than dbaron.

Replace MOZ_ASSUME_UNREACHABLE with MOZ_CRASH in widget/android. MOZ_ASSUME_UNREACHABLE is an optimization that invokes possibly dangerous undefined behavior if actually hit. It does not necessarily crash. btw, it seems like a lot of this generated boilerplate code that calls env->PushLocalFrame() and HandleUncaughtException() could be refactored into one helper function.

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in the IPDL code generator. MOZ_ASSUME_UNREACHABLE is an optimization that invokes possibly dangerous undefined behavior if actually hit. Note that my proposed (but not yet r+'d) implemention of MOZ_ASSERT_UNREACHABLE checks both debug and release builds on the Nightly channel (but only debug builds of Aurora, Beta, and Release) to help determine whether any MOZ_ASSUME_UNREACHABLE calls on older channels might be hit. Is it acceptable for "message protocol not supported" errors in generated OnCallReceived() and OnMessageReceived() to crash in a release build (of Nightly)? If not than, I can replace MOZ_ASSUME_UNREACHABLE with a debug-only MOZ_ASSERT or NS_ASSERTION.

Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in mfbt/. MOZ_ASSUME_UNREACHABLE is an optimization that invokes possibly dangerous undefined behavior if actually hit. Note that my proposed (but not yet r+'d) implemention of MOZ_ASSERT_UNREACHABLE checks both debug and release builds on the Nightly channel (but only debug builds of Aurora, Beta, and Release) to help determine whether any MOZ_ASSUME_UNREACHABLE calls on older channels might be hit. Is it acceptable for mfbt/decimal/Decimal.cpp's ASSERT_NOT_REACHED() to crash in release builds of Nightly? If not, then it really shouldn't be calling MOZ_ASSUME_UNREACHABLE either. I can replace it was a debug-only MOZ_ASSERT.

Comment on attachment 8413537 [details] [diff] [review] fix-parser.patch Replace MOZ_ASSUME_UNREACHABLE with MOZ_ASSERT_UNREACHABLE in parser/htmlparser/.

Comment on attachment 8413536 [details] [diff] [review] fix-dom.patch I don't know which assert AsmJSCache.cpp wants. I'd assume MOZ_RELEASE_ASSERT or MOZ_ASSERT. But ask whoever wrote that code. ScriptSettings.cpp should use something which crashes always.

Why is there a difference between nightly and other channels? Unless this is extremely performance-sensitive code, we should be a runtime assert on all channels.

Needinfo for Luke about comment 24, regarding AsmJSCache.cpp.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #25) > Why is there a difference between nightly and other channels? Unless this is > extremely performance-sensitive code, we should be a runtime assert on all > channels. bsmedberg: I proposed making MOZ_ASSERT_UNREACHABLE a release assert in Nightly only because I wanted to reduce possible objections from reviewers. This bug involves many patches and many different reviewers, so I wanted to minimize landing delays. :) For the optimization marker MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE, the macro is (supposed to be) used in performance-critical code paths. I wanted to ensure that the (post-Nightly) release builds have no unnecessary overhead (like calling other functions) that might pessimize the compiler's optimizations.

Landed fix-android.patch. It has no dependency on the proposed MOZ_ASSERT_UNREACHABLE or MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE macros. https://hg.mozilla.org/integration/mozilla-inbound/rev/c8744094c985

(In reply to Olli Pettay [:smaug] from comment #24) > I don't know which assert AsmJSCache.cpp wants. AsmJSCache.cpp wants to squelch the "control flow path does not return a value" warnings. These paths are all reasonably hot (part of asm.js compile-time) and never expected to fail in practice (any more than any other code) so they do not want a release-mode assert. Does MOZ_ASSERT() somehow squelch warnings in release builds?

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #30) > Does MOZ_ASSERT() somehow squelch warnings in release builds? No. That's a good point. MOZ_ASSERT_UNREACHABLE is a nop in release builds, but MOZ_CRASH and MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE do squelch warnings in release and debug builds. Because AsmJSCache.cpp is hot, I will use MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE.

Comment on attachment 8409855 [details] [diff] [review] part-1-v2-add-MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.patch Review of attachment 8409855 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +331,5 @@ > + */ > +#if defined(NIGHTLY_BUILD) > +# define MOZ_NIGHTLY_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__) > +#else > +# define MOZ_NIGHTLY_ASSERT(...) MOZ_ASSERT(__VA_ARGS__) You'll want to double-check this works correctly with MSVC, which has issues with variadic arguments. I don't remember the full details offhand, but this usage smells rotten to me. @@ +430,5 @@ > + > +/* > + * Assert in all debug builds plus the Nightly channel's release builds. There > + * should be no performance overhead because this code is supposedly never > + * executed. (Right??) Take this extra testing precaution because hitting "Right??" is wrong. The compiler has to generate code for this line, even if only for fallthrough, unless it can prove to itself it can't run. And that code has the potential to throw off switch jump tables, hurt instruction caching, burden the branch predictor, &c. Just get rid of the parenthetical and preceding sentence.

Comment on attachment 8413545 [details] [diff] [review] fix-mfbt.patch Review of attachment 8413545 [details] [diff] [review]: ----------------------------------------------------------------- I still think we're throwing out the baby with the bath water here, but whatever.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5153021485ba https://hg.mozilla.org/integration/mozilla-inbound/rev/335bd3cd0802 https://hg.mozilla.org/integration/mozilla-inbound/rev/db13bb9473cc https://hg.mozilla.org/integration/mozilla-inbound/rev/8c25478e68dc https://hg.mozilla.org/integration/mozilla-inbound/rev/e69f4a011613 https://hg.mozilla.org/integration/mozilla-inbound/rev/c648f9211172 * TODO: I still need to replace MOZ_ASSUME_UNREACHABLE in the js directory. I have a WIP.

https://hg.mozilla.org/mozilla-central/rev/5153021485ba https://hg.mozilla.org/mozilla-central/rev/335bd3cd0802 https://hg.mozilla.org/mozilla-central/rev/db13bb9473cc https://hg.mozilla.org/mozilla-central/rev/8c25478e68dc https://hg.mozilla.org/mozilla-central/rev/e69f4a011613 https://hg.mozilla.org/mozilla-central/rev/c648f9211172

I forgot to land fix-parser.patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0860c0ba33

Remove MOZ_ASSUME_UNREACHABLE and MOZ_NIGHTLY_ASSERT macros. MOZ_ASSUME_UNREACHABLE is currently an alias for MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. The last calls to MOZ_ASSUME_UNREACHABLE were removed in bug 1036781: https://mxr.mozilla.org/mozilla-central/search?string=MOZ_ASSUME_UNREACHABLE New code should use: * MOZ_ASSERT_UNREACHABLE for code that can safely recover in release builds * MOZ_CRASH for code that can't recover or indicates a major bug * MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE to invoke compiler-specific optimizations with undefined behavior This patch also removes the MOZ_NIGHTLY_ASSERT macro, intended for "extra testing", that was defined in all channels' debug builds plus Nightly's release build. This macro is used nowhere else and may mask compiler warnings (e.g. unused variables in release builds) that don't surface until Nightly is merged to Aurora.

Comment on attachment 8485368 [details] [diff] [review] remove-MOZ_ASSUME_UNREACHABLE.patch Review of attachment 8485368 [details] [diff] [review]: ----------------------------------------------------------------- I still think this is all a bunch of FUD, and people should either have the courage of their convictions or handle the cases they "think" are impossible. But whatever, it's not the end of the world to give up a little free performance for only marginally "safer" crashes that still can often do pretty bad things.

Did you miss the dev-platform conversation, in particular the part with the results from this experiments, https://github.com/bjacob/builtin-unreachable-study

I did miss it. But it doesn't change my view. How compilers behave now, or at any particular time, does not speak to how they will behave forever. Giving compilers *more* information when it's easy to do so should be a no-brainer to me. Worst that happens is the compiler ignores it.

"How compilers behave now" seems important to me. The above-mentioned study shows that at least GCC 4.6, which is the version used by B2G/JB, generates code that is highly unsafe when an unreachability marker is hit.

Sure. If you're not sure a condition will always hold, don't assert it. An "assertion" you think might hold in some infinitesimal number of unknown cases, is not something that should be asserted. But every belief that *does* pass that bar, should be asserted, and should be communicated as such to the compiler. Either you believe something is true, and you assert it and assume forever and everywhere that it's the case. Or you don't, and you handle every eventuality no matter how unlikely (but still possible) you think it might be.

It sounds like what you're aruing for is to have MOZ_ASSERT_UNREACHABLE additionally hint MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. Is that right? MOZ_ASSUME_UNREACHABLE/MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE by itself never asserts anything.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #43) > Sure. If you're not sure a condition will always hold, don't assert it. An > "assertion" you think might hold in some infinitesimal number of unknown > cases, is not something that should be asserted. I don't understand why we're talking about assertions here; I was only talking about unreachability markers. That's very different. Hitting an assertion failure means crash cleanly (or do nothing if assertions are disabled). Hitting an unreachability marker means that at least on GCC 4.6 we can be jumping at an arbitrary address or continuing execution past the end of a function (no 'ret' instruction). That's a lot more unsafe than just crashing.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #44) > It sounds like what you're aruing for is to have MOZ_ASSERT_UNREACHABLE > additionally hint MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. Is that right? > MOZ_ASSUME_UNREACHABLE/MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE by itself > never asserts anything. In debug builds today, MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE calls both MOZ_ASSERT_UNREACHABLE and __builtin_unreachable/whatever. However, there is a risk that the compiler might optimize away the MOZ_ASSERT_UNREACHABLE even in debug builds, seeing that the code path leads to __builtin_unreachable. For more predictable debug tests, we could change MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE to not call __builtin_unreachable in debug builds: #ifdef DEBUG # define MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE(reason) MOZ_CRASH(reason) #else # define MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE(reason) __builtin_unreachable() #endif

(In reply to Chris Peterson (:cpeterson) from comment #46) > In debug builds today, MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE calls both > MOZ_ASSERT_UNREACHABLE and __builtin_unreachable/whatever. Whoops, my bad. I skimmed the code but only saw the compiler-specific unreachable markers.

(In reply to Chris Peterson (:cpeterson) from comment #46) > (In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #44) > > It sounds like what you're aruing for is to have MOZ_ASSERT_UNREACHABLE > > additionally hint MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE. Is that right? > > MOZ_ASSUME_UNREACHABLE/MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE by itself > > never asserts anything. > > In debug builds today, MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE calls both > MOZ_ASSERT_UNREACHABLE and __builtin_unreachable/whatever. However, there is > a risk that the compiler might optimize away the MOZ_ASSERT_UNREACHABLE even > in debug builds, seeing that the code path leads to __builtin_unreachable. Really? The undefined behavior in MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE allows to compiler to generate any code at that point. However, the crashing code in MOZ_ASSERT_UNREACHABLE has side effects (crashing the program) before that point, that can't be undone by anything one could do at that point. So it would seem illegal for a compiler to interprete MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE as allowing to ignore a _previously hit_ assertion failure. Now, the above paragraph is assuming that MOZ_ASSERT failure (i.e. MOZ_REALLY_CRASH) is correctly implemented so as to force the compiler to consider the possibility that it may have side effects. Unfortunately, that is not the case. Currently MOZ_REALLY_CRASH is implemented as dereferencing the null pointer, which is itself undefined behavior. So the scenario that you describe above is actually a possibility, but only as part of the more general problem that our assertion failures are undefined behavior. Fortunately, no compiler is crazy enough to take this obviously-unintentional undefined behavior to the letter (whereas the obviously-intentional undefined behavior of an unreachability marker is taken literally by gcc 4.6 at least). Still, having undefined behavior in MOZ_REALLY_CRASH is unfortunate and it would be nice if we could have it be merely implementation-defined behavior. Filed Bug 1070674 about that.

(In reply to Benoit Jacob [:bjacob] from comment #45) > I don't understand why we're talking about assertions here; I was only > talking about unreachability markers. That's very different. I don't agree. An unreachability marker is an assertion that a given code path can't be reached. That MOZ_ASSERT builds in the condition, and only tests the condition in debug builds, doesn't change my analysis. I see no difference in how the reader or compiler should interpret any of these: MOZ_ASSERT(!impossibleCondition()); if (impossibleCondition()) MOZ_ASSERT(false); if (impossibleCondition()) MOZ_ASSUME_UNREACHABLE(); They're all assertions, in my book, no matter that they're spelled differently, or might or might not be tested outside debug builds, or might or might not affect compiler behavior. Obviously, others such as you try to make finer distinctions about what is/isn't an assertion. I simply read it, generally, as a claim about what at some point constitutes a valid program state.

The difference is very simple: Assertions 1) have well-defined semantics (if false then crash) and 2) even if one writes a bad assertion, the worst thing that can come out of it is a crash. By contrast, unreachability markers are undefined behavior that some compilers aggressively interprete as such for optimization purposes, and a bad unreachability marker can be much worse than a crash. It can let the program continue in weird and often exploitable ways such as jumping to a exploiter-controllable address, or continuing execution past the end of the current function into whatever comes next in our address space. See above discussion/study. When I review a patch that introduces an assertion, I can easily reason about its logic, and I can also think "oh well, if that is wrong then this particular mochitest will hit it". Neither is true about an unreachability marker.

In the last paragraph, I meant to expand: "oh well, if that is wrong then this particular mochitest will hit it, and will crash, so we'll know about it, and even if for whatever reason this wasn't covered by a test, the worst thing that could happen would be a plain crash". None of these things are true about an unreachability marker.

And we know hitting MOZ_ASSUME_UNREACHABLE happens. We have at least 20 bugs (5 sec-*) reported for MOZ_ASSUME_UNREACHABLE assertion failures from fuzzing debug builds. Who knows how many mystery crashes from tests or the field are related to MOZ_ASSUME_UNREACHABLE executing bad code in release builds? https://bugzilla.mozilla.org/buglist.cgi?short_desc=Assertion+failure+MOZ_ASSUME_UNREACHABLE&short_desc_type=allwordssubstr

I don't agree that assertions necessarily have well-defined semantics. When an assertion fails, you are off the rails. Any code assuming the condition, which may be widely distributed, may be in error. It's not reasonably possible to understand or audit all of it, to say that just because this assertion fails *here*, it's a "safe" failure. That we hit MOZ_ASSUME_UNREACHABLE just means we sometimes have bugs in our code. A shocker. And so we should fix them. This semantic disagreement about what constitutes an assertion isn't going to go anywhere. I'm done here.

This did land on mozilla-central, though the bug didn't get marked. Sorry for any confusion. https://hg.mozilla.org/mozilla-central/rev/73272a534297

Split off bug 1112029 for comment 46 (ensuring the asserts don't get optimized out in debug builds).