diff --git a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs b/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs index a78c6e0a4e7ac..ab9a11305baa3 100644 --- a/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs +++ b/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", diff --git a/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs b/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs index 36ea76cbc51a0..553e4d3d2fe09 100644 --- a/compiler/rustc_codegen_gcc/src/intrinsic/mod.rs +++ b/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!(); } diff --git a/compiler/rustc_codegen_llvm/src/intrinsic.rs b/compiler/rustc_codegen_llvm/src/intrinsic.rs index 481f75f337d63..bf61dd34b590c 100644 --- a/compiler/rustc_codegen_llvm/src/intrinsic.rs +++ b/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) => { diff --git a/compiler/rustc_hir_analysis/src/check/intrinsic.rs b/compiler/rustc_hir_analysis/src/check/intrinsic.rs index c84c1a8ca16d8..cfdd56aa032eb 100644 --- a/compiler/rustc_hir_analysis/src/check/intrinsic.rs +++ b/compiler/rustc_hir_analysis/src/check/intrinsic.rs @@ -215,6 +215,7 @@ fn intrinsic_operation_unsafety(tcx: TyCtxt<'_>, intrinsic_id: LocalDefId) -> hi | sym::type_name | sym::type_of | sym::ub_checks + | sym::va_copy | sym::variant_count | sym::vtable_for | sym::wrapping_add @@ -627,14 +628,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)), diff --git a/library/core/src/ffi/va_list.rs b/library/core/src/ffi/va_list.rs index d5166baf0c7ca..66d1c9396b4c9 100644 --- a/library/core/src/ffi/va_list.rs +++ b/library/core/src/ffi/va_list.rs @@ -5,7 +5,7 @@ #[cfg(not(target_arch = "xtensa"))] use crate::ffi::c_void; use crate::fmt; -use crate::intrinsics::{va_arg, va_copy}; +use crate::intrinsics::{va_arg, va_copy, va_end}; use crate::marker::PhantomCovariantLifetime; // There are currently three flavors of how a C `va_list` is implemented for @@ -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,34 @@ 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<'f> Drop for VaList<'f> { + fn drop(&mut self) { + // SAFETY: this variable argument list is being dropped, so won't be read from again. + unsafe { va_end(self) } + } +} + mod sealed { pub trait Sealed {} @@ -253,26 +281,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 _: () = { diff --git a/library/core/src/intrinsics/mod.rs b/library/core/src/intrinsics/mod.rs index 20f34036b25c9..6fe3c25579f87 100644 --- a/library/core/src/intrinsics/mod.rs +++ b/library/core/src/intrinsics/mod.rs @@ -3450,19 +3450,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. /// @@ -3481,7 +3468,25 @@ pub unsafe fn va_copy<'f>(dest: *mut VaList<'f>, src: &VaList<'f>); #[rustc_nounwind] pub unsafe fn va_arg(ap: &mut VaList<'_>) -> T; -/// Destroy the arglist `ap` after initialization with `va_start` or `va_copy`. +/// 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 variable argument list `ap` after initialization with `va_start` (part of the +/// desugaring of `...`) or `va_copy`. +/// +/// Code generation backends should not provide a custom implementation for this intrinsic. This +/// intrinsic *does not* map to the LLVM `va_end` intrinsic. +/// +/// This function is a no-op on all current targets, but used as a hook for const evaluation to +/// detect UB when a variable argument list is used incorrectly. /// /// # Safety /// @@ -3489,4 +3494,6 @@ pub unsafe fn va_arg(ap: &mut VaList<'_>) -> T; /// #[rustc_intrinsic] #[rustc_nounwind] -pub unsafe fn va_end(ap: &mut VaList<'_>); +pub unsafe fn va_end(ap: &mut VaList<'_>) { + /* deliberately does nothing */ +} diff --git a/tests/codegen-llvm/cffi/c-variadic-copy.rs b/tests/codegen-llvm/cffi/c-variadic-copy.rs deleted file mode 100644 index 0cbdcb4bbb85c..0000000000000 --- a/tests/codegen-llvm/cffi/c-variadic-copy.rs +++ /dev/null @@ -1,16 +0,0 @@ -// Tests that `VaList::clone` gets inlined into a call to `llvm.va_copy` - -#![crate_type = "lib"] -#![feature(c_variadic)] -#![no_std] -use core::ffi::VaList; - -extern "C" { - fn foreign_c_variadic_1(_: VaList, ...); -} - -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, 42i32); -} diff --git a/tests/codegen-llvm/cffi/c-variadic-opt.rs b/tests/codegen-llvm/cffi/c-variadic-opt.rs index 3cc0c3e9f9bdd..c779b25450fd1 100644 --- a/tests/codegen-llvm/cffi/c-variadic-opt.rs +++ b/tests/codegen-llvm/cffi/c-variadic-opt.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 -} diff --git a/tests/ui/c-variadic/copy.rs b/tests/ui/c-variadic/copy.rs new file mode 100644 index 0000000000000..e9171738aa157 --- /dev/null +++ b/tests/ui/c-variadic/copy.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::(), 1); + assert_eq!(ap2.arg::(), 1); + + assert_eq!(ap2.arg::(), 2); + assert_eq!(ap1.arg::(), 2); + + drop(ap1); + assert_eq!(ap2.arg::(), 3); +}