Skip to content

Cherry-pick NoTrapAfterNoreturn fix for wasm backend #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

majaha
Copy link

@majaha majaha commented Oct 26, 2023

In trying to enable the option NoTrapAfterNoreturn in rustc, we discovered a bug with that option in the WebAssembly back-end.
The fix was merged into llvm upstream: llvm#65876

And here's the relevant Rust issue, which I'll update when this PR is merged.
rust-lang/rust#110494

In the WebAssembly back end, the TrapUnreachable option is currently
load-bearing for correctness, inserting wasm `unreachable` instructions
where needed to create valid wasm. There is another option,
NoTrapAfterNoreturn, that removes some of those traps and causes
incorrect wasm to be emitted.

This turns off `NoTrapAfterNoreturn` for the Wasm backend and adds new   
tests.
@cuviper
Copy link
Member

cuviper commented Oct 27, 2023

Can you propose this as an upstream llvm-17 backport? That's the preferred route if you can land it that way.

@nikic
Copy link

nikic commented Oct 27, 2023

I think we should special case this on the rust side instead. That has the benefit that we don't need to make the behavior LLVM-version specific.

@majaha
Copy link
Author

majaha commented Oct 29, 2023

Which do we think is the best path here? Or maybe it's both?

@cuviper
Copy link
Member

cuviper commented Oct 31, 2023

@nikic It's not clear to me what you're suggesting.

@nikic
Copy link

nikic commented Oct 31, 2023

@cuviper We can enable NoTrapAfterNoreturn for non-wasm targets only. This backport force-disables the option for wasm, but we can also not enable it in the first place in rustc.

@cuviper
Copy link
Member

cuviper commented Oct 31, 2023

OK, @majaha can you try that in rustc? Then we don't need a backport at all, and the special case can be commented to be removed once LLVM 18 is our minimum supported version.

@majaha
Copy link
Author

majaha commented Nov 1, 2023

Okay, I've pushed a patch over at the other pull request. Let me know what you think.

@nikic
Copy link

nikic commented Nov 8, 2023

Closing this, as we no longer need this backport with the updated implementation in rust-lang/rust#110494.

@nikic nikic closed this Nov 8, 2023
vext01 pushed a commit to vext01/llvm-project that referenced this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants