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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added inline comments.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121750