improper_ctypes lint: allow 128 bit integers for the sysv64 ABI

[est31]

For the sysv64 ABI, 128 bit integers are well defined.

Fixes #78473.

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[nagisa]

If my memory is not wrong, i/u128 implementation suggests that although this is specified in the ABI documentation, the implementations of the ABI for i/u128 are sometimes just plain wrong, either in LLVM or other compilers.

If we're doing this, we should make absolutely sure code we generate is as specified. A codegen test (for at least T1 architectures, but more ideally for all T2 architectures that have a sysv64 document for them) would be great. A run-pass one could be good too.

[est31]

@nagisa yes I remember that being a problem. There is src/test/ui/abi/numbers-arithmetic/i128-ffi.rs which already has some ABI tests. Do you think it needs any extensions?

[nagisa]

It only really checks for extern "C" behaviour, on a limited set of platforms and against C compiler (which we saw being broken in the past as well). This PR affects specifically sysv and is thus relevant to windows targets as well, which aren't tested by that test either.

I'd love to see something a test that checked our compliance with the SysV ABI documents themselves before we commit to something like this.

[nagisa]

Though if we find that other compilers continue mis-compiling i128-containing signatures, I'd argue that the type should remain ffi-unsafe regardless of what the ABI documents say.

[est31]

It only really checks for extern "C" behaviour, on a limited set of platforms

My thought was that extern "C" is equivalent to extern "sysv64" on the targets that use it, and there are such targets in the CI matrix (linux, etc). But you do make a point that it doesn't test whether it works on windows. There might be LLVM bugs hidden that only get triggered if the target is Windows. It's probably also a bit of a challenge to get sysv64 abi speaking binaries on windows in the first place, outside of handrolling assembly or using C to target linux and then using one of the elf parser crates to load the symbols of the binary.

@haraldh you likely have more experience with using sysv64 overall, and probably also with sysv64 on Windows. Would you be interested in contributing an unit test for Rust that specifically tests sysv64 on Windows?

[petrochenkov]

r? @nagisa
(I know that 128 bit integers are is in sysv64 spec, but have no context regarding what happens with them in reality.)

[haraldh]

It only really checks for extern "C" behaviour, on a limited set of platforms

My thought was that extern "C" is equivalent to extern "sysv64" on the targets that use it, and there are such targets in the CI matrix (linux, etc). But you do make a point that it doesn't test whether it works on windows. There might be LLVM bugs hidden that only get triggered if the target is Windows. It's probably also a bit of a challenge to get sysv64 abi speaking binaries on windows in the first place, outside of handrolling assembly or using C to target linux and then using one of the elf parser crates to load the symbols of the binary.

@haraldh you likely have more experience with using sysv64 overall, and probably also with sysv64 on Windows. Would you be interested in contributing an unit test for Rust that specifically tests sysv64 on Windows?

@est31 : I am currently busy, but here is what I would do:

Construct an extern function and call it from rust and assembler, so both have the same assembler output in opt-level=3:

https://godbolt.org/z/Mra9a7

Maybe double check with gcc on Linux:

#include <stdbool.h>

typedef __uint128_t u128;

extern u128 sum_u128(u128 a, u128 b, u128 c, u128 d);
/*
{
    return a + b + c + d;
}
*/

#define ONE   (((u128) 1 << 64 ) + 1)
#define TWO   (((u128) 2 << 64 ) + 2)
#define THREE (((u128) 3 << 64 ) + 3)
#define FOUR  (((u128) 4 << 64 ) + 4)
#define TEN   (((u128) 10 << 64 ) + 10)

extern bool test() {
    u128 ret;
    ret = sum_u128(ONE, TWO, THREE, FOUR);
    return ret == TEN;
}

$ gcc -o - test.c -O3 -fno-asynchronous-unwind-tables -masm=intel -S
[…]
test:
	sub	rsp, 8
	mov	r8d, 3
	mov	r9d, 3
	mov	edx, 2
	push	4
	mov	ecx, 2
	mov	edi, 1
	mov	esi, 1
	push	4
	call	sum_u128
	xor	rax, 10
	xor	rdx, 10
	or	rax, rdx
	sete	al
	add	rsp, 24
	ret
[…]

and then do a rust unit test like this:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=e3895d80c809da01de49a08fa1cbd33d

[haraldh]

The above test succeeds also on windows for me.

[ollie27]

Is the differing alignment between Rust and C (#54341) not going to be a problem?

[est31]

@ollie27 hmmm yeah that's a good point. Alignment has to match whatever sysv64 does. One doesn't have to solve the alignment issues for all platforms/targets, but at least on sysv64 ABIs there must be a match for this PR to get merged.

[est31]

Alright, going to close this then, as #54341 is not resolved yet.