Need a macro that expands to __builtin_unreachable on release builds

RESOLVED FIXED in mozilla13

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla13
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

3.88 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
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.
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
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #594100 - Flags: review?(ted.mielczarek)
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.
Attachment #594100 - Attachment is obsolete: true
Attachment #594100 - Flags: review?(ted.mielczarek)
Attachment #594103 - Flags: review?(ted.mielczarek)

Updated

5 years ago
Assignee: respindola → nobody
Component: General → MFBT
QA Contact: general → mfbt
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.
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
Assignee: nobody → respindola
Attachment #594103 - Attachment is obsolete: true
Attachment #594103 - Flags: review?(ted.mielczarek)
Attachment #594113 - Flags: review?(ted.mielczarek)
Blocks: 723864
Attachment #594113 - Flags: review?(ted.mielczarek)
Attachment #594113 - Flags: review?(jones.chris.g)
Attachment #594113 - Flags: feedback?(jwalden+bmo)
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
Attachment #594113 - Attachment is obsolete: true
Attachment #594113 - Flags: review?(jones.chris.g)
Attachment #594113 - Flags: feedback?(jwalden+bmo)
Attachment #594133 - Flags: review?(jones.chris.g)
Attachment #594133 - Flags: feedback?(jwalden+bmo)
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"
Attachment #594133 - Flags: feedback?(jwalden+bmo)
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.
Attachment #594133 - Flags: review?(jones.chris.g)
(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.
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).
Attachment #594133 - Attachment is obsolete: true
Attachment #596896 - Flags: review?(jwalden+bmo)
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
Attachment #596896 - Attachment is obsolete: true
Attachment #596896 - Flags: review?(jwalden+bmo)
Attachment #596945 - Flags: review?(jwalden+bmo)
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
Attachment #596945 - Attachment is obsolete: true
Attachment #596945 - Flags: review?(jwalden+bmo)
Attachment #596951 - Flags: review?(jwalden+bmo)
Blocks: 723534
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
Attachment #596951 - Attachment is obsolete: true
Attachment #596951 - Flags: review?(jwalden+bmo)
Attachment #596997 - Flags: review?(jwalden+bmo)
(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?
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
Attachment #596997 - Attachment is obsolete: true
Attachment #596997 - Flags: review?(jwalden+bmo)
Attachment #597791 - Flags: review?(jwalden+bmo)
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.
Attachment #597791 - Flags: review?(jwalden+bmo) → review+
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!
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f077e2e7e38d
https://hg.mozilla.org/mozilla-central/rev/f077e2e7e38d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.