[lld] 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:12:20 PDT 2023


majaha wrote:

@aheejin I suggest looking at each of the first three commits individually. I carefully crafted and stacked them so that the test passes after each commit. That way, the changes to the test declaratively describe the effect of each commit.
You can click on each commit in turn near the top of this page. Also, when looking at changes, there's a little pulldown near the top right that says `Changes from * commits` that you can use to flick through the different commits.

I probably should have mentioned that earlier. It seems like GitHub doesn't surface this stacked-commit style information as well as it could. I was following nikic's guide for the old pull request system, where I assume this style was better supported.

I'll give a quick overview of what each of the first three commits do:
1. [Add no-trap-after-noreturn flag and wasm tests](https://github.com/llvm/llvm-project/pull/65876/commits/7c43c803764bf9e0256d4e3e9f497d2622bb8f69)
This commit doesn't actually contain any real changes. It simply adds some new tests in `unreachable.ll` that *describe* the current state of affairs, and adds the command line option `--no-trap-after-noreturn` to support those tests. This option was previously usable from C++ projects (clang, rustc etc.), but not from the command line, making it untestable (and thus buggy!).
In particular, the function [`@missing_ret_noreturn_unreach()`](https://github.com/llvm/llvm-project/pull/65876/commits/7c43c803764bf9e0256d4e3e9f497d2622bb8f69#diff-1b973bc4d02665dc4a2d31916df466a5bf53c64ef70aa701ef579fc67dfbd2e4R97) produces invalid Wasm code when NoTrapAfterNoreturn is turned on: it's missing a necessary `unreachable`.

2. [Ensure NoTrapAfterNoreturn is false for wasm](https://github.com/llvm/llvm-project/pull/65876/commits/7e834694b09e6a7787674879442b5f0d0c92eb22)
This is the bug fix commit. I did the simplest possible thing to fix the bug: copy the behaviour for `TargetOptions.TrapUnreachable` and overwrite the value of `TargetOptions.NoTrapAfterNoreturn` with false*. Note that TargetOptions is part of the public API and that these options can be set to anything by llvm's embedders (i.e. clang, rustc).
This is a bit of a hack, but a hack that already existed.
As you can see in the tests, this makes all combinations of the two command line options output the same code, and fixes the invalid output. But there's still those pesky double unreachables, which leads to the next commit:

3. [Add peephole optimisation](https://github.com/llvm/llvm-project/pull/65876/commits/c8ad813e9d5164ace9aa2178f912272762824e91)
This commit adds a simple peephole optimisation that removes unnecessary drops and unreachables near other unreachables. It would be nice if the wasm backend was able to model these properly so as not to add them in the first place, but this fixes the most obvious cases well enough for now. This commit has been reverted and moved to a different PR.


\* Incidently, is this already buggy? We are overwriting our user's TargetOptions, and so if they re-used that object for a different backend they might be surprised by it changing on them. Should we be copying the whole TargetOptions instead?

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


More information about the llvm-commits mailing list