Stream: git-wasmtime

Topic: wasmtime / Issue #1510 Always check if struct-return para...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 10:51):

github-actions[bot] commented on Issue #1510:

Subscribe to Label Action

cc @bnjbvr

<details>
This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 11:14):

teapotd commented on Issue #1510:

Ugh, looks like it uncovered another issue - on i686 return pointer parameter will be on stack, but we're assuming it will always be in a register:

https://github.com/bytecodealliance/wasmtime/blob/c5b6c57c34f89d865e07939eff67d6687e9339e2/cranelift/codegen/src/isa/x86/abi.rs#L287-L293

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 12:55):

teapotd commented on Issue #1510:

There was one test that didn't pass (cranelift/filetests/filetests/legalizer/popcnt-i128.clif) and it looks like its target should be x86_64, not i686 (see commit). I changed it to x86_64.

It looks like function signature legalization for i686 is incomplete. In particular, it doesn't differentiate between registers designated for return values on i686 and x64. I think it should be discussed in separate issue.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 17:07):

bjorn3 commented on Issue #1510:

For SystemV I believe i128 should be passed and returned in two registers, like it currently does, instead of through sret.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 16:52):

teapotd commented on Issue #1510:

@bjorn3

Okay, i128 arguments are definitely handled incorrectly and I'm not sure anymore how i128 return values should be handled. The problem is that Microsoft ABI doesn't support 128-bit integer type. It defines __m128 vector type, but that's different thing.

A scalar return value that can fit into 64 bits is returned through RAX; this includes __m64 types. Non-scalar types including floats, doubles, and vector types such as __m128, __m128i, __m128d are returned in XMM0. [...] User-defined types can be returned by value from global functions and static member functions. To return a user-defined type by value in RAX, it must have a length of 1, 2, 4, 8, 16, 32, or 64 bits. [...] Otherwise, the caller assumes the responsibility of allocating memory and passing a pointer for the return value as the first argument. Subsequent arguments are then shifted one argument to the right. The same pointer must be returned by the callee in RAX.

So if i128 is different from __m128 it should be treated like user-defined type and returned via sret. That's what rustc does for win64 calling convention and what Cranelift would do with this change. On the other hand, gcc and clang treat int128 like __m128.

<details><summary>msvc</summary>

#include <xmmintrin.h>
__m128 test_m128(__m128 x) {
    return x;
}
x$ = 8
__m128 test_m128(__m128) PROC           ; test_m128, COMDAT
        movups  xmm0, XMMWORD PTR [rcx]
        ret     0
__m128 test_m128(__m128) ENDP           ; test_m128

</details>

<details><summary>gcc</summary>

__attribute__((ms_abi)) __int128 test_i128(__int128 x) {
    return x;
}
test_i128(__int128):
        movdqa  xmm0, XMMWORD PTR [rcx]
        ret

</details>

<details><summary>clang</summary>

__attribute__((ms_abi)) __int128 test_i128(__int128 x) {
    return x;
}
test_i128(__int128):                          # @test_i128(__int128)
        movaps  xmm0, xmmword ptr [rcx]
        ret

</details>

<details><summary>rustc</summary>

pub extern "win64" fn test_i128(x: i128) -> i128 {
    x
}
example::test_i128:
        mov     rax, rcx
        movups  xmm0, xmmword ptr [rdx]
        movups  xmmword ptr [rcx], xmm0
        ret

</details>

Regarding the i128 arguments:

Any argument that doesn't fit in 8 bytes, or isn't 1, 2, 4, or 8 bytes, must be passed by reference. A single argument is never spread across multiple registers. [...] 16-byte arguments are passed by reference.

From what I see, Cranelift doesn't support specifying that argument should be passed by reference.

Microsoft spec: https://docs.microsoft.com/en-us/cpp/build/x64-calling-convention
Compilers comparison: https://godbolt.org/z/bS9-FG

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 17:14):

bjorn3 commented on Issue #1510:

I would prefer matching the rustc abi for all calling conventions. This means splitting into two registers for SystemV and passing by-ref for WindowsFastCall. Matching the rustc abi is important for cg_clif to eventually be able to call between it and cg_llvm (the default backend for rustc)

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 16:02):

teapotd commented on Issue #1510:

In the current state, return values should work properly for win64 fastcall. I think fixing problems with argument passing on win64 deserves a separate PR, as it requires implementing passing-by-ref.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 13:07):

bjorn3 commented on Issue #1510:

Can somebody review this PR? This is necessary for Windows support in cg_clif.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 14:50):

bjorn3 commented on Issue #1510:

@teapotd I just tried this and the following resulted in a panic:

function u0:29(i128) -> i128 windows_fastcall {
    sig0 = (i128) -> i128 windows_fastcall
    fn0 = u0:35 sig0

block0(v0: i128):
    trap user0

block1:
    v6 = call fn0(v0)
    trap user0
}
thread 'main' panicked at 'Signature still wrong: v11 = call fn0(v0, v10), sig0(i64 [%18], i64 [%24], i64 sret [%17]) -> i64 sret [%16] windows_fastcall', cranelift/codegen/src/legalizer/boundary.rs:773:5
stack backtrace:
[...]
  12: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:332
  13: cranelift_codegen::legalizer::boundary::handle_call_abi
             at cranelift/codegen/src/legalizer/boundary.rs:773
  14: cranelift_codegen::legalizer::legalize_inst
             at cranelift/codegen/src/legalizer/mod.rs:58
  15: cranelift_codegen::legalizer::legalize_function
             at cranelift/codegen/src/legalizer/mod.rs:170
  16: cranelift_codegen::context::Context::legalize
             at cranelift/codegen/src/context.rs:319
  17: cranelift_codegen::context::Context::compile
             at cranelift/codegen/src/context.rs:167
  18: cranelift_codegen::context::Context::compile_and_emit
             at cranelift/codegen/src/context.rs:131
[...]

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 15:47):

teapotd commented on Issue #1510:

It looks like arguments to sret calls are not legalized at all (see legalize_sret_call and handle_call_abi). For example this gives the same panic on system_v:

function u0:29(i128) {
    sig0 = (i128) -> i64, i64, i64, i64 system_v
    fn0 = u0:35 sig0
block0(v0: i128):
    v1, v2, v3, v4 = call fn0(v0)
    trap user0
}

I'll look into this.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 19:29):

teapotd commented on Issue #1510:

@bjorn3 It should be okay now. I think you should try #1670 instead, as it contains this changes and fixes incoming arguments as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 19:32):

bjorn3 commented on Issue #1510:

I was trying to get it compiling first. With the latest changes and a few changes at my side to shim some inline asm with aborts it now compiles libstd fine. Linking fails for cross-compiling from linux using mingw:

  = note: x86_64-w64-mingw32-gcc: error: rsbegin.o: No such file or directory
          x86_64-w64-mingw32-gcc: error: rsend.o: No such file or directory

This is probably a problem with my setup though.

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2020 at 20:10):

bjorn3 commented on Issue #1510:

I had to copy a few object files from the cg_llvm sysroot, disable the atomic shim in cg_clif and update object for a bugfix. It now gives the following when run in wine:

wine ./target/out/std_example.exe                                                                                       bjorn@laptopbjorn-lenovo
some <unknown> text
cargo:rustc-link-lib=z
9.974182
4.9245777
2.3
1.5165751
2
3
1
2.3
5.29
1.2016338
5.29
wine: Unhandled page fault on read access to 0x00000000 at address 0x8a8a39 (thread 002a), starting debugger...

will try the other PR tomorrow.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 09:08):

bjorn3 edited a comment on Issue #1510:

I had to copy a few object files from the cg_llvm sysroot, disable the atomic shim in cg_clif and update object for a bugfix. It now gives the following when run in wine:

$ wine ./target/out/std_example.exe
some <unknown> text
cargo:rustc-link-lib=z
9.974182
4.9245777
2.3
1.5165751
2
3
1
2.3
5.29
1.2016338
5.29
wine: Unhandled page fault on read access to 0x00000000 at address 0x8a8a39 (thread 002a), starting debugger...

will try the other PR tomorrow.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 10:07):

bjorn3 commented on Issue #1510:

The problem was in a compiler-builtins hack to match the abi LLVM expects. It uses #[repr(simd)] which is not implemented in cg_clif.

https://github.com/rust-lang/compiler-builtins/blob/2541f27e8c3d61505815c0492c045b7d17436e35/src/macros.rs#L160

simple-raytracer works fine now in wine.

view this post on Zulip Wasmtime GitHub notifications bot (May 26 2020 at 10:21):

bjorn3 edited a comment on Issue #1510:

The problem was in a compiler-builtins hack to match the abi LLVM expects. It uses #[repr(simd)] which is not implemented in cg_clif.

https://github.com/rust-lang/compiler-builtins/blob/2541f27e8c3d61505815c0492c045b7d17436e35/src/macros.rs#L160

simple-raytracer now works fine in wine.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 23:05):

teapotd commented on Issue #1510:

@fitzgen Could you take a look at this and related #1670?


Last updated: Dec 23 2024 at 12:05 UTC