Rework c_variadic

Author: beetrees

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

@@ -1450,7 +1450,7 @@ fn codegen_regular_intrinsic_call<'tcx>(
         }
 
         // FIXME implement variadics in cranelift
-        sym::va_copy | sym::va_arg | sym::va_end => {
+        sym::va_copy | sym::va_arg => {
             fx.tcx.dcx().span_fatal(
                 source_info.span,
                 "Defining variadic functions is not yet supported by Cranelift",

compiler/rustc_codegen_ssa/src/mir/intrinsic.rs

@@ -131,8 +131,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 return Ok(());
             }
 
-            sym::va_start => bx.va_start(args[0].immediate()),
-            sym::va_end => bx.va_end(args[0].immediate()),
             sym::size_of_val => {
                 let tp_ty = fn_args.type_at(0);
                 let (_, meta) = args[0].val.pointer_parts();

compiler/rustc_codegen_ssa/src/traits/intrinsic.rs

@@ -30,10 +30,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;
 }

compiler/rustc_hir_analysis/src/check/intrinsic.rs

@@ -506,10 +506,6 @@ pub(crate) fn check_intrinsic_type(
             )
         }
 
-        sym::va_start | sym::va_end => {
-            (0, 0, vec![mk_va_list_ty(hir::Mutability::Mut).0], tcx.types.unit)
-        }
-
         sym::va_copy => {
             let (va_list_ref_ty, va_list_ty) = mk_va_list_ty(hir::Mutability::Not);
             let va_list_ptr_ty = Ty::new_mut_ptr(tcx, va_list_ty);

compiler/rustc_span/src/symbol.rs

@@ -2310,9 +2310,7 @@ symbols! {
         v8plus,
         va_arg,
         va_copy,
-        va_end,
         va_list,
-        va_start,
         val,
         validity,
         values,

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

@@ -5,13 +5,27 @@
 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::PhantomInvariantLifetime;
 
-// 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.
+
+// Note that currently support for `#[rustc_pass_indirectly_in_non_rustic_abis]` is only implemented
+// on architectures which need it here, so when adding support for a new architecture the following
+// will need to happen:
+//
+// - Check that the calling conventions used on the new architecture correctly check
+//   `arg.layout.pass_indirectly_in_non_rustic_abis()` and call `arg.make_indirect()` if it returns
+//   `true`.
+// - Add a revision to the `tests/ui/abi/pass-indirectly-attr.rs` test for the new architecture.
+// - Add the new architecture to the `supported_architectures` array in the
+//   `check_pass_indirectly_in_non_rustic_abis` function in
+//   `compiler/rustc_passes/src/check_attr.rs`. This will stop the compiler from emitting an error
+//   message when the attribute is used on that architecture.
 crate::cfg_select! {
     all(
         target_arch = "aarch64",
@@ -27,7 +41,8 @@ crate::cfg_select! {
         #[cfg_attr(not(doc), repr(C))] // work around https://github.com/rust-lang/rust/issues/66401
         #[derive(Debug)]
         #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        pub struct VaList<'f> {
             stack: *mut c_void,
             gr_top: *mut c_void,
             vr_top: *mut c_void,
@@ -41,7 +56,8 @@ crate::cfg_select! {
         #[cfg_attr(not(doc), repr(C))] // work around https://github.com/rust-lang/rust/issues/66401
         #[derive(Debug)]
         #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        pub struct VaList<'f> {
             gpr: u8,
             fpr: u8,
             reserved: u16,
@@ -55,7 +71,8 @@ crate::cfg_select! {
         #[cfg_attr(not(doc), repr(C))] // work around https://github.com/rust-lang/rust/issues/66401
         #[derive(Debug)]
         #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        pub struct VaList<'f> {
             gpr: i64,
             fpr: i64,
             overflow_arg_area: *mut c_void,
@@ -68,7 +85,8 @@ crate::cfg_select! {
         #[cfg_attr(not(doc), repr(C))] // work around https://github.com/rust-lang/rust/issues/66401
         #[derive(Debug)]
         #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        pub struct VaList<'f> {
             gp_offset: i32,
             fp_offset: i32,
             overflow_arg_area: *mut c_void,
@@ -81,7 +99,8 @@ crate::cfg_select! {
         #[repr(C)]
         #[derive(Debug)]
         #[lang = "va_list"]
-        pub struct VaListImpl<'f> {
+        #[rustc_pass_indirectly_in_non_rustic_abis]
+        pub struct VaList<'f> {
             stk: *mut i32,
             reg: *mut i32,
             ndx: i32,
@@ -94,97 +113,30 @@ 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 `VaList` 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> {
+        pub struct VaList<'f> {
             ptr: *mut c_void,
 
-            // Invariant over `'f`, so each `VaListImpl<'f>` object is tied to
+            // Invariant over `'f`, so each `VaList<'f>` object is tied to
             // the region of the function it's defined in
             _marker: PhantomInvariantLifetime<'f>,
         }
 
-        impl<'f> fmt::Debug for VaListImpl<'f> {
+        impl<'f> fmt::Debug for VaList<'f> {
             fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(f, "va_list* {:p}", self.ptr)
+                f.debug_tuple("VaList").field(&self.ptr).finish()
             }
         }
     }
 }
 
-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 }
-            }
-        }
-    }
-}
-
-impl<'a, 'f: 'a> Deref for VaList<'a, 'f> {
-    type Target = VaListImpl<'f>;
-
-    #[inline]
-    fn deref(&self) -> &VaListImpl<'f> {
-        &self.inner
-    }
-}
-
-impl<'a, 'f: 'a> DerefMut for VaList<'a, 'f> {
-    #[inline]
-    fn deref_mut(&mut self) -> &mut VaListImpl<'f> {
-        &mut self.inner
-    }
-}
-
 mod sealed {
     pub trait Sealed {}
 
@@ -202,7 +154,7 @@ mod sealed {
     impl<T> Sealed for *const T {}
 }
 
-/// Trait which permits the allowed types to be used with [`VaListImpl::arg`].
+/// Trait which permits the allowed types to be used with [`VaList::arg`].
 ///
 /// # Safety
 ///
@@ -232,52 +184,31 @@ 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 the next arg.
     #[inline]
     pub unsafe fn arg<T: VaArgSafe>(&mut self) -> T {
         // 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, DiscriminantKind, PointeeSized, Tuple};
 use crate::ptr;
 
@@ -3149,19 +3149,12 @@ 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;
-
-/// 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_arg<T: VaArgSafe>(ap: &mut VaList<'_>) -> T;

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,

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/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);
 }