Auto merge of #141980 - beetrees:va-list-proposal, r=workingjubilee

Rework `c_variadic`

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`

Author: bors

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, while otherwise matching Rust semantics.
+// 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
@@ -258,46 +231,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::{mem, ptr};
 
@@ -3447,7 +3447,7 @@ pub(crate) const fn miri_promise_symbolic_alignment(ptr: *const (), align: usize
 ///
 #[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.
@@ -3465,7 +3465,7 @@ pub unsafe fn va_copy<'f>(dest: *mut VaListImpl<'f>, src: &VaListImpl<'f>);
 ///
 #[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`.
 ///
@@ -3475,4 +3475,4 @@ pub unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaListImpl<'_>) -> T;
 ///
 #[rustc_intrinsic]
 #[rustc_nounwind]
-pub unsafe fn va_end(ap: &mut VaListImpl<'_>);
+pub unsafe fn va_end(ap: &mut VaList<'_>);