Rework c_variadic

[beetrees]

tracking issue: #44930
related PR: #144529

On some platforms, the C va_list type is actually a single-element array of a struct (on other platforms it is just a pointer). In C, arrays passed as function arguments expirience array-to-pointer decay, which means that C will pass a pointer to the array in the caller instead of the array itself, and modifications to the array in the callee will be visible to the caller (this does not match Rust by-value semantics). However, for va_list, the C standard explicitly states that it is undefined behaviour to use a va_list after it has been passed by value to a function (in Rust parlance, the va_list is moved, not copied). This matches Rust's pass-by-value semantics, meaning that when the C va_list type is a single-element array of a struct, the ABI will match C as long as the Rust type is always be passed indirectly.

In the old implementation, this ABI was achieved by having two separate types: VaList was the type that needed to be used when passing a VaList as a function parameter, whereas VaListImpl was the actual va_list type that was correct everywhere else. This however is quite confusing, as there are lots of footguns: it is easy to cause bugs by mixing them up (e.g. the C function void foo(va_list va) was equivalent to the Rust fn foo(va: VaList) whereas the C function void bar(va_list* va) was equivalent to the Rust fn foo(va: *mut VaListImpl), not fn foo(va: *mut VaList) as might be expected); also converting from VaListImpl to VaList with as_va_list() had platform specific behaviour: on single-element array of a struct platforms it would return a VaList referencing the original VaListImpl, whereas on other platforms it would return a cioy,

In this PR, there is now just a single VaList type (renamed from VaListImpl) which represents the C va_list type and will just work in all positions. Instead of having a separate type just to make the ABI work, #144529 adds a #[rustc_pass_indirectly_in_non_rustic_abis] attribute, which when applied to a struct will force the struct to be passed indirectly by non-Rustic calling conventions. This PR then implements the VaList rework, making use of the new attribute on all platforms where the C va_list type is a single-element array of a struct.

Cleanup of the VaList API and implementation is also included in this PR: since it was decided it was OK to experiment with Rust requiring that not calling va_end is not undefined behaviour (#141524 (comment)), I've removed the with_copy method as it was redundant to the Clone impl (the Drop impl of VaList is a no-op as va_end is a no-op on all known platforms).

Previous discussion: #141524 and t-compiler > c_variadic API and ABI
Tracking issue: #44930
r? @joshtriplett

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

[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?

[beetrees]

I've rebased, and the two tests still pass locally.

[folkertdev]

For me too, I must have messed something up earlier. Thanks for checking!

[beetrees]

I've reworked this to use a #[rustc_pass_indirectly_in_non_rustic_abis] attribute, which I've implemented independently in a separate commit.

[rustbot]

joshtriplett is not on the review rotation at the moment.
They may take a while to respond.

[bors]

☔ The latest upstream changes (presumably #144673) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

compiletest directives have been modified. Please add or update docs for the
new or modified directive in src/doc/rustc-dev-guide/.

[bors]

☔ The latest upstream changes (presumably #148356) made this pull request unmergeable. Please resolve the merge conflicts.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[folkertdev]

How do we feel about this wildcard match? Is it feasible to instead list the targets for which we actually know that they use an opaque pointer, and fail to build for any other targets?

Being exhaustive may create some churn, but at least we'll then never miscompile for a target that we did not consider.