Skip to content
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

[WebAssembly] Fix conflict between ret legalization and sjlj #25

Conversation

@tlively
Copy link

commented Oct 2, 2019

Cherry-picks support for passing aggregates by value in the transformation necessary to implement Emscripten's exception handling in the upstream WebAssembly LLVM backend. This will allow exception handling on Emscripten targets to be re-enabled after rust-lang/rust#63649.

Summary:
When the WebAssembly backend encounters a return type that doesn't
fit within i32, SelectionDAG performs sret demotion, adding an
additional argument to the start of the function that contains
a pointer to an sret buffer to use instead. However, this conflicts
with the emscripten sjlj lowering pass. There we translate calls like:

	call {i32, i32} @foo()

into (in pseudo-llvm)

	%addr = @foo
	call {i32, i32} @__invoke_{i32,i32}(%addr)

i.e. we perform an indirect call through an extra function.
However, the sret transform now transforms this into
the equivalent of

        %addr = @foo
        %sret = alloca {i32, i32}
        call {i32, i32} @__invoke_{i32,i32}(%sret, %addr)

(while simultaneously translation the implementation of @foo as well).
Unfortunately, this doesn't work out. The _invoke ABI expected
the function address to be the first argument, causing crashes.

There is several possible ways to fix this:

  1. Implementing the sret rewrite at the IR level as well and performing
    it as part of lowering to __invoke
  2. Fixing the wasm backend to recognize that __invoke has a special ABI
  3. A change to the binaryen/emscripten ABI to recognize this situation

This revision implements the middle option, teaching the backend to
treat _invoke functions specially in sret lowering. This is achieved
by

  1. Introducing a new CallingConv ID for invoke functions
  2. When this CallingConv ID is seen in the backend and the first argument
    is marked as sret (a function pointer would never be marked as sret),
    swapping the first two arguments.

Reviewed By: tlively, aheejin
Differential Revision: https://reviews.llvm.org/D65463

llvm-svn: 367935

Summary:
When the WebAssembly backend encounters a return type that doesn't
fit within i32, SelectionDAG performs sret demotion, adding an
additional argument to the start of the function that contains
a pointer to an sret buffer to use instead. However, this conflicts
with the emscripten sjlj lowering pass. There we translate calls like:

```
	call {i32, i32} @foo()
```

into (in pseudo-llvm)
```
	%addr = @foo
	call {i32, i32} @__invoke_{i32,i32}(%addr)
```

i.e. we perform an indirect call through an extra function.
However, the sret transform now transforms this into
the equivalent of
```
        %addr = @foo
        %sret = alloca {i32, i32}
        call {i32, i32} @__invoke_{i32,i32}(%sret, %addr)
```
(while simultaneously translation the implementation of @foo as well).
Unfortunately, this doesn't work out. The __invoke_ ABI expected
the function address to be the first argument, causing crashes.

There is several possible ways to fix this:
1. Implementing the sret rewrite at the IR level as well and performing
   it as part of lowering to __invoke
2. Fixing the wasm backend to recognize that __invoke has a special ABI
3. A change to the binaryen/emscripten ABI to recognize this situation

This revision implements the middle option, teaching the backend to
treat __invoke_ functions specially in sret lowering. This is achieved
by
1) Introducing a new CallingConv ID for invoke functions
2) When this CallingConv ID is seen in the backend and the first argument
   is marked as sret (a function pointer would never be marked as sret),
   swapping the first two arguments.

Reviewed By: tlively, aheejin
Differential Revision: https://reviews.llvm.org/D65463

llvm-svn: 367935
@nikic

This comment has been minimized.

Copy link
Collaborator

commented Oct 2, 2019

This would make us nominally incompatible with LLVM 9 -- though I guess that wasm isn't really relevant for the x-lang LTO use case, so probably fine?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Yeah I don't think there's a ton of users of x-lang LTO and wasm, and tbh there's tools like binaryen which postprocess the "final executable" to do LTO there, which greatly reduces the need for x-lang LTO anyway.

I'm gonna go ahead and merge this since it looks like it will be necessary for the emscripten targets, and otherwise I don't think this should cause issues for our other targets.

@tlively

This comment has been minimized.

Copy link
Author

commented Oct 7, 2019

@alexcrichton Did you mean to merge this?

@alexcrichton alexcrichton merged commit 14a3b12 into rust-lang:rustc/9.0-2019-09-19 Oct 7, 2019
@alexcrichton

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Er oops, yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.