improve core::ffi::VaList
#141835
Closed
improve core::ffi::VaList
#141835
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
Collaborator
|
Some changes occurred in compiler/rustc_codegen_ssa |
This comment has been minimized.
This comment has been minimized.
Contributor
Member
|
Correct, we decided to follow C after that + a lang decision a while back. |
beetrees
reviewed
Jun 1, 2025
library/core/src/ffi/va_list.rs
Outdated
Comment on lines
147
to
166
| pub fn copy( | ||
| &self, | ||
| tag: &mut MaybeUninit<[*const (); size_of::<VaListTag<'_>>() / size_of::<*const ()>()]>, | ||
| ) -> Self { | ||
| const { | ||
| type A = [*const (); size_of::<VaListTag<'_>>() / size_of::<*const ()>()]; | ||
| assert!(size_of::<A>() == size_of::<VaListTag<'_>>()); | ||
| assert!(align_of::<A>() == align_of::<VaListTag<'_>>()); | ||
| } | ||
|
|
||
| // SAFETY: we verified that the type has the right size and alignment. | ||
| let tag = unsafe { &mut *(tag as *mut MaybeUninit<_> as *mut MaybeUninit<VaListTag<'a>>) }; | ||
|
|
||
| // SAFETY: `self` is a valid `va_list`, `tag` has sufficient space to store a `va_list`. | ||
| unsafe { va_copy_intrinsic::va_copy(tag.as_mut_ptr(), &self.inner) }; | ||
|
|
||
| // SAFETY: `va_copy` initializes the tag. | ||
| VaList { inner: unsafe { tag.assume_init_mut() } } | ||
| } |
Contributor
There was a problem hiding this comment.
The returned VaList has the lifetime 'a, which isn't connected to the lifetime of the &mut reference to the tag. This means that the returned VaList<'a> can outlive the tag, causing undefined behaviour. (This doesn't cause a compiler error because the unsafe { &mut *(tag as *mut MaybeUninit<_> as *mut MaybeUninit<VaListTag<'a>>) } pointer cast discards the lifetime.)
The lifetimes need to look something like pub fn copy<'b>(&self, tag: &'b mut MaybeUninit<VaListTag<'a>>) -> VaList<'b> where 'a: 'b.
With regards to the type of the tag parameter, I think it should be ok to just use &mut MaybeUninit<VaListTag<'a>> and just #[expect(private_interfaces)] the lint.
Contributor
Author
There was a problem hiding this comment.
Allright, I've reworked that interface a bunch, the 'a: 'b idea worked out nicely, and makes a lot more sense in hindsight.
For the time being the va_copy intrinsic still equates the lifetimes of the input and output. Once we have the new stage0 (and hence no more #[cfg(bootstrap)]) that can maybe change, but for now that was too tricky (for me anyway).
folkertdev
force-pushed
the
exp-new-va-list
branch
from
52adac0 to
63865df
Compare
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
exp-new-va-list
branch
from
63865df to
d6baf43
Compare
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
exp-new-va-list
branch
from
d6baf43 to
489e6dc
Compare
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
exp-new-va-list
branch
from
489e6dc to
5785a35
Compare
This comment has been minimized.
This comment has been minimized.
Collaborator
|
☔ The latest upstream changes (presumably #142099) made this pull request unmergeable. Please resolve the merge conflicts. |
folkertdev
force-pushed
the
exp-new-va-list
branch
from
5785a35 to
dc64c96
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
Author
|
closing this in favor of #141980 |
rustbot
removed
the
S-waiting-on-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tracking issue: #44930
This PR refactors the
VaListAPI. The highlights areVaListImplis no longer user-visible (and internally renamed toVaListTag)VaListnow has only a single lifetime parameterva_copy!macro is added to duplicate aVaListThis PR mostly touches the
coreimplementation ofVaList, but needs some changes in the rest of the compiler to make it all work.This change was discussed in #141524.
This next section is a kind of pseudo-rfc: it describes what the
c_variadicfeature is, and what API surface we have. The lang team likely wants to see a new RFC or extensive stabilization report before stabilizingc_variadic.Syntax
A c-variadic function looks like this:
The special
...argument stands in for an arbitrary number of arguments that the caller may pass. The compiler has no way of verifying that the arguments that are passed to a c-variadic function are correct, and therefore calling a c-variadic function is alwaysunsafe.The
...argument must be the last argument in the parameter list of a function. Contrary to earlier versions of C, but in line with C23, in rust the...argument can be the first (and therefore only) argument to a function. The...syntax is already stable in foreign functions,c_variadicadditionally allows it in function definitions.A function with a
...argument must be anunsafefunction. Passing an incorrect number of arguments, or arguments of the wrong type is UB, and hence every call site should have a safety comment.This document only considers functions with the
"C"and"C-unwind" calling conventions. It is possible to in the future also support e.g."sysv64"or"win64", but for now we consider that to be out-of-scope.VaListThe type of
...iscore::ffi::VaList: the list of variadic arguments. The rustVaListtype on a given platform is equivalent to the Cva_listtype on that platform: values of this type can be passed to C via FFI, and used like any otherva_listvalue.Across ABIs, there are two ways that
VaListis implemented:VaListis just an opaque pointer. (in practice it is usually a pointer to the caller's stack where the variadic arguments are stored)VaListis a mutable reference to a structure (calledVaListTag) on the callee's stack.In both cases, there are lifetime constraints on the
VaList: it must not escape the function in which it was defined!desugaring
...A function
Semantically desugars in one of two ways, depending on the ABI of the target platform. The actual desugaring is built into the compiler, these examples just illustrate what happens.
the "pointer" approach
the "struct on stack" approach
The builtin desugaring process guarantees the correct lifetime is selected for
VaList, and that the call tova_endis inserted on all return paths.va_argIn C, the
va_argmacro is used to read the next variadic argument. Rust exposes this functionality asThe
argmethod is unsafe because attempting to read more arguments than were passed or an argument of the wrong type is undefined behavior.The type parameter
Tis constrained by theVaArgSafetrait: small numeric types are subject to implicit upcasting in C. Attempting to read a value of such a type (e.g.bool,i8,u16,f32) is undefined behavior. TheVaArgSafetrait is only implemented for numeric types for which no implicit upcasting occurs, and for const/mut pointer types.The
VaArgSafetrait is public, but cannot currently be implemented outside ofcore.A note on LLVM
va_argThe llvm
va_argintrinsic is known to silently miscompile. I believe this is due to a combination of:va_arg, so the LLVM implementation is mostly untestedHence, like clang,
rustcimplementsva_argfor most commonly-used targets (specifically including all tier-1 targets) inva_arg.rs. If no custom implementation is provided, the LLVM implementation is used as a fallback. But again, it may silently miscompile the input program.the
va_copy!macroThe
VaListtype has awith_copymethod, that provides access to a copy of theVaList. However,c2rustran into this method not being flexible enough to translate C programs seen in the wild (immunant/c2rust#43). Theva_copy!macro mirrors the Cva_copymacro, enabling a straightforward translation from C to rust.The concrete pattern that this macro enables is to (easily) iterate over two copies of the
VaListin parallel.The implementation of this macro uses
super letto give the copiedVaListthe correct lifetime. Currently it relies on some#[doc(hidden)]methods onVaList, so thatVaListTagdoes not need to be exposed.C-variadics and
const fnC-variadics cannot be
const fnat the time of writing. This is a limitation in MIRI that may be lifted at some point.Other calling conventions
For now we only consider the
"C"and"C-unwind"calling conventions. Other calling conventions are disallowed for now, but we have considered how support may be added in the future.The difficulty is that currently the target determines the layout of
VaListandVaListTag. Accepting multiple c-variadic functions with ABIs in the same program means that the layout of those types may be different depending on the exact ABI that is used in a function. We also must be clear about when a rustVaListis compatible with the Cva_listtype.Speaking of C,
clangandgccwon't compile a function that uses variadic arguments and a non-standard calling convention. See also #141618, in particular this commentNevertheless, two potential solutions for supporting multiple c-variadic ABIs in the same program have been proposed.
A generic parameter with a default
The
VaListstructure can be extended like soThe
...parameter in a c-variadicextern "C" fnwill have typeVaList<'a, C>, which is guaranteed to be compatible with Cva_listtype for the target platform. Functions using a non-standard abi get a type likeVaList<'a, Win64>.Multiple types
Alternatively, every abi could get its own
VaListtype, and...in anextern "C" fndesugars to the type that is compatible with the Cva_listtype for the target platform.A type alias could be used to always be able to refer to the C-compatible
VaListtype.Review notes
va_copy!macro. Maybe there are some tricks incoreto make that nicer? (e.g. makeVaListTagpublic, but#[doc(hidden)]and perma-unstable somehow, but then allow that unstable feature in theva_copymacro expansion?)r? @joshtriplett
cc @workingjubilee
@rustbot label: +F-c_variadic