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

Matt Harding via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 21 03:54:54 PDT 2023


================
@@ -127,6 +127,7 @@ WebAssemblyTargetMachine::WebAssemblyTargetMachine(
   // LLVM 'unreachable' to ISD::TRAP and then lower that to WebAssembly's
   // 'unreachable' instructions which is meant for that case.
   this->Options.TrapUnreachable = true;
+  this->Options.NoTrapAfterNoreturn = false;
----------------
majaha wrote:

> This option is false by default and since we haven't overridden it, it has been false so far. What effect does this line have then?

While the option is false by default, it is part of the public API and can be set to true by llvm's users (e.g. clang, rustc). This has been available from the C++ API for a while now, I've just added the ability to use it from the command line (and so the ability to actually test it!).

> Even if you give --no-trap-after-noreturn to the command line, this line will override it. Is that a bug or your intention?

Yes, that's the intention.

> If it is your intention, did you want to check if we can override it, i.e., ignore it, even if we are given --no-trap-after-noreturn, and was that the reason you added the command line flag?

Yes that's more-or-less the point. Except that it's not just the command line option we need to ignore, we also need to override NoTrapAfterNoreturn when it's passed to us in C++.

> If that's the case I'm still not sure it's worth adding a flag, especially to the common file in lib/CodeGen that can be used by all targets.

I have to disagree here: This is clearly a sensitive and bug-prone option for WebAssembly, and we should be able to test that this bug doesn't return.
Also, there is the more general argument that if the option exists, we should be able to test it. This feels in line with llvm's highly pro-testing ethos to me.
The option also affects other backends, and adding a command line option is useful for observing and testing its effects on them too. Previously this was impossible. I think we're justified in editing a common file to turn the obscured and untestable into the exposed and testable.

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


More information about the llvm-commits mailing list