[PATCH] D121750: Add a cmake flag to turn `llvm_unreachable()` into builtin_trap() when assertions are disabled

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In D121750#3397382 <https://reviews.llvm.org/D121750#3397382>, @dexonsmith wrote:

> I suggest:
>
>   #ifndef NDEBUG
>   #define llvm_unreachable(msg) \
>     ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
>   #elif !defined(LLVM_BUILTIN_UNREACHABLE)
>   #define llvm_unreachable(msg) ::llvm::llvm_unreachable_internal()
>   #elif LLVM_UNREACHABLE_OPTIMIZE
>   #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
>   #else
>   #define llvm_unreachable(msg) LLVM_BUILTIN_TRAP, LLVM_BUILTIN_UNREACHABLE
>   #endif
>
> I *think* this will have all the intended effects:
>
> - If asserts are off, use the function (like a fancy assertion).
> - Else, if there is no LLVM_BUILTIN_UNREACHABLE, call the function without args (works again as of https://reviews.llvm.org/D122167).
> - Else, always call LLVM_BUILTIN_UNREACHABLE to suppress warnings about this control flow path...
>   - ... but if !LLVM_UNREACHABLE_OPTIMIZE, first call LLVM_BUILTIN_TRAP to block optimizations.
>   - ... and if LLVM_BUILTIN_TRAP happens to be marked no-return, the extra LLVM_BUILTIN_UNREACHABLE should get cleaned up by the optimizer.

LG:  https://reviews.llvm.org/D122170



================
Comment at: llvm/include/llvm/Support/ErrorHandling.h:144
+#define llvm_unreachable(msg) LLVM_BUILTIN_TRAP
 #elif defined(LLVM_BUILTIN_UNREACHABLE)
 #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
----------------
dexonsmith wrote:
> dexonsmith wrote:
> > mehdi_amini wrote:
> > > dexonsmith wrote:
> > > > Just noticed that this pre-existing logic was wrong.
> > > > 
> > > > LLVM_BUILTIN_UNREACHABLE is defined whenever llvm/Support/Compiler.h is included:
> > > > ```
> > > > lang=c++
> > > > #if __has_builtin(__builtin_unreachable) || defined(__GNUC__)
> > > > # define LLVM_BUILTIN_UNREACHABLE __builtin_unreachable()
> > > > #elif defined(_MSC_VER)
> > > > # define LLVM_BUILTIN_UNREACHABLE __assume(false)
> > > > #else
> > > > # define LLVM_BUILTIN_UNREACHABLE
> > > > #endif
> > > > ```
> > > > That probably didn't used to be true.
> > > > 
> > > > I think the intent of this code is to check whether `LLVM_BUILTIN_UNREACHABLE` is empty. If the Compiler.h logic exports another variable that can be used for detecting that, this could become:
> > > > ```
> > > > lang=c++
> > > > #ifndef NDEBUG
> > > > #define llvm_unreachable(msg) \
> > > >   ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
> > > > #elif LLVM_BUILTIN_UNREACHABLE_NON_EMPTY
> > > > #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
> > > > #else
> > > > #define llvm_unreachable(msg) ::llvm::llvm_unreachable_internal()
> > > > #endif
> > > > ```
> > > > (before your patch)
> > > > 
> > > I wonder if just:
> > > 
> > > ```
> > > #ifndef NDEBUG
> > > #define llvm_unreachable(msg) \
> > >   ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
> > > #elif!LLVM_UNREACHABLE_OPTIMIZE
> > > #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
> > > #else
> > > #define llvm_unreachable(msg) LLVM_BUILTIN_TRAP
> > > #endif
> > > ```
> > > 
> > > Wouldn't be enough? Yes it would accounts to "nothing" when LLVM_BUILTIN_UNREACHABLE is empty, but that seems just correct and maybe even desirable: when LLVM_UNREACHABLE_OPTIMIZE is enabled  "do nothing" 
> > > seems closer to the behavior we're looking for rather than calling `llvm_unreachable_internal()` right?
> > > 
> > Did you mean:
> > ```
> > lang=c++
> > #ifndef NDEBUG
> > #define llvm_unreachable(msg) \
> >   ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
> > #elif LLVM_UNREACHABLE_OPTIMIZE
> > #define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
> > #else
> > #define llvm_unreachable(msg) LLVM_BUILTIN_TRAP
> > #endif
> > ```
> > ? (Or maybe I'm reading it backwards)
> > 
> > 
> > Regardless, one of the important pieces of `llvm_unreachable()` is that it suppresses diagnostics about the control path it's on. If it expands to "empty"/nothing then it won't suppress diagnostics.
> > 
> > I think the behaviour here changed accidentally. @aeubanks updated LLVM_BUILTIN_UNREACHABLE as a drive-by in 26827337dff26ba3450721f880d4c6caaf2a8219 / https://reviews.llvm.org/D111581:
> > ```
> > -#if __has_builtin(__builtin_unreachable) || LLVM_GNUC_PREREQ(4, 5, 0)
> > +#if __has_builtin(__builtin_unreachable) || defined(__GNUC__)
> >  # define LLVM_BUILTIN_UNREACHABLE __builtin_unreachable()
> >  #elif defined(_MSC_VER)
> >  # define LLVM_BUILTIN_UNREACHABLE __assume(false)
> > +#else
> > +# define LLVM_BUILTIN_UNREACHABLE
> >  #endif
> > ```
> > >     Add a missing #endif for LLVM_ATTRIBUTE_UNREACHABLE.
> > 
> > Before that, the fallback behaviour was to hit the `#else` case here so that `llvm_unreachable()` always expanded to something that was no-return.
> > 
> > Maybe we should revert that part of 26827337dff26ba3450721f880d4c6caaf2a8219 patch before landing this.
> Done in https://reviews.llvm.org/D122167 / 892c104fb71b86dc6399e36a82ae920e00ccae17.
> Regardless, one of the important pieces of llvm_unreachable() is that it suppresses diagnostics about the control path it's on. If it expands to "empty"/nothing then it won't suppress diagnostics.

Ah I forgot about this. That said: when you encounter a toolchain that does not support a proper "unreachable", that would just mean they don't have a way to suppress the diagnostics and have to live with warnings. For the `*(volatile int*)0x11 = 0`, you can wrap it in a macro with a noreturn attribute: `[]() __attribute__((__noreturn__)) { *(volatile int*)0x11 = 0; }();` (the same may apply to Windows debugbreak() builtin by the way).

Anyway I think your solution will work just fine!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121750/new/

https://reviews.llvm.org/D121750