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

Mon Mar 21 11:38:23 PDT 2022

dexonsmith added subscribers: rnk, aeubanks.
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
----------------
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.

================
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:
> dexonsmith wrote:
> > 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
> > ```
> > 
> Right, that would be a valid path. But what about another path of making LLVM_BUILTIN_TRAP correctly "noreturn" on Windows as well? I'm not a Windows user but someone more versed into Windows may be able to help on this?
Maybe the windows case could be fixed with another intrinsic, other than `__debugbreak`. Note that `__debugbreak()` can return. From https://docs.microsoft.com/en-us/cpp/intrinsics/debugbreak?view=msvc-170 :
> Causes a breakpoint in your code, where the user will be prompted to run the debugger.

Maybe @rnk has an idea for something else.

Even with that fixed, is there a way to mark the fall-through case as "no-return"?
```
# define LLVM_BUILTIN_TRAP *(volatile int*)0x11 = 0
```

That said, I'm not sure if making `builtin_trap()` no-return needs to be a blocker for this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121750