c_variadic: use Clone instead of LLVM va_copy

Author: folkertdev

compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

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

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

@@ -391,9 +391,6 @@ impl<'a, 'gcc, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'a, 'gcc, 'tc
             sym::breakpoint => {
                 unimplemented!();
             }
-            sym::va_copy => {
-                unimplemented!();
-            }
             sym::va_arg => {
                 unimplemented!();
             }

compiler/rustc_codegen_llvm/src/intrinsic.rs

@@ -269,14 +269,6 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
                 return Ok(());
             }
             sym::breakpoint => self.call_intrinsic("llvm.debugtrap", &[], &[]),
-            sym::va_copy => {
-                let dest = args[0].immediate();
-                self.call_intrinsic(
-                    "llvm.va_copy",
-                    &[self.val_ty(dest)],
-                    &[dest, args[1].immediate()],
-                )
-            }
             sym::va_arg => {
                 match result.layout.backend_repr {
                     BackendRepr::Scalar(scalar) => {

compiler/rustc_hir_analysis/src/check/intrinsic.rs

@@ -214,6 +214,7 @@ fn intrinsic_operation_unsafety(tcx: TyCtxt<'_>, intrinsic_id: LocalDefId) -> hi
         | sym::type_id_eq
         | sym::type_name
         | sym::ub_checks
+        | sym::va_copy
         | sym::variant_count
         | sym::vtable_for
         | sym::wrapping_add
@@ -617,14 +618,13 @@ 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);
-            (0, 0, vec![va_list_ptr_ty, va_list_ref_ty], tcx.types.unit)
+            (0, 0, vec![va_list_ref_ty], va_list_ty)
+        }
+
+        sym::va_start | sym::va_end => {
+            (0, 0, vec![mk_va_list_ty(hir::Mutability::Mut).0], tcx.types.unit)
         }
 
         sym::va_arg => (1, 0, vec![mk_va_list_ty(hir::Mutability::Mut).0], param(0)),

library/core/src/ffi/va_list.rs

@@ -48,7 +48,7 @@ crate::cfg_select! {
         /// [AArch64 Procedure Call Standard]:
         /// http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
         #[repr(C)]
-        #[derive(Debug)]
+        #[derive(Debug, Clone, Copy)]
         struct VaListInner {
             stack: *const c_void,
             gr_top: *const c_void,
@@ -66,7 +66,7 @@ crate::cfg_select! {
         /// 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)]
+        #[derive(Debug, Clone, Copy)]
         #[rustc_pass_indirectly_in_non_rustic_abis]
         struct VaListInner {
             gpr: u8,
@@ -84,7 +84,7 @@ crate::cfg_select! {
         /// [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)]
+        #[derive(Debug, Clone, Copy)]
         #[rustc_pass_indirectly_in_non_rustic_abis]
         struct VaListInner {
             gpr: i64,
@@ -101,7 +101,7 @@ crate::cfg_select! {
         /// [System V AMD64 ABI]:
         /// https://refspecs.linuxbase.org/elf/x86_64-abi-0.99.pdf
         #[repr(C)]
-        #[derive(Debug)]
+        #[derive(Debug, Clone, Copy)]
         #[rustc_pass_indirectly_in_non_rustic_abis]
         struct VaListInner {
             gp_offset: i32,
@@ -118,7 +118,7 @@ crate::cfg_select! {
         /// [LLVM source]:
         /// https://github.com/llvm/llvm-project/blob/af9a4263a1a209953a1d339ef781a954e31268ff/llvm/lib/Target/Xtensa/XtensaISelLowering.cpp#L1211-L1215
         #[repr(C)]
-        #[derive(Debug)]
+        #[derive(Debug, Clone, Copy)]
         #[rustc_pass_indirectly_in_non_rustic_abis]
         struct VaListInner {
             stk: *const i32,
@@ -135,7 +135,7 @@ crate::cfg_select! {
         /// [LLVM source]:
         /// https://github.com/llvm/llvm-project/blob/0cdc1b6dd4a870fc41d4b15ad97e0001882aba58/clang/lib/CodeGen/Targets/Hexagon.cpp#L407-L417
         #[repr(C)]
-        #[derive(Debug)]
+        #[derive(Debug, Clone, Copy)]
         #[rustc_pass_indirectly_in_non_rustic_abis]
         struct VaListInner {
             __current_saved_reg_area_pointer: *const c_void,
@@ -157,7 +157,7 @@ crate::cfg_select! {
     _ => {
         /// Basic implementation of a `va_list`.
         #[repr(transparent)]
-        #[derive(Debug)]
+        #[derive(Debug, Clone, Copy)]
         struct VaListInner {
             ptr: *const c_void,
         }
@@ -179,6 +179,39 @@ impl fmt::Debug for VaList<'_> {
     }
 }
 
+impl VaList<'_> {
+    // Helper used in the implementation of the `va_copy` intrinsic.
+    pub(crate) fn duplicate(&self) -> Self {
+        Self { inner: self.inner.clone(), _marker: self.__marker }
+    }
+}
+
+impl Clone for VaList<'_> {
+    #[inline]
+    fn clone(&self) -> Self {
+        // We only implement Clone and not Copy because some future target might not be able to
+        // implement Copy (e.g. because it allocates).
+
+        // We still use a `va_copy` intrinsic to provide a hook for const evaluation. The hook is
+        // used to report UB when a variable argument list is duplicated with a manual `memcpy`.
+        // While that works in practice for all current targets, we want to be able to support
+        // targets in the future where that is not the case.
+        va_copy(self)
+    }
+}
+
+impl Drop for VaList<'_> {
+    fn drop(&mut self) {
+        // For all current LLVM targets `va_end` is a no-op.
+        //
+        // We implement `Drop` here because some future target might need to actually run
+        // destructors (e.g. to deallocate).
+        //
+        // Rust requires that not calling `va_end` on a `va_list` does not cause undefined
+        // behaviour: it is safe to leak values.
+    }
+}
+
 mod sealed {
     pub trait Sealed {}
 
@@ -253,26 +286,6 @@ impl<'f> VaList<'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.
-        unsafe {
-            va_copy(dest.as_mut_ptr(), self);
-            dest.assume_init()
-        }
-    }
-}
-
-impl<'f> Drop for VaList<'f> {
-    fn drop(&mut self) {
-        // 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.
-    }
-}
-
 // Checks (via an assert in `compiler/rustc_ty_utils/src/abi.rs`) that the C ABI for the current
 // target correctly implements `rustc_pass_indirectly_in_non_rustic_abis`.
 const _: () = {

library/core/src/intrinsics/mod.rs

@@ -3445,19 +3445,6 @@ pub(crate) const fn miri_promise_symbolic_alignment(ptr: *const (), align: usize
     )
 }
 
-/// Copies the current location of arglist `src` to the arglist `dst`.
-///
-/// # Safety
-///
-/// You must check the following invariants before you call this function:
-///
-/// - `dest` must be non-null and point to valid, writable memory.
-/// - `dest` must not alias `src`.
-///
-#[rustc_intrinsic]
-#[rustc_nounwind]
-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.
 ///
@@ -3476,6 +3463,17 @@ pub unsafe fn va_copy<'f>(dest: *mut VaList<'f>, src: &VaList<'f>);
 #[rustc_nounwind]
 pub unsafe fn va_arg<T: VaArgSafe>(ap: &mut VaList<'_>) -> T;
 
+/// Duplicates a variable argument list. The returned list is initially at the same position as
+/// the one in `src`, but can be advanced independently.
+#[rustc_intrinsic]
+#[rustc_nounwind]
+pub fn va_copy<'f>(src: &VaList<'f>) -> VaList<'f> {
+    // NOTE: this intrinsic exists only as a hook for constant evaluation, and is used to detect UB
+    // when `VaList` is used incorrectly. Codegen backends should not have custom behavior for this
+    // intrinsic, they should always use this fallback implementation.
+    src.duplicate()
+}
+
 /// Destroy the arglist `ap` after initialization with `va_start` or `va_copy`.
 ///
 /// # Safety

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

@@ -17,14 +17,3 @@ pub unsafe extern "C" fn c_variadic_no_use(fmt: *const i8, mut ap: ...) -> i32 {
     vprintf(fmt, ap)
     // CHECK: call void @llvm.va_end
 }
-
-// Check that `VaList::clone` gets inlined into a direct call to `llvm.va_copy`
-#[no_mangle]
-pub unsafe extern "C" fn c_variadic_clone(fmt: *const i8, mut ap: ...) -> i32 {
-    // CHECK: call void @llvm.va_start
-    let mut ap2 = ap.clone();
-    // CHECK: call void @llvm.va_copy
-    let res = vprintf(fmt, ap2);
-    res
-    // CHECK: call void @llvm.va_end
-}

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

@@ -0,0 +1,24 @@
+//@ run-pass
+//@ ignore-backends: gcc
+#![feature(c_variadic)]
+
+// Test the behavior of `VaList::clone`. In C a `va_list` is duplicated using `va_copy`, but the
+// rust api just uses `Clone`. This should create a completely independent cursor into the
+// variable argument list: advancing the original has no effect on the copy and vice versa.
+
+fn main() {
+    unsafe { variadic(1, 2, 3) }
+}
+
+unsafe extern "C" fn variadic(mut ap1: ...) {
+    let mut ap2 = ap1.clone();
+
+    assert_eq!(ap1.arg::<i32>(), 1);
+    assert_eq!(ap2.arg::<i32>(), 1);
+
+    assert_eq!(ap2.arg::<i32>(), 2);
+    assert_eq!(ap1.arg::<i32>(), 2);
+
+    drop(ap1);
+    assert_eq!(ap2.arg::<i32>(), 3);
+}