Rework c_variadic

compiler/rustc_codegen_ssa/src/traits/intrinsic.rs

@@ -36,10 +36,10 @@ pub trait IntrinsicCallBuilderMethods<'tcx>: BackendTypes {
         vtable_byte_offset: u64,
         typeid: Self::Metadata,
     ) -> Self::Value;
-    /// Trait method used to inject `va_start` on the "spoofed" `VaListImpl` in
+    /// Trait method used to inject `va_start` on the "spoofed" `VaList` in
     /// Rust defined C-variadic functions.
     fn va_start(&mut self, val: Self::Value) -> Self::Value;
-    /// Trait method used to inject `va_end` on the "spoofed" `VaListImpl` before
+    /// Trait method used to inject `va_end` on the "spoofed" `VaList` before
     /// Rust defined C-variadic functions return.
     fn va_end(&mut self, val: Self::Value) -> Self::Value;
 }

library/core/src/ffi/mod.rs

@@ -28,7 +28,7 @@ pub mod c_str;
     issue = "44930",
     reason = "the `c_variadic` feature has not been properly tested on all supported platforms"
 )]
-pub use self::va_list::{VaArgSafe, VaList, VaListImpl};
+pub use self::va_list::{VaArgSafe, VaList};
 
 #[unstable(
     feature = "c_variadic",

library/core/src/ffi/va_list.rs

@@ -4,185 +4,158 @@
 
 #[cfg(not(target_arch = "xtensa"))]
 use crate::ffi::c_void;
-#[allow(unused_imports)]
 use crate::fmt;
-use crate::intrinsics::{va_arg, va_copy, va_end};
-use crate::marker::{PhantomData, PhantomInvariantLifetime};
-use crate::ops::{Deref, DerefMut};
+use crate::intrinsics::{va_arg, va_copy};
+use crate::marker::PhantomCovariantLifetime;
 
-// The name is WIP, using `VaListImpl` for now.
+// There are currently three flavors of how a C `va_list` is implemented for
+// targets that Rust supports:
 //
-// Most targets explicitly specify the layout of `va_list`, this layout is matched here.
+// - `va_list` is an opaque pointer
+// - `va_list` is a struct
+// - `va_list` is a single-element array, containing a struct
+//
+// The opaque pointer approach is the simplest to implement: the pointer just
+// points to an array of arguments on the caller's stack.
+//
+// The struct and single-element array variants are more complex, but
+// potentially more efficient because the additional state makes it
+// possible to pass variadic arguments via registers.
+//
+// The Rust `VaList` type is ABI-compatible with the C `va_list`.
+// The struct and pointer cases straightforwardly map to their rust equivalents,
+// but the single-element array case is special: in C, this type is subject to
+// array-to-pointer decay.
+//
+// The `#[rustc_pass_indirectly_in_non_rustic_abis]` attribute is used to match
+// the pointer decay behavior in Rust. This attribute ensures that the compiler
+// uses the correct ABI for functions like
+// `extern "C" fn takes_va_list(va: VaList<'_>)` by passing `va` indirectly.
 crate::cfg_select! {
     all(
         target_arch = "aarch64",
         not(target_vendor = "apple"),
         not(target_os = "uefi"),
         not(windows),
     ) => {
-        /// AArch64 ABI implementation of a `va_list`. See the
-        /// [AArch64 Procedure Call Standard] for more details.
+        /// AArch64 ABI implementation of a `va_list`.
+        ///
+        /// See the [AArch64 Procedure Call Standard] for more details.
         ///
         /// [AArch64 Procedure Call Standard]:
         /// http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
         #[repr(C)]
         #[derive(Debug)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
-            stack: *mut c_void,
-            gr_top: *mut c_void,
-            vr_top: *mut c_void,
+        struct VaListInner {
+            stack: *const c_void,
+            gr_top: *const c_void,
+            vr_top: *const c_void,
             gr_offs: i32,
             vr_offs: i32,
-            _marker: PhantomInvariantLifetime<'f>,
         }
     }
     all(target_arch = "powerpc", not(target_os = "uefi"), not(windows)) => {
         /// PowerPC ABI implementation of a `va_list`.
+        ///
+        /// See the [LLVM source] and [GCC header] for more details.
+        ///
+        /// [LLVM source]:
+        /// https://github.com/llvm/llvm-project/blob/af9a4263a1a209953a1d339ef781a954e31268ff/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L4089-L4111
+        /// [GCC header]: https://web.mit.edu/darwin/src/modules/gcc/gcc/ginclude/va-ppc.h
         #[repr(C)]
         #[derive(Debug)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        struct VaListInner {
             gpr: u8,
             fpr: u8,
             reserved: u16,
-            overflow_arg_area: *mut c_void,
-            reg_save_area: *mut c_void,
-            _marker: PhantomInvariantLifetime<'f>,
+            overflow_arg_area: *const c_void,
+            reg_save_area: *const c_void,
         }
     }
     target_arch = "s390x" => {
         /// s390x ABI implementation of a `va_list`.
+        ///
+        /// See the [S/390x ELF Application Binary Interface Supplement] for more details.
+        ///
+        /// [S/390x ELF Application Binary Interface Supplement]:
+        /// https://docs.google.com/gview?embedded=true&url=https://github.com/IBM/s390x-abi/releases/download/v1.7/lzsabi_s390x.pdf
         #[repr(C)]
         #[derive(Debug)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        struct VaListInner {
             gpr: i64,
             fpr: i64,
-            overflow_arg_area: *mut c_void,
-            reg_save_area: *mut c_void,
-            _marker: PhantomInvariantLifetime<'f>,
+            overflow_arg_area: *const c_void,
+            reg_save_area: *const c_void,
         }
     }
     all(target_arch = "x86_64", not(target_os = "uefi"), not(windows)) => {
-        /// x86_64 ABI implementation of a `va_list`.
+        /// x86_64 System V ABI implementation of a `va_list`.
+        ///
+        /// See the [System V AMD64 ABI] for more details.
+        ///
+        /// [System V AMD64 ABI]:
+        /// https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf
         #[repr(C)]
         #[derive(Debug)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        struct VaListInner {
             gp_offset: i32,
             fp_offset: i32,
-            overflow_arg_area: *mut c_void,
-            reg_save_area: *mut c_void,
-            _marker: PhantomInvariantLifetime<'f>,
+            overflow_arg_area: *const c_void,
+            reg_save_area: *const c_void,
         }
     }
     target_arch = "xtensa" => {
         /// Xtensa ABI implementation of a `va_list`.
+        ///
+        /// See the [LLVM source] for more details.
+        ///
+        /// [LLVM source]:
+        /// https://github.com/llvm/llvm-project/blob/af9a4263a1a209953a1d339ef781a954e31268ff/llvm/lib/Target/Xtensa/XtensaISelLowering.cpp#L1211-L1215
         #[repr(C)]
         #[derive(Debug)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
-            stk: *mut i32,
-            reg: *mut i32,
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        struct VaListInner {
+            stk: *const i32,
+            reg: *const i32,
             ndx: i32,
-            _marker: PhantomInvariantLifetime<'f>,
         }
     }
 
     // The fallback implementation, used for:
     //
     // - apple aarch64 (see https://github.com/rust-lang/rust/pull/56599)
     // - windows
+    // - powerpc64 & powerpc64le
     // - uefi
-    // - any other target for which we don't specify the `VaListImpl` above
+    // - any other target for which we don't specify the `VaListInner` above
     //
     // In this implementation the `va_list` type is just an alias for an opaque pointer.
     // That pointer is probably just the next variadic argument on the caller's stack.
     _ => {
         /// Basic implementation of a `va_list`.
         #[repr(transparent)]
-        #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
-            ptr: *mut c_void,
-
-            // Invariant over `'f`, so each `VaListImpl<'f>` object is tied to
-            // the region of the function it's defined in
-            _marker: PhantomInvariantLifetime<'f>,
-        }
-
-        impl<'f> fmt::Debug for VaListImpl<'f> {
-            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(f, "va_list* {:p}", self.ptr)
-            }
-        }
-    }
-}
-
-crate::cfg_select! {
-    all(
-        any(
-            target_arch = "aarch64",
-            target_arch = "powerpc",
-            target_arch = "s390x",
-            target_arch = "x86_64"
-        ),
-        not(target_arch = "xtensa"),
-        any(not(target_arch = "aarch64"), not(target_vendor = "apple")),
-        not(target_family = "wasm"),
-        not(target_os = "uefi"),
-        not(windows),
-    ) => {
-        /// A wrapper for a `va_list`
-        #[repr(transparent)]
-        #[derive(Debug)]
-        pub struct VaList<'a, 'f: 'a> {
-            inner: &'a mut VaListImpl<'f>,
-            _marker: PhantomData<&'a mut VaListImpl<'f>>,
-        }
-
-
-        impl<'f> VaListImpl<'f> {
-            /// Converts a [`VaListImpl`] into a [`VaList`] that is binary-compatible with C's `va_list`.
-            #[inline]
-            pub fn as_va_list<'a>(&'a mut self) -> VaList<'a, 'f> {
-                VaList { inner: self, _marker: PhantomData }
-            }
-        }
-    }
-
-    _ => {
-        /// A wrapper for a `va_list`
-        #[repr(transparent)]
         #[derive(Debug)]
-        pub struct VaList<'a, 'f: 'a> {
-            inner: VaListImpl<'f>,
-            _marker: PhantomData<&'a mut VaListImpl<'f>>,
-        }
-
-        impl<'f> VaListImpl<'f> {
-            /// Converts a [`VaListImpl`] into a [`VaList`] that is binary-compatible with C's `va_list`.
-            #[inline]
-            pub fn as_va_list<'a>(&'a mut self) -> VaList<'a, 'f> {
-                VaList { inner: VaListImpl { ..*self }, _marker: PhantomData }
-            }
+        struct VaListInner {
+            ptr: *const c_void,
         }
     }
 }
 
-impl<'a, 'f: 'a> Deref for VaList<'a, 'f> {
-    type Target = VaListImpl<'f>;
-
-    #[inline]
-    fn deref(&self) -> &VaListImpl<'f> {
-        &self.inner
-    }
+/// A variable argument list, equivalent to `va_list` in C.
+#[repr(transparent)]
+#[lang = "va_list"]
+pub struct VaList<'a> {
+    inner: VaListInner,
+    _marker: PhantomCovariantLifetime<'a>,
 }
 
-impl<'a, 'f: 'a> DerefMut for VaList<'a, 'f> {
-    #[inline]
-    fn deref_mut(&mut self) -> &mut VaListImpl<'f> {
-        &mut self.inner
+impl fmt::Debug for VaList<'_> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // No need to include `_marker` in debug output.
+        f.debug_tuple("VaList").field(&self.inner).finish()
     }
 }
 
@@ -203,7 +176,7 @@ mod sealed {
     impl<T> Sealed for *const T {}
 }
 
-/// Types that are valid to read using [`VaListImpl::arg`].
+/// Types that are valid to read using [`VaList::arg`].
 ///
 /// # Safety
 ///
@@ -238,7 +211,7 @@ unsafe impl VaArgSafe for f64 {}
 unsafe impl<T> VaArgSafe for *mut T {}
 unsafe impl<T> VaArgSafe for *const T {}
 
-impl<'f> VaListImpl<'f> {
+impl<'f> VaList<'f> {
     /// Advance to and read the next variable argument.
     ///
     /// # Safety
@@ -257,46 +230,25 @@ impl<'f> VaListImpl<'f> {
         // SAFETY: the caller must uphold the safety contract for `va_arg`.
         unsafe { va_arg(self) }
     }
-
-    /// Copies the `va_list` at the current location.
-    pub unsafe fn with_copy<F, R>(&self, f: F) -> R
-    where
-        F: for<'copy> FnOnce(VaList<'copy, 'f>) -> R,
-    {
-        let mut ap = self.clone();
-        let ret = f(ap.as_va_list());
-        // SAFETY: the caller must uphold the safety contract for `va_end`.
-        unsafe {
-            va_end(&mut ap);
-        }
-        ret
-    }
 }
 
-impl<'f> Clone for VaListImpl<'f> {
+impl<'f> Clone for VaList<'f> {
     #[inline]
     fn clone(&self) -> Self {
         let mut dest = crate::mem::MaybeUninit::uninit();
-        // SAFETY: we write to the `MaybeUninit`, thus it is initialized and `assume_init` is legal
+        // SAFETY: we write to the `MaybeUninit`, thus it is initialized and `assume_init` is legal.
         unsafe {
             va_copy(dest.as_mut_ptr(), self);
             dest.assume_init()
         }
     }
 }
 
-impl<'f> Drop for VaListImpl<'f> {
+impl<'f> Drop for VaList<'f> {
     fn drop(&mut self) {
-        // FIXME: this should call `va_end`, but there's no clean way to
-        // guarantee that `drop` always gets inlined into its caller,
-        // so the `va_end` would get directly called from the same function as
-        // the corresponding `va_copy`. `man va_end` states that C requires this,
-        // and LLVM basically follows the C semantics, so we need to make sure
-        // that `va_end` is always called from the same function as `va_copy`.
-        // For more details, see https://github.com/rust-lang/rust/pull/59625
-        // and https://llvm.org/docs/LangRef.html#llvm-va-end-intrinsic.
-        //
-        // This works for now, since `va_end` is a no-op on all current LLVM targets.
+        // Rust requires that not calling `va_end` on a `va_list` does not cause undefined behaviour
+        // (as it is safe to leak values). As `va_end` is a no-op on all current LLVM targets, this
+        // destructor is empty.
     }
 }
 

library/core/src/intrinsics/mod.rs

@@ -54,7 +54,7 @@
 )]
 #![allow(missing_docs)]
 
-use crate::ffi::va_list::{VaArgSafe, VaListImpl};
+use crate::ffi::va_list::{VaArgSafe, VaList};
 use crate::marker::{ConstParamTy, Destruct, DiscriminantKind, PointeeSized, Tuple};
 use crate::ptr;
 
@@ -3305,19 +3305,19 @@ pub(crate) const fn miri_promise_symbolic_alignment(ptr: *const (), align: usize
 /// FIXME: document safety requirements
 #[rustc_intrinsic]
 #[rustc_nounwind]
-pub unsafe fn va_copy<'f>(dest: *mut VaListImpl<'f>, src: &VaListImpl<'f>);
+pub unsafe fn va_copy<'f>(dest: *mut VaList<'f>, src: &VaList<'f>);
 
 /// Loads an argument of type `T` from the `va_list` `ap` and increment the
 /// argument `ap` points to.
 ///
 /// FIXME: document safety requirements
 #[rustc_intrinsic]
 #[rustc_nounwind]
-pub unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;
+pub unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaList<'_>) -> T;
 
 /// Destroy the arglist `ap` after initialization with `va_start` or `va_copy`.
 ///
 /// FIXME: document safety requirements
 #[rustc_intrinsic]
 #[rustc_nounwind]
-pub unsafe fn va_end(ap: &mut VaListImpl<'_>);
+pub unsafe fn va_end(ap: &mut VaList<'_>);