Skip to content

Conversation

@beetrees
Copy link
Contributor

@beetrees beetrees commented Jun 3, 2025

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 rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2025

This PR modifies run-make tests.

cc @jieyouxu

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@beetrees beetrees marked this pull request as draft June 3, 2025 18:45
@beetrees beetrees force-pushed the va-list-proposal branch from cb90479 to 23a983d Compare June 3, 2025 19:04
@folkertdev

This comment was marked as outdated.

@beetrees
Copy link
Contributor Author

beetrees commented Jul 3, 2025

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
Copy link
Member

RalfJung commented Jul 3, 2025

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
Copy link
Contributor

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

@beetrees beetrees force-pushed the va-list-proposal branch from 23a983d to 57bc996 Compare July 3, 2025 20:30
@beetrees
Copy link
Contributor Author

beetrees commented Jul 3, 2025

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

@folkertdev
Copy link
Contributor

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

@beetrees beetrees force-pushed the va-list-proposal branch from 57bc996 to 6347a21 Compare July 4, 2025 19:26
@rustbot rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Jul 4, 2025
@beetrees beetrees force-pushed the va-list-proposal branch from 6347a21 to 6e1e74f Compare July 4, 2025 19:34
@beetrees
Copy link
Contributor Author

beetrees commented Jul 4, 2025

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

@beetrees beetrees changed the title Prototype VaList proposal Rework c_variadic Jul 4, 2025
@rust-log-analyzer

This comment has been minimized.

@beetrees beetrees force-pushed the va-list-proposal branch from 6e1e74f to 26f7025 Compare July 4, 2025 19:52
@beetrees beetrees closed this Jul 4, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2025
@beetrees beetrees reopened this Jul 4, 2025
@beetrees beetrees force-pushed the va-list-proposal branch from 26f7025 to 79a6396 Compare July 4, 2025 21:25
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2025

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

rust-bors bot added a commit that referenced this pull request Dec 4, 2025
Rework `c_variadic`

try-job: x86_64-gnu-llvm-21-3
try-job: armhf-gnu
try-job: aarch64-apple
try-job: x86_64-msvc-1
@rust-bors
Copy link

rust-bors bot commented Dec 4, 2025

☀️ Try build successful (CI)
Build commit: eb844f1 (eb844f1dc1429bc1f191384f78d6e11167015883, parent: 556beb9ec72360512d0294eb0855c92fb2c20c88)

@folkertdev
Copy link
Contributor

Hopefully that is sufficient then

@bors r=workingjubilee

@bors
Copy link
Collaborator

bors commented Dec 4, 2025

📌 Commit 0897974 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 4, 2025
@folkertdev folkertdev changed the title Rework c_variadic c_variadic: make VaList abi-compatible with C Dec 4, 2025
@bors
Copy link
Collaborator

bors commented Dec 5, 2025

⌛ Testing commit 0897974 with merge 2243444...

bors added a commit that referenced this pull request Dec 5, 2025
`c_variadic`: make `VaList` abi-compatible with C

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](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/c_variadic.20API.20and.20ABI)
Tracking issue: #44930
r? `@joshtriplett`
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Dec 5, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 5, 2025
@folkertdev
Copy link
Contributor

@bors try jobs=i686-msvc-1

@rust-bors

This comment has been minimized.

rust-bors bot added a commit that referenced this pull request Dec 5, 2025
`c_variadic`: make `VaList` abi-compatible with C

try-job: i686-msvc-1
@rust-log-analyzer

This comment has been minimized.

the 32-bit variant differs only in the alignment/size, and I can't test it locally
@folkertdev
Copy link
Contributor

@bors try cancel

@rust-bors
Copy link

rust-bors bot commented Dec 5, 2025

Try build cancelled. Cancelled workflows:

@folkertdev
Copy link
Contributor

@bors try jobs=x86_64-gnu-llvm-21-3,armhf-gnu,aarch64-apple,x86_64-msvc-1,i686-msvc-1

rust-bors bot added a commit that referenced this pull request Dec 5, 2025
`c_variadic`: make `VaList` abi-compatible with C

try-job: x86_64-gnu-llvm-21-3
try-job: armhf-gnu
try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: i686-msvc-1
@rust-bors

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Dec 5, 2025

☀️ Try build successful (CI)
Build commit: 4465eb8 (4465eb82acb0211cbc7aa35c1672f2f37fba5f2f, parent: 864339abf952f07098dd82610256338520167d4a)

@folkertdev
Copy link
Contributor

folkertdev commented Dec 5, 2025

@bors r=workingjubilee

a layout test failed on 32-bit windows because usize is smaller there. Not very interesting otherwise (all windows targets use an opaque pointer to pass VaList so I just ignore 32-bit windows in that test now.

Hopefully I've try-jobbed everything now?

@bors
Copy link
Collaborator

bors commented Dec 5, 2025

📌 Commit d6db951 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) A-compiletest Area: The compiletest test runner A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc F-c_variadic `#![feature(c_variadic)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.