[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
----------------
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)



================
Comment at: llvm/include/llvm/Support/ErrorHandling.h:143
+#elif LLVM_UNREACHABLE_OPTIMIZE
+#define llvm_unreachable(msg) LLVM_BUILTIN_TRAP
 #elif defined(LLVM_BUILTIN_UNREACHABLE)
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > dexonsmith wrote:
> > > Based on:
> > > > 7>C:\src\upstream\llvm_clean_git\llvm\lib\Support\Compression.cpp(104): error C4716: 'llvm::zlib::uncompress': must return a value
> > > 
> > > it looks like `LLVM_BUILTIN_TRAP` isn't guaranteed to be something the compiler understands as `noreturn`. Maybe this logic needs to fallback to `llvm_unreachable_internal` if `LLVM_BUILTIN_TRAP` is not a function.
> > The bug is that when you suggested a renaming the macro I didn't inverse the condition: the previous name was something like `SHOULD_TRAP` but with the new name the logic is reversed! I need `#elif !LLVM_UNREACHABLE_OPTIMIZE` I think.
> I fixed the condition but I'm not sure how to address the windows situation. I think I'd try to add an attribute or something to make it "noreturn" there as well, but rather leave it to someone who could actually test it.
Hah, that's what I thought at first, and then I convinced myself that it wasn't wrong and jumped to the other conclusion :).

For the windows thing, maybe don't rely on `LLVM_BUILTIN_TRAP` for now, if it's not guaranteed to be `noreturn`. Could change the builtin trap logic to export another variable:
```
lang=c++
#if __has_builtin(__builtin_trap) || defined(__GNUC__)
# define LLVM_BUILTIN_TRAP __builtin_trap()
# define LLVM_BUILTIN_TRAP_NO_RETURN 1
#elif defined(_MSC_VER)
// The __debugbreak intrinsic is supported by MSVC, does not require forward
// declarations involving platform-specific typedefs (unlike RaiseException),
// results in a call to vectored exception handlers, and encodes to a short
// instruction that still causes the trapping behavior we want.
# define LLVM_BUILTIN_TRAP __debugbreak()
# define LLVM_BUILTIN_TRAP_NO_RETURN 0
#else
# define LLVM_BUILTIN_TRAP *(volatile int*)0x11 = 0
# define LLVM_BUILTIN_TRAP_NO_RETURN 0
#endif
```
Assuming something similar creates a variable `LLVM_BUILTIN_UNREACHABLE_NON_EMPTY` (to fix the bug mentioned in the other inline comment):
```
lang=c++
#ifndef NDEBUG
#define llvm_unreachable(msg) \
  ::llvm::llvm_unreachable_internal(msg, __FILE__, __LINE__)
#elif LLVM_UNREACHABLE_OPTIMIZE && LLVM_BUILTIN_UNREACHABLE_NON_EMPTY
#define llvm_unreachable(msg) LLVM_BUILTIN_UNREACHABLE
#elif LLVM_BUILTIN_TRAP_NO_RETURN
#define llvm_unreachable(msg) LLVM_BUILTIN_TRAP
#else
#define llvm_unreachable(msg) ::llvm::llvm_unreachable_internal()
#endif
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121750