Rework c_variadic

Author: beetrees

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,15 +4,15 @@
 
 #[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.
-//
 // Most targets explicitly specify the layout of `va_list`, this layout is matched here.
+// For `va_list`s which are single-element array in C (and therefore experience array-to-pointer
+// decay when passed as arguments in C), the `VaList` struct is annotated with
+// `#[rustc_pass_indirectly_in_non_rustic_abis]`. This 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",
@@ -27,66 +27,60 @@ crate::cfg_select! {
         /// 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`.
         #[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`.
         #[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`.
         #[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`.
         #[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>,
         }
     }
 
@@ -95,94 +89,32 @@ crate::cfg_select! {
     // - apple aarch64 (see https://github.com/rust-lang/rust/pull/56599)
     // - windows
     // - 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 +135,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 +170,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 +189,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<'_>);

library/std/src/ffi/mod.rs

@@ -172,7 +172,7 @@ pub use core::ffi::c_void;
               all supported platforms",
     issue = "44930"
 )]
-pub use core::ffi::{VaArgSafe, VaList, VaListImpl};
+pub use core::ffi::{VaArgSafe, VaList};
 #[stable(feature = "core_ffi_c", since = "1.64.0")]
 pub use core::ffi::{
     c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,

src/tools/compiletest/src/directives/directive_names.rs

@@ -112,6 +112,7 @@ pub(crate) const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
     "ignore-thumbv8m.base-none-eabi",
     "ignore-thumbv8m.main-none-eabi",
     "ignore-tvos",
+    "ignore-uefi",
     "ignore-unix",
     "ignore-unknown",
     "ignore-uwp",

tests/auxiliary/rust_test_helpers.c

@@ -314,6 +314,10 @@ double rust_interesting_average(uint64_t n, ...) {
     return sum;
 }
 
+int32_t rust_va_list_next_i32(va_list* ap) {
+    return va_arg(*ap, int32_t);
+}
+
 int32_t rust_int8_to_int32(int8_t x) {
     return (int32_t)x;
 }

tests/codegen-llvm/cffi/c-variadic-copy.rs

@@ -1,4 +1,4 @@
-// Tests that `VaListImpl::clone` gets inlined into a call to `llvm.va_copy`
+// Tests that `VaList::clone` gets inlined into a call to `llvm.va_copy`
 
 #![crate_type = "lib"]
 #![feature(c_variadic)]
@@ -12,5 +12,5 @@ extern "C" {
 pub unsafe extern "C" fn clone_variadic(ap: VaList) {
     let mut ap2 = ap.clone();
     // CHECK: call void @llvm.va_copy
-    foreign_c_variadic_1(ap2.as_va_list(), 42i32);
+    foreign_c_variadic_1(ap2, 42i32);
 }