We have functions like GetImageFromSourceSurface that look like ---------------- if (...) ... else if(...) ... // no else assert(0); ----------------- This works OK on a debug, but on an release build assert does nothing, so we get a warning from the compiler. This is particularly bad when building with clang, as it puts this warning in -Wreturn-type and we build with -Werror=return-type.

This is based on llvm's implementation of llvm_unreachable(). https://tbpl.mozilla.org/?tree=Try&rev=c3bbca690c99

https://tbpl.mozilla.org/?tree=Try&rev=b137b70a516d Added missing include.

Comment on attachment 594103 [details] [diff] [review] Need a macro that expands to __builtin_unreachable on release builds >--- a/mfbt/Assertions.h >+++ b/mfbt/Assertions.h >+#include "Attributes.h" mozilla/Attributes.h > extern MFBT_API(void) >-JS_Assert(const char* s, const char* file, int ln); >+JS_Assert(const char* s, const char* file, int ln) MOZ_NORETURN; I think there was some concern about this making debugging harder

> mozilla/Attributes.h Will do. > > extern MFBT_API(void) > >-JS_Assert(const char* s, const char* file, int ln); > >+JS_Assert(const char* s, const char* file, int ln) MOZ_NORETURN; > > I think there was some concern about this making debugging harder Do you remember what they were? I can create a MozUnreachablF function that is marked noreturn and just calls JS_Assert if you want.

Changed the include and the attribute position to make msvc happy. https://tbpl.mozilla.org/?tree=Try&rev=9703b2263a68

Unfortunately we still use gcc 4.4 on android, so this updated patch adds support for it. https://tbpl.mozilla.org/?tree=Try&rev=29d4d3396469

The concern is that if the function's marked noreturn, you can't return from it in a debugger (using "signal 0" in gdb, and such). Does using MOZ_NORETURN make that happen? Clang has an attribute that indicates unreachability without affecting codegen, which was what I was thinking of as the better alternative, but I hadn't written a patch or done much more than think about doing so.

Comment on attachment 594133 [details] [diff] [review] Need a macro that expands to __builtin_unreachable on release builds Review of attachment 594133 [details] [diff] [review]: ----------------------------------------------------------------- So we'd have both MOZ_NOT_REACHED and MOZ_BUILTIN_NOT_REACHED? That seems highly undesirable to me. We should just do the work of auditing the existing users and making MOZ_NOT_REACHED expand to __builtin_unreachable() when possible. ::: mfbt/Assertions.h @@ +43,5 @@ > #ifndef mozilla_Assertions_h_ > #define mozilla_Assertions_h_ > > #include "mozilla/Types.h" > +#include "mozilla/Attributes.h" Put this extra #include first to keep the list in alphabetical order. @@ +144,5 @@ > #ifdef __cplusplus > extern "C" { > #endif > > +MOZ_NORETURN extern MFBT_API(void) Looking at C11, it looks like C11 requires its _Noreturn keyword to appear after "extern". That placement's compatible with C++11 and with all the existing MOZ_NORETURN implementations, as far as I can tell. We don't try to have MOZ_NORETURN expand to _Noreturn when compiling C yet, but it seems like something we should at least keep in mind. So do this: extern MOZ_NORETURN MFBT_API(void) I should update docs for this, as well as all the current users, for consistency. @@ +149,1 @@ > JS_Assert(const char* s, const char* file, int ln); I'd like to know how this affects debugging, when the underlying exception is ignored and stepped past using "signal 0". Given that noreturn is supposed to provide an optimization opportunity, such that calling a noreturn function need not preserve callee registers, preserve control flow into subsequent code, &c., I'd be pretty surprised if you could step out of the function, back into calling code, without something going horribly awry very quickly. @@ +227,5 @@ > + * compiler to reach this point. Otherwise is not defined. > + */ > +#if defined(__clang__) || (__GNUC__ > 4) \ > + || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5) > +# define MOZ_BUILTIN_NOT_REACHED __builtin_unreachable() MSVC has a moral equivalent to this, __assume(0), so whatever form this appears in eventually should include it. Probably that means this stuff should be figured out in the massive #if near the top of the file, using a MOZ_HAVE_NOT_REACHED macro (similar to how MOZ_HAVE_NORETURN and MOZ_HAVE_NEVER_INLINE are used). @@ +255,5 @@ > +#elif defined(__GNUC__) > +/* > + * On older versions of gcc we need to call a noreturn function to mark the > + * code as unreachable. > + */ This should be indented to line up with the "define"

Comment on attachment 594133 [details] [diff] [review] Need a macro that expands to __builtin_unreachable on release builds Jeff's review on this is more than sufficient.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #7) > The concern is that if the function's marked noreturn, you can't return from > it in a debugger (using "signal 0" in gdb, and such). Does using > MOZ_NORETURN make that happen? Clang has an attribute that indicates > unreachability without affecting codegen, which was what I was thinking of > as the better alternative, but I hadn't written a patch or done much more > than think about doing so. clang's attribute is the same as gcc. The implementation I suggested is based on the one used by llvm :-) I will try to write up a patch that uses the builtin on both debug and opt builds.

> I'd like to know how this affects debugging, when the underlying exception > is ignored and stepped past using "signal 0". Given that noreturn is > supposed to provide an optimization opportunity, such that calling a > noreturn function need not preserve callee registers, preserve control flow > into subsequent code, &c., I'd be pretty surprised if you could step out of > the function, back into calling code, without something going horribly awry > very quickly. True, but that can also happen with the debugger reaching code marked with __bultin_unreachable. I will try to upload a patch that uses only bultin_unreachable so that existing asserts are not affected.

https://tbpl.mozilla.org/?tree=Try&rev=bf9dee35fd02 The main differences from the previous patch are * Add win32's __assume(0) * Don't mark JS_Assert as noreturn * Document that most code should use MOZ_NOT_REACHED A consequence of not marking JS_Assert is that we need a small helper function when using older gcc (android).

This version will hopefully work on apple's gcc too: https://tbpl.mozilla.org/?tree=Try&rev=0e6de94ecc43

Sorry about the noise, I don't have gcc 4.2 anymore and forgot to add __USER_LABEL_PREFIX__ on the last patch. https://tbpl.mozilla.org/?tree=Try&rev=5100bac8f828

Sorry once again for the noise, I forgot to stringify __USER_LABEL_PREFIX__ on the previous patch. This one is green: https://tbpl.mozilla.org/?tree=Try&rev=45ffb5958eee

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #11) > True, but that can also happen with the debugger reaching code marked with > __bultin_unreachable. Certainly. There's a practical difference between the two cases. We know we hit assertions all the time and may want to examine what happens after they hit, if they're hit. Not-reached points we're confident enough in to mark as such, on the other hand, are hit much less often than assertions (and not just because there are fewer of them). And we don't actually have any points marked with __builtin_unreachable now anyway. The attribute I meant for indicating unreachability without affecting codegen was analyzer_noreturn, which I don't believe applies to gcc. (Whether that attribute also silences clang warnings like noreturn does, I don't know. It really should, if it doesn't.)

> Certainly. There's a practical difference between the two cases. We know > we hit assertions all the time and may want to examine what happens after > they hit, if they're hit. Not-reached points we're confident enough in to > mark as such, on the other hand, are hit much less often than assertions > (and not just because there are fewer of them). And we don't actually have > any points marked with __builtin_unreachable now anyway. > > The attribute I meant for indicating unreachability without affecting > codegen was analyzer_noreturn, which I don't believe applies to gcc. > (Whether that attribute also silences clang warnings like noreturn does, I > don't know. It really should, if it doesn't.) I never had problems debugging programs that had reached an abort or the C assert and enjoy the fact that the compiler produces faster and smaller binaries based on the knowledge of what those functions do. In any case, the last patch only affects MOZ_NOT_REACHED, not JS_Assert. Is it OK?

Comment on attachment 597791 [details] [diff] [review] rebased and updated for the s/JS_Assert/MOZ_Assert/ change. Review of attachment 597791 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +210,5 @@ > #else > # define MOZ_ASSERT_IF(cond, expr) ((void)0) > #endif > > +/* MOZ_NOT_REACHED_MARKER - On compilers which support it, expands Let's make invoking this more function-like, so MOZ_NOT_REACHED_MARKER(). Stylistically, our documentation comments don't speak in "two-step" form -- that is, FOO -- It does foo. Rather, we write "FOO does...". In that vein: MOZ_NOT_REACHED_MARKER() expands (in compilers that support it) to an expression which states that it is... @@ +216,5 @@ > + * compiler to reach this point. Most code should probably use the higher > + * level MOZ_NOT_REACHED (which expands to this when appropriate). > + */ > +#if defined(__clang__) || (__GNUC__ > 4) \ > + || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5) Let's be consistent with the other #if-sequences here and not unify the clang/gcc conditions even if the bodies would be identical -- more readable that way: #if defined(__clang__) # define MOZ_NOT_REACHED_MARKER() __builtin_unreachable() #elif defined(__GNUC__) # if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5) # define MOZ_NOT_REACHED_MARKER() __builtin_unreachable() # endif #elif defined(_MSC_VER) # define MOZ_NOT_REACHED_MARKER() __assume(0) #endif @@ +219,5 @@ > +#if defined(__clang__) || (__GNUC__ > 4) \ > + || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5) > +# define MOZ_NOT_REACHED_MARKER __builtin_unreachable() > +#elif defined(WIN32) > +# define MOZ_NOT_REACHED_MARKER __assume(0) Note that mfbt style is two-space indentation in macro-nesting (see above). @@ +244,5 @@ > +# if defined(DEBUG) > +# define MOZ_NOT_REACHED(reason) do { \ > + MOZ_Assert(reason, __FILE__, __LINE__); \ > + MOZ_NOT_REACHED_MARKER; \ > + } while (0) # if defined(DEBUG) # define MOZ_NOT_REACHED(reason) \ do { \ MOZ_Assert(reason, __FILE__, __LINE__); \ MOZ_NOT_REACHED_MARKER(); \ } while (0) # else ... @@ +259,5 @@ > +# define MOZ_GETASMPREFIX(X) MOZ_GETASMPREFIX2(X) > +# define MOZ_ASMPREFIX MOZ_GETASMPREFIX(__USER_LABEL_PREFIX__) > + extern MOZ_NORETURN MFBT_API(void) > + MOZ_ASSERT_NR(const char* s, const char* file, int ln) \ > + asm (MOZ_ASMPREFIX "MOZ_Assert"); Please include a comment linking to something like http://gcc.gnu.org/onlinedocs/gcc-4.3.4/gcc/Asm-Labels.html#Asm-Labels to explain exactly what it is you're doing here. Also add a comment that MOZ_ASSERT_NR is not to be used except through this macro.

I did a last try push to https://tbpl.mozilla.org/?tree=Try&rev=8fa94238b9f8 If all compilers like the new version I will push to m-i. Thanks!