Prototype VaList proposal

[beetrees]

#141524 (comment)

r? @ghost

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[workingjubilee]

I only see this as having an effect on x86-64 and aarch64, but doesn't the argument-passing algorithm already enforce this by making sure that VaList gets passed via Memory?

[beetrees]

This method is only needed by the code in rustc_target::callconv to ensure that VaList gets PassMode::Indirect { on_stack: false, .. }. repr_options_of_def sets the PASS_INDIRECTLY ReprFlag for VaList when it needs to be passed indirectly for non-rustic ABIs, however there is no way to directly access ReprFlags from the calling convention code. There is already a method is_transparent on TyAbiInterface to expose the IS_TRANSPARENT ReprFlag, so I used a similar pattern to expose PASS_INDIRECTLY to the calling convention code. Without this method, the System V x86-64 ABI would pass VaList with PassMode::Indirect { on_stack: true, .. }.

[folkertdev]

And just to finish the thought: the difference is important (only) in an FFI context: a foreign extern "C" { fn(ap: VaList); } would expect ap to be passed as PassMode::Indirect { on_stack: false, .. }

[workingjubilee]

huh, fascinating.

[RalfJung]

So randomly in the middle of repr_options_of_def we decide what the ABI for VaList will be? That's a disaster waiting to happen IMO, nobody will suspect ABI-relevant code here...

Also, in the discussion the argument was that this is necessary when va_list is an array -- but I can't see that reflected here at all. Given that the actual source of the issue is array decay, shouldn't we just fix the extern "C" logic so that it passes all arrays by-ptr (there different ways to do that, none of them pretty until #119183 are resolved), and then that'll do the right thing for array-based va_list by itself?

[beetrees]

This is where the (very-ABI-affecting) IS_TRANSPARENT repr flag is set (based on the function attributes), so it seemed like the best place to put it. Having it as a repr flag seemed like the best way to communicate it to the code in rustc_target (the only place that needs to be aware of it), but it would be easy enough to do it another way if this way is sub-optimal. You're correct about this not actually checking whether it is an array type: this was the easiest way to differentiate between the two types of array list when I was prototyping this, but it should be replaced with a more precise check. Passing all arrays by reference seems to have been discussed on Zulip and in the parent issue; I think it would be a footgun as C allows the user to observe modifications the callee makes to the array in the caller (unlike va_list, where the C standard says that using the va_list after passing it to a function is UB).

I think @folkertdev has suggested before something along the lines of a #[rustc_always_pass_indirectly] attribute which could be applied to VaList type as needed: would that be preferable?

[RalfJung]

Yeah I think if we do go for this (which I am not convinced we should), then it should be its own attribute, not magically connected to the lang item.

[folkertdev]

@beetrees when I run this PR locally, on x86_64, ./x test --keep-stage 1 tests/run-make/c-link-to-rust-va-list-fn fails. I suspect CI passes because it runs on AARCH64 now.

(btw I landed a PR a while ago that makes it easy to cross-compile that test if you have the relevant linker installed and runner defined in bootstrap.toml, e.g.

[target.powerpc-unknown-linux-gnu]
runner = "/home/folkertdev/c/qemu/build/qemu-ppc -L /usr/powerpc-linux-gnu"

[target.s390x-unknown-linux-gnu]
runner = "qemu-s390x -L /usr/s390x-linux-gnu"

---

I also tried this tests/assembly test:

//@ assembly-output: emit-asm
//@ compile-flags: -O
#![feature(c_variadic)]
#![crate_type = "lib"]

use std::ffi::VaList;

// CHECK-LABEL: by_value:
#[unsafe(no_mangle)]
extern "C" fn by_value(mut x: VaList) -> u32 {
    unsafe { x.arg::<u32>() }
}

// CHECK-LABEL: by_reference:
#[unsafe(no_mangle)]
extern "C" fn by_reference(x: &mut VaList) -> u32 {
    unsafe { x.arg::<u32>() }
}

// CHECK-LABEL: dummy:
// CHECK: xlkj
#[unsafe(no_mangle)]
extern "C" fn dummy() {}

When I diff by_value and by_reference, they are very different while I suspected them to be the same.

Am I missing something, or is this broken? Over in #t-compiler > array-to-pointer decay npopov also mentions that on_stack: false is still distinct from array-to-pointer decay (though perhaps it's not meaningful for c_variadic.

[beetrees]

when I run this PR locally, on x86_64, ./x test --keep-stage 1 tests/run-make/c-link-to-rust-va-list-fn fails.

I've just ran ./x test tests/run-make/c-link-to-rust-va-list-fn locally and it passed. Maybe --keep-stage 1 is messing things up? If someone runs an @bors try we could check if it passes in CI.

When I diff by_value and by_reference, they are very different while I suspected them to be the same.

I ran the assembly test you supplied locally and got this assembly:

Test assembly

	.file	"temp.9a048525d681a25f-cgu.0"
	.section	.text.by_reference,"ax",@progbits
	.globl	by_reference
	.p2align	4
	.type	by_reference,@function
by_reference:
	.cfi_startproc
	movl	(%rdi), %ecx
	cmpq	$40, %rcx
	ja	.LBB0_2
	movq	%rcx, %rax
	addq	16(%rdi), %rax
	addl	$8, %ecx
	movl	%ecx, (%rdi)
	movl	(%rax), %eax
	retq
.LBB0_2:
	movq	8(%rdi), %rax
	leaq	8(%rax), %rcx
	movq	%rcx, 8(%rdi)
	movl	(%rax), %eax
	retq
.Lfunc_end0:
	.size	by_reference, .Lfunc_end0-by_reference
	.cfi_endproc

	.section	.text.dummy,"ax",@progbits
	.globl	dummy
	.p2align	4
	.type	dummy,@function
dummy:
	.cfi_startproc
	retq
.Lfunc_end1:
	.size	dummy, .Lfunc_end1-dummy
	.cfi_endproc

	.globl	by_value
	.type	by_value,@function
.set by_value, by_reference
	.ident	"rustc version 1.89.0-dev"
	.section	".note.GNU-stack","",@progbits

LLVM appears to have merged the functions as they had identical assembly.

Over in #t-compiler > array-to-pointer decay npopov also mentions that on_stack: false is still distinct from array-to-pointer decay (though perhaps it's not meaningful for c_variadic.

on_stack: false does differ from array-to-pointer decay, but none of the differences matter for this particular use case as VaList is not Copy and C doesn't allow using a va_list after passing it as a function argument.

[RalfJung]

on_stack: false does differ from array-to-pointer decay, but none of the differences matter for this particular use case as VaList is not Copy and C doesn't allow using a va_list after passing it as a function argument.

I am somewhat uncomfortable relying on that, and would personally prefer a slightly less slick API that requires fewer hacks. This feature is too niche to justify so much hackery, IMO.

But maybe I am overly paranoid. If we do end up going for it, please add walls of comments everywhere that explain this, or else we can be sure some poor compiler contributor will spend hours digging through this in a few years...

[folkertdev]

@beetrees hmm, I did rebase, maybe that messed something up? Can you rebase this branch to something more recent?