Last Comment Bug 723114 - Need a macro that expands to __builtin_unreachable on release builds
: Need a macro that expands to __builtin_unreachable on release builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 723534 723864
  Show dependency treegraph
 
Reported: 2012-02-01 07:50 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-02-23 18:39 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Need a macro that expands to __builtin_unreachable on release builds (1.32 KB, patch)
2012-02-03 02:25 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Need a macro that expands to __builtin_unreachable on release builds (3.26 KB, patch)
2012-02-03 02:38 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Need a macro that expands to __builtin_unreachable on release builds (3.17 KB, patch)
2012-02-03 03:14 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Need a macro that expands to __builtin_unreachable on release builds (3.37 KB, patch)
2012-02-03 04:53 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
New version (3.72 KB, patch)
2012-02-13 18:50 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
version that doesn't ICE apple's gcc (3.70 KB, patch)
2012-02-14 01:59 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
With __USER_LABEL_PREFIX__ (3.73 KB, patch)
2012-02-14 02:29 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
Correct the use of __USER_LABEL_PREFIX__ (3.88 KB, patch)
2012-02-14 06:17 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Review
rebased and updated for the s/JS_Assert/MOZ_Assert/ change. (3.88 KB, patch)
2012-02-16 06:03 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
jwalden+bmo: review+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-01 07:50:11 PST
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.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 02:25:57 PST
Created attachment 594100 [details] [diff] [review]
Need a macro that expands to __builtin_unreachable on release builds

This is based on llvm's implementation of llvm_unreachable().

https://tbpl.mozilla.org/?tree=Try&rev=c3bbca690c99
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 02:38:54 PST
Created attachment 594103 [details] [diff] [review]
Need a macro that expands to __builtin_unreachable on release builds

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

Added missing include.
Comment 3 :Ms2ger 2012-02-03 02:58:18 PST
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
Comment 4 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 03:01:28 PST
> 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.
Comment 5 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 03:14:59 PST
Created attachment 594113 [details] [diff] [review]
Need a macro that expands to __builtin_unreachable on release builds

Changed the include and the attribute position to make msvc happy.

https://tbpl.mozilla.org/?tree=Try&rev=9703b2263a68
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 04:53:18 PST
Created attachment 594133 [details] [diff] [review]
Need a macro that expands to __builtin_unreachable on release builds

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
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-03 12:03:25 PST
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 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-03 14:49:06 PST
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 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-03 18:29:22 PST
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.
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 17:46:04 PST
(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.
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 17:52:04 PST
> 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.
Comment 12 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-13 18:50:00 PST
Created attachment 596896 [details] [diff] [review]
New version

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).
Comment 13 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-14 01:59:09 PST
Created attachment 596945 [details] [diff] [review]
version that doesn't ICE apple's gcc

This version will hopefully work on apple's gcc too:

https://tbpl.mozilla.org/?tree=Try&rev=0e6de94ecc43
Comment 14 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-14 02:29:02 PST
Created attachment 596951 [details] [diff] [review]
With __USER_LABEL_PREFIX__

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
Comment 15 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-14 06:17:33 PST
Created attachment 596997 [details] [diff] [review]
Correct the use of __USER_LABEL_PREFIX__

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
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-15 17:05:40 PST
(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.)
Comment 17 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-16 04:31:43 PST
> 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 18 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-16 06:03:59 PST
Created attachment 597791 [details] [diff] [review]
rebased and updated for the s/JS_Assert/MOZ_Assert/ change.

https://tbpl.mozilla.org/?tree=Try&rev=02d3408083fa
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-22 16:22:13 PST
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.
Comment 20 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-23 07:23:19 PST
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!
Comment 21 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-23 11:35:18 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f077e2e7e38d
Comment 22 Richard Newman [:rnewman] 2012-02-23 18:39:21 PST
https://hg.mozilla.org/mozilla-central/rev/f077e2e7e38d

Note You need to log in before you can comment on or make changes to this bug.