Aarch64 call abi does not zeroext (and one cannot assume it does so)

[pnkfelix]

Fix #97463

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[pnkfelix]

beta-nominating since this is a P-critical issue affecting a tier-1 target.

I'm also stable-nominating because I think we should at least have the discussion about whether we would want this included if there were a point release. (But I don't personally think the issue warrants making a point release.)

@rustbot label: beta-nominated stable-nominated

[Mark-Simulacrum]

I'm probably not a good reviewer for this, so going to r? rust-lang/compiler

[nagisa]

We have a test setup for ABI tests with auxiliary/rust_test_helpers.c and ui/abi. Can that mechanism be used instead?

[pnkfelix]

let me go inspect that.

(whatever it is, it will need to use -O to replicate the problems here...)

[pnkfelix]

auxiliary/rust_test_helpers.c doesn't currently include <stdlib.h>, which is "needed" here if we want to reproduce the original test faithfully, in that it calls malloc. (And given how sensitive reproducing the failure is to small changes, I'm inclined to assume that malloc is a necessity.)

@nagisa do you know offhand if we are deliberately trying to avoid introducing a dependence from rust_test_helpers.c upon stdlib.h and/or malloc itself, perhaps to ensure we can compile the file on esoteric targets that do not have that available? Just curious.

I'll keep moving forward with trying to move this test into auxiliary/rust_test_helpers.c, under the assumption that we can add such a dependence. But given the amount of work I put into getting the codegen tests working, I'm sort of inclined to just live with a run-make test here, if I encounter any hurdles on the way...

[apiraino]

beta backport discussed in T-compiler and postponed to next week (Zulip discussion)

stable backport declined

@rustbot label -stable-nominated

[nbdd0121]

I think the test should really be a couple of codegen test (instead of testing using run-make-fulldeps) to ensure that we emit the same function signature as Clang does for a extern "C" function.

[nagisa]

I disagree – the test should be a codegen test in addition to a test that actually verifies that the assumptions hold in practice. There's a good reason why we have a bunch of ABI tests that exercise behaviour at runtime.

[pnkfelix]

Yes, I agree with @nagisa : the run-make-fulldeps test is very fragile (in the sense that changes to the optimizations can cause the test to stop actually exercising that the proper change is in place), but it is a proper integration test.

I have now confirmed on my Apple M1 that the existing codegen tests even catch the mistake injected by this PR. Namely, I just tried to do a local x.py test src/test/codegen on my Apple M1 (with these patches cherry-picked), and got:

failures:
    [codegen] src/test/codegen/abi-repr-ext.rs
    [codegen] src/test/codegen/pic-relocation-model.rs

However, I think it will improve things a bit if I add a set of codegen tests specifically checking how i8/i16/u8/u16 are passed and returned. (I'm not quite sure yet how best to encode the variation between different targets on this front; it seems like the current strategy is to put variations into distinct files, each with the right set of // ignore-* directives.)

[pnkfelix]

(I'm not quite sure yet how best to encode the variation between different targets on this front; it seems like the current strategy is to put variations into distinct files, each with the right set of // ignore-* directives.)

Ah, thank goodness: that is not the only strategy available. I just discovered that in addition to the usual // CHECK: prefix, we also have target-specific checks, like so:

rust/src/test/codegen/abi-efiapi.rs

Lines 27 to 33 in ca98305

	//x86_64: define win64cc void @has_efiapi
	//i686: define void @has_efiapi
	//aarch64: define dso_local void @has_efiapi
	//arm: define dso_local void @has_efiapi
	//riscv: define dso_local void @has_efiapi
	#[no_mangle]
	pub extern "efiapi" fn has_efiapi() {}

(... though... can that encode the aarch64-apple-darwin vs aarch-unknown-linux-* distinction that we need here... I guess we will find out...)

[apiraino]

Beta backport revisited and declined as per compiler team on Zulip

@rustbot label -beta-nominated

[nbdd0121]

I suppose for the test it's easier to trigger with a C and Rust roundtripping:

extern "C" {
    fn return_u16() -> u16;
}

#[no_mangle]
extern "C" fn return_u32() -> u32 {
	0xdeadbeef
}

#[inline(never)]
pub fn return_u32_cvt() -> u32 {
    unsafe { return_u16() as u32 }
}

pub fn main() {
    assert_eq!(return_u32_cvt(), 0xbeef);
}

and

unsigned int return_u32();

unsigned short return_u16() {
    return return_u32();
}

[bors]

📌 Commit ed9b12d has been approved by wesleywiser

It is now in the queue for this repository.

[wesleywiser]

@bors p=1

Fixes P-critical issue.

[bors]

⌛ Testing commit ed9b12d with merge 7fa8fd520fe631b6afec160a397ca98c9cc4dd2a...

[bors]

💔 Test failed - checks-actions

[pnkfelix]

ha ha ha ha ha this pull request is never going to land ha ha ha ha ha. 😵‍💫

[pnkfelix]

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[pnkfelix]

@bors r=wesleywiser

[bors]

📌 Commit a2de75a has been approved by wesleywiser

It is now in the queue for this repository.

[bors]

⌛ Testing commit a2de75a with merge 95a992a...

[bors]

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 95a992a to master...

[rust-timer]

Finished benchmarking commit (95a992a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	2.1%	[0.9%, 3.3%]	2
Regressions ❌ (secondary)	2.3%	[2.3%, 2.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-2.9%, -2.9%]	1
All ❌✅ (primary)	2.1%	[0.9%, 3.3%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²