[paradoxicalguy]

View all comments

fixes #165299

this moves the byte count calculation into a shared helper in rustc_codegen_ssa. it now derives the count from the original argument layouts rather than the lowered signature.

the fix is wired up for llvm. symbols are emitted with a \x01 prefix so llvm doesn’t decorate them a second time, and the linker-facing symbol naming path now handles the \x01 and ? prefixes.

testing:

verified fastcall, stdcall and vectorcall on msvc, including cross-crate references.
added a run-make regression test for the linker failure in #165299. unpatched nightly fails with LNK2001; patched compiler links successfully.
added codegen regression tests and updated the existing abi, unwind and assembly tests for the corrected symbol names and byte counts. this also resolves the over-aligned argument FIXME in win32-byte-count-suffix.rs.

cc @folkertdev @workingjubilee

[rustbot]

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: codegen, compiler
• codegen, compiler expanded to 77 candidates
• Random selection from 21 candidates

[paradoxicalguy]

current CI failure is pointing to this existing FIXME

rust/tests/assembly-llvm/win32-byte-count-suffix.rs

Lines 234 to 255 in 2d20b42

	#[target_feature(enable = "sse2")]
	extern "vectorcall" fn four_vectors_vectorcall(_: i32x4, _: i32x4, _: i32x4, _: i32x4) {}
	// CHECK-LABEL: eight_vectors_vectorcall@@128:
	#[unsafe(no_mangle)]
	#[target_feature(enable = "sse2")]
	extern "vectorcall" fn eight_vectors_vectorcall(
	_: i32x4,
	_: i32x4,
	_: i32x4,
	_: i32x4,
	_: i32x4,
	_: i32x4,
	_: i32x4,
	_: i32x4,
	) {
	}
	/// FIXME: the mangled name for this type is incorrect on msvc, where the argument is passed
	/// indirectly and a pointer takes up 4 bytes, but clang counts the size of the type and uses
	/// @8 there. With GNU the value is passed directly, so the suffix is correct.
	#[repr(C, align(8))]

the existing win32-byte-count-suffix.rs test expected @4 for over-aligned arguments on msvc. the patch now emits @8, matching the original argument layout and the existing windows-gnu expectations.
updated that and edited the FIXME.

[paradoxicalguy]

@bors squash msg = "fix MSVC calling convention symbol byte counts"

[rust-bors]

Unknown value for argument "". Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

[paradoxicalguy]

is it because of blank space 🤔
@bors squash msg="fix MSVC calling convention symbol byte counts"

[rust-bors]

🔨 5 commits were squashed into 71146c7.

[folkertdev]

A downside of this rmake test is that it only runs on windows msvc. For my local development, I have this setup as a plain UI test.

This works on x86_64-unknown-linux-gnu because it uses lld as the linker, I think it should also work in the msvc CI jobs, but then in most other cases this test is ignored. So, I'm not sure we should add it, but just wanted to share this.

Some notes

• uses build-pass so it builds and links all the way
• uses add-minicore so it runs anywhere
• uses compile-flags: -Clinker=rust-lld, a linker that I actually have, in combination with //@ needs-rust-lld
• compile-flags: -Clink-arg=/NOENTRY ensures there are no external dependencies, only this file is needed to produce the final link
• #![crate_type = "cdylib"] make sure there is something to link (this creates a final artifact)

//@ build-pass 
//@ add-minicore
//@ revisions: i686 x86_64
//@[i686] compile-flags: --target i686-pc-windows-msvc
//@[x86_64] compile-flags: --target x86_64-pc-windows-msvc
//@ compile-flags: -Copt-level=3 -Clinker=rust-lld -Clink-arg=/NOENTRY
//@ needs-llvm-components: x86
//@ needs-rust-lld
//@ ignore-backends: gcc

#![feature(no_core, abi_vectorcall)]
#![no_core]
#![crate_type = "cdylib"]

extern crate minicore;

// then test whatever

use minicore::simd::i32x4;

#[unsafe(no_mangle)]
pub extern "vectorcall" fn eight_vectors_vectorcall(
    _: i32x4,
    _: i32x4,
    _: i32x4,
    _: i32x4,
    _: i32x4,
    _: i32x4,
    _: i32x4,
    _: i32x4,
) {
}

View changes since the review

[paradoxicalguy]

this is much nicer tbh!
i ran into the missing target libraries while trying to get the i686 linker regression working locally, so minicore + rust-lld looks like a good way around that.

i'll try moving the existing regression into a build-pass test and check that it still fails on unpatched rustc for the expected linker errors

thanks for sharing this =D

[paradoxicalguy]

it passed locally after configuring my local setup into this
pushed the portable rust-lld version for ci; kept the run-make test for now until we confirm it runs there. thanks