implement va_arg for x86_64 systemv

[folkertdev]

tracking issue: #44930

Turns out LLVM's va_arg is also unreliable for this target.

llvm/llvm-project#141361

So, like clang, we implement our own. I used

• the spec at https://gitlab.com/x86-psABIs/x86-64-ABI
• the clang implementation at https://github.com/llvm/llvm-project/blob/9a440f84773c56d3803f330774acb2b4f471d5b4/clang/lib/CodeGen/Targets/X86.cpp#L3041

We can take a bunch of shortcuts because the return type of va_list must implement VaArgSafe. I also extended some of the tests, because up to 11 floats can be stored in the reg_safe_area for this calling convention.

r? @workingjubilee
@rustbot label +F-c_variadic

try-job: x86_64-apple-1

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

[workingjubilee]

some nits and general questions

[folkertdev]

This now supports structs like

struct B(i32, i32);
struct C(i64, i64, i64);
struct D(f64, i64);
struct E(i64, f64);
struct F(f64, f64);

It however does not support

#[repr(C)]
struct Empty;

impl Copy for Empty {}

#[repr(C)]
struct Cursed([Empty; 8], i32);

Because rust passes this as BackendRepr::Memory, while in C it is treated as a scalar (based on the generated assembly). I don't know why rust passes it that way, or whether that is in fact correct.

[folkertdev]

@rustbot ready

[folkertdev]

the cause is this optimize_abi value:

rust/compiler/rustc_abi/src/layout.rs

Lines 1373 to 1396 in 95a2212

	let optimize_abi = !repr.inhibit_newtype_abi_optimization();
	// Try to make this a Scalar/ScalarPair.
	if sized && size.bytes() > 0 {
	// We skip *all* ZST here and later check if we are good in terms of alignment.
	// This lets us handle some cases involving aligned ZST.
	let mut non_zst_fields = fields.iter_enumerated().filter(|&(_, f)| !f.is_zst());
	match (non_zst_fields.next(), non_zst_fields.next(), non_zst_fields.next()) {
	// We have exactly one non-ZST field.
	(Some((i, field)), None, None) => {
	layout_of_single_non_zst_field = Some(field);
	// Field fills the struct and it has a scalar or scalar pair ABI.
	if offsets[i].bytes() == 0 && align.abi == field.align.abi && size == field.size
	{
	match field.backend_repr {
	// For plain scalars, or vectors of them, we can't unpack
	// newtypes for `#[repr(C)]`, as that affects C ABIs.
	BackendRepr::Scalar(_) | BackendRepr::SimdVector { .. }
	if optimize_abi =>
	{
	abi = field.backend_repr;
	}

when I disable that if optimize_abi => that struct does work. Anyway, I think the va_arg code is correct, it may just need some adjusted repr as input.

[workingjubilee]

when I disable that if optimize_abi => that struct does work. Anyway, I think the va_arg code is correct, it may just need some adjusted repr as input.

It's fine if it's buggy for non-scalars, it just felt slightly silly to write it in the way that can support at least some structs and then not support any structs.

[folkertdev]

I added this bit of logic and now it actually does work correctly for that struct

    // Peel off any newtype wrappers.
    let layout = {
        let mut layout = bx.cx.layout_of(target_ty);

        while let Some((_, inner)) = layout.non_1zst_field(bx.cx) {
            layout = inner;
        }

        layout
    };

[kouhe3]

This now supports structs like

struct B(i32, i32);
struct C(i64, i64, i64);
struct D(f64, i64);
struct E(i64, f64);
struct F(f64, f64);

Does it support tulple that mix 8bit 32bit 64bit or different length type

[folkertdev]

Currently, arguments to va_arg must implement VaArgSafe. Only 32-bit and 64-bit integers implement this trait.

So, the testing with structs is just to verify the implementation, because the standard library currently does not allow va_arg on 8-bit integers, or any sort of struct/tuple/custom type.

The exact size of the fields does not matter, because they all get 8 bytes of space:

    // l->gp_offset = l->gp_offset + num_gp * 8
    if num_gp_registers > 0 {
        let offset = bx.const_u32(num_gp_registers * 8);
        let sum = bx.add(gp_offset_v, offset);
        bx.store(sum, gp_offset_ptr, Align::from_bytes(8).unwrap());
    }

[workingjubilee]

Yeah, I'm not blocking this on being correct for all structs, I just kinda wanted to see us bother to land something that could be bugfixed, essentially.

[folkertdev]

rebased and turned some inbounds_gep into inbounds_ptradd. I think this is good to go?

[tgross35]

@bors try
@bors delegate=folkertdev for retries

Maybe varargs should also get asm tests since it involves ABI intricacies?

[bors]

✌️ @folkertdev, you can now approve this pull request!

If @tgross35 told you to "r=me" after making some further change, please make that change, then do @bors r=@tgross35

[bors]

⌛ Trying commit fd76915 with merge 61d3c5d...

[bors]

☀️ Try build successful - checks-actions
Build commit: 61d3c5d (61d3c5d4666a9f1e8c58f4b08798221ebd759bcf)

[workingjubilee]

Seems like we're good.

[folkertdev]

Yes, I think so.

[workingjubilee]

while scrutinizing this more closely before the final approval I came up with some questions that may need me to re-reference the layout code to answer...

[workingjubilee]

r=me after review addressed

[workingjubilee]

hm. should already be aligned to 16 bytes so we should handle f128 fine when that becomes relevant? right?

[folkertdev]

Exactly. Similarly SIMD types are stored in the same area, and always with an alignment of 16.

[workingjubilee]

seems fine

[workingjubilee]

cool

@bors r+

[bors]

📌 Commit 94cc726 has been approved by workingjubilee

It is now in the queue for this repository.