[llvm] Ensure NoTrapAfterNoreturn is false for the wasm backend (PR #65876)

Heejin Ahn via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 20 17:05:18 PDT 2023


================
@@ -37,6 +37,11 @@ static cl::opt<bool>
     EnableTrapUnreachable("trap-unreachable", cl::Hidden,
                           cl::desc("Enable generating trap for unreachable"));
 
+static cl::opt<bool> EnableNoTrapAfterNoreturn(
+    "no-trap-after-noreturn", cl::Hidden,
+    cl::desc("Do not emit a trap instruction for 'unreachable' IR instructions "
+             "after noreturn calls, even if --trap-unreachable is set."));
----------------
aheejin wrote:

> I think you may have missed [this change](https://github.com/llvm/llvm-project/pull/65876/files#diff-174c1ba15a00ab323f265d131a24ddd8854ab4ce70133d83ec52972f188f9559R130), which is the most important line and the actual bug fix.

Why is this a bugfix? It has been false so far anyway.

> The point is that code external to llvm can pass NoTrapAfterNoreturn as True, just as it can pass TrapUnreachable as False. However, the wasm backend is unusual in that it (ab)uses TrapUnreachable to emit correct code. And so in the WebAssemblyTargetMachine constructor, TrapUnreachable is hard-coded to True. My change is that NoTrapAfterNoreturn must also be hard-coded to False to always emit correct code. This is because NoTrapAfterNoreturn has the effect of suppressing TrapUnreachable after a noreturn call, which can cause invalid wasm.
> 
> > And why does the rust code need to turn that option on?
> 
> [Rust has had TrapUnreachable on for a while now](https://github.com/rust-lang/rust/pull/45920), because it worked around a few [nasty soundness bugs](https://github.com/rust-lang/rust/issues/28728), and it limits the effect of undesired undefined behaviour in some cases. They don't seem confident that it's safe to turn off yet. Rust _wants_ to turn NoTrapAfterNoreturn on across all llvm backends, because it cuts down on unnecessary trap instructions being emitted after noreturn calls, a common case in Rust code, and should be safe. You also see [silly double traps](https://godbolt.org/z/1z6vMGavz) when the code is intentionally trying to trap, because llvmir's `@llvm.trap()` is a noreturn function. This is what I was trying to fix up with my wasm peephole optimisation, removing obviously unnecessary double `unreachable`s, to make up for the fact that we can't use NoTrapAfterNoreturn on the wasm backend.

Can you give an example of when it is safe to set `NoTrapAfterNoreturn` on and when it is not? 

Btw the current PR makes setting `--no-trap-after-noreturn` true impossible, because you overrided it in `WebAssemblyTargetMachine.cpp`.

https://github.com/llvm/llvm-project/pull/65876


More information about the llvm-commits mailing list