[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/CMakeLists.txt:468
 option(LLVM_ENABLE_DUMP "Enable dump functions even when assertions are disabled" OFF)
+option(LLVM_UNREACHABLE_OPTIMIZE "Whether llvm_unreachable() should be considered undefined behavior (default) or instead trap when assertions are disabled" ON)
 
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > dexonsmith wrote:
> > > dexonsmith wrote:
> > > > I wonder if the wording should be tied to the word "optimize". Also, note that trapping is a legit implementation of undefined behaviour; so another interpretation is that `llvm_unreachable()` is being treated as UB either way.
> > > > 
> > > > E.g.:
> > > > 
> > > > > A "reached" llvm_unreachable() is undefined behaviour; ON optimizes the unreachable control flow (default), while OFF triggers a guaranteed trap
> > > > 
> > > > WDYT?
> > > > (I'm okay with landing the wording as you had it if you prefer it; can always update it later)
> > > Or maybe this should be short and sweet:
> > > 
> > > > Optimize control flow based on llvm_unreachable(); guaranteed trap when OFF
> > > 
> > > (and leave the longer explanation for llvm/docs/...)
> > Trapping is a legit implementation of UB, but the opposite isn't and this is the important bit I was trying to express. 
> > I see what you mean with "optimize" but it may not be obvious for everyone that we're turning a trap into UB here.
> What about: "Optimize llvm_unreachable() as undefined behavior (default), guaranteed trap when OFF"?
Sure, sounds fine to me!


================
Comment at: llvm/include/llvm/Support/ErrorHandling.h:134-135
+///   program.
+/// * When "OFF", a builtin_trap is emitted instead of involving undefined
+///   behavior.
 ///
----------------
I'd prefer one of:
```
/// * When "OFF", a builtin_trap is emitted instead.
/// * When "OFF", a builtin_trap is emitted instead of enabling
//    optimizations or printing a reduced message.
/// * When "OFF", a builtin_trap is emitted instead of an
//    optimizer hint or printing a reduced message.
```
But your text here LGTM too; I don't think it needs to be perfect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121750