Skip to content

Duplicated WASM unreachable instructions #102132

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

Open
Expyron opened this issue Sep 22, 2022 · 8 comments
Open

Duplicated WASM unreachable instructions #102132

Expyron opened this issue Sep 22, 2022 · 8 comments
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Expyron
Copy link
Contributor

Expyron commented Sep 22, 2022

Targets: wasm32-unknown-unknown, wasm32-wasi

When calling core::arch::wasm32::unreachable(), the generated WASM contains two unreachable instructions instead of only one.

Example:

pub fn main(){
    core::arch::wasm32::unreachable();
}

Output (from Godbolt):

example::main:
        unreachable
        unreachable
        end_function

Godbolt link: https://rust.godbolt.org/z/Yqz6Kh9eq

This is mostly noticeable when using build-std-features=panic_immediate_abort (which I know is unstable), as all instances of panic!() are basically replaced with unreachable(), and then compile down to two unreachable instructions.

This behavior appears to have been present since core::arch::wasm32::unreachable() was made available, so this is not a regression.

@Expyron Expyron added the C-bug Category: This is a bug. label Sep 22, 2022
@Rageking8
Copy link
Contributor

@rustbot label +A-codegen +O-wasm

@rustbot rustbot added A-codegen Area: Code generation O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Sep 22, 2022
@Noratrieb
Copy link
Member

This isn't just a WASM thing but a general codegen thing where LLVM still inserts the TrapUnreachable trap after a call to llvm.trap. See this https://godbolt.org/z/a7Wv58EPo for a simpler example.

@Noratrieb
Copy link
Member

@rustbot label -O-wasm +A-llvm

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Sep 25, 2022
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
@majaha
Copy link
Contributor

majaha commented Apr 17, 2023

I'm having a look into this.

The x86 and Wasm examples may not be entirely related, as llc targeting x86 doesn't exhibit this bug: https://godbolt.org/z/qcqhYbqz5

Even going back to older llc versions in Godbolt doesn't reproduce the bug, so I assume for the x86 case it's either a bug in rustc's llvm fork, or an issue with the way rustc is using llvm.

@majaha
Copy link
Contributor

majaha commented Apr 17, 2023

I've identified a suspicious unstable rustc option -Z trap-unreachable=false which makes the x86 example work as expected, but not the Wasm version: https://godbolt.org/z/7ja4nY77e This probably accounts for the difference I was seeing with llc.

Another function of interest is std::hint::unreachable_unchecked: https://godbolt.org/z/5d4sEePb7

Among the various parties (rustc, llvm, wasm), there are different understandings of what "unreachable" means. I wonder if this is contributing to the problems.

@majaha
Copy link
Contributor

majaha commented Apr 17, 2023

I also found this related llvm issue: https://reviews.llvm.org/D66980

@majaha
Copy link
Contributor

majaha commented Apr 18, 2023

It turns out LLVM has a NoTrapAfterNoreturn to complement the TrapUnreachable option. Making rustc use that option fixes the x86 case.

I found this issue: #88826 about turning TrapUnreachable off by default. @nikic do you think it's safe to turn TrapUnreachable off yet? If not, would a pull request enabling NoTrapAfterNoReturn be welcome?

I still have yet to investigate the Wasm case more thoroughly.

@nikic
Copy link
Contributor

nikic commented Apr 18, 2023

@majaha I'd suggest filing an MCP (https://forge.rust-lang.org/compiler/mcp.html) for enabling NoTrapAfterNoReturn. I think doing this is fine, but other people may have concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants