rust-lang / rustc_codegen_cranelift

Cranelift based backend for rustc
Apache License 2.0
1.61k stars 100 forks source link

s390x test failure due to unsupported stack realignment #1258

Closed uweigand closed 6 months ago

uweigand commented 2 years ago

On s390x, the test case ptr_bitops_tagging in sysroot_src/library/core/tests/atomic.rs is failing. This is because the test attempts to allocate a 16-byte aligned variable on the stack:

    #[repr(align(16))]
    struct Tagme(u128);

and then verifies that this variable is indeed 16-byte aligned.

On s390x, the default stack alignment is only 8 bytes, therefore allocating a 16-byte aligned variable on the stack would require dynamic stack re-alignment. This is not currently implemented in the cranelift backend. In the alternative, rustc_codegen_cranelift could implement re-alignment itself by allocating a larger buffer and adjusting its address, but it doesn't look like this is being done either.

Should this be implemented at some point, either in the front end or back end?

For now, would it be OK to disable this test on s390x, to make the test suite pass?

bjorn3 commented 2 years ago

I think it should be done in the backend as the backend knows the default stack alignment and would be able to collapse re-alignment for multiple stack slots. For now adding #[cfg_attr(target_arch = "s390x", ignore)] // s390x backend doesn't support stack alignment >8 bytes in the patches/0023-sysroot-Ignore-failing-tests.patch patch is fine.

bjorn3 commented 2 years ago

Specifying it in the backend would also allow removing this hack that depends on the stack alignment being 16 bytes on x86_64: https://github.com/bjorn3/rustc_codegen_cranelift/blob/64c73d0b3c4b91fee0e9a840be30e1d6faac7957/src/abi/pass_mode.rs#L187-L195

(The cmp::max needs to stay, but the rounding to 16 bytes can then be removed.)

uweigand commented 2 years ago

For now adding #[cfg_attr(target_arch = "s390x", ignore)] // s390x backend doesn't support stack alignment >8 bytes in the patches/0023-sysroot-Ignore-failing-tests.patch patch is fine.

https://github.com/bjorn3/rustc_codegen_cranelift/pull/1260

bjorn3 commented 6 months ago

Fixed in https://github.com/rust-lang/rustc_codegen_cranelift/commit/b03b41420b2dc900a9db019f4b5a5c22c05d2bb8