From d4a7a5287a3d01955bd32b342d0723aeeaf8f10c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 18 Aug 2024 21:32:32 +0200 Subject: [PATCH] declare imported functions with a nondescript signature This means when the function is imported with multiple different signatures, they don't clash --- compiler/rustc_ast_passes/src/feature_gate.rs | 2 +- compiler/rustc_codegen_llvm/src/abi.rs | 25 ++++++++++-- compiler/rustc_codegen_llvm/src/attributes.rs | 29 +++++++++----- compiler/rustc_codegen_llvm/src/builder.rs | 6 +-- compiler/rustc_codegen_llvm/src/declare.rs | 40 ++++++++++++++++++- tests/codegen/box-uninit-bytes.rs | 6 +-- tests/codegen/cffi/ffi-const.rs | 11 +++-- tests/codegen/cffi/ffi-pure.rs | 11 +++-- tests/codegen/issues/issue-111603.rs | 2 +- tests/codegen/iter-repeat-n-trivial-drop.rs | 2 +- tests/codegen/no_builtins-at-crate.rs | 6 +-- tests/codegen/pic-relocation-model.rs | 5 +-- tests/codegen/pie-relocation-model.rs | 2 +- tests/codegen/unwind-extern-imports.rs | 22 +++++++--- tests/codegen/unwind-landingpad-cold.rs | 3 +- 15 files changed, 124 insertions(+), 48 deletions(-) diff --git a/compiler/rustc_ast_passes/src/feature_gate.rs b/compiler/rustc_ast_passes/src/feature_gate.rs index 214a37bca03e2..07324dddad0a7 100644 --- a/compiler/rustc_ast_passes/src/feature_gate.rs +++ b/compiler/rustc_ast_passes/src/feature_gate.rs @@ -324,7 +324,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> { &self, link_llvm_intrinsics, i.span, - "linking to LLVM intrinsics is experimental" + "linking to LLVM intrinsics is an internal feature reserved for the standard library" ); } } diff --git a/compiler/rustc_codegen_llvm/src/abi.rs b/compiler/rustc_codegen_llvm/src/abi.rs index 5ff580e295a60..46e5bd385f078 100644 --- a/compiler/rustc_codegen_llvm/src/abi.rs +++ b/compiler/rustc_codegen_llvm/src/abi.rs @@ -321,7 +321,12 @@ pub trait FnAbiLlvmExt<'ll, 'tcx> { ); /// Apply attributes to a function call. - fn apply_attrs_callsite(&self, bx: &mut Builder<'_, 'll, 'tcx>, callsite: &'ll Value); + fn apply_attrs_callsite( + &self, + bx: &mut Builder<'_, 'll, 'tcx>, + callsite: &'ll Value, + instance: Option>, + ); } impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> { @@ -527,11 +532,18 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> { // If the declaration has an associated instance, compute extra attributes based on that. if let Some(instance) = instance { - llfn_attrs_from_instance(cx, llfn, instance); + llfn_attrs_from_instance(cx, instance, Some(llfn), |place, attrs| { + attributes::apply_to_llfn(llfn, place, attrs) + }); } } - fn apply_attrs_callsite(&self, bx: &mut Builder<'_, 'll, 'tcx>, callsite: &'ll Value) { + fn apply_attrs_callsite( + &self, + bx: &mut Builder<'_, 'll, 'tcx>, + callsite: &'ll Value, + instance: Option>, + ) { let mut func_attrs = SmallVec::<[_; 2]>::new(); if self.ret.layout.abi.is_uninhabited() { func_attrs.push(llvm::AttributeKind::NoReturn.create_attr(bx.cx.llcx)); @@ -649,6 +661,13 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> { &[element_type_attr], ); } + + // If the call site has an associated instance, compute extra attributes based on that. + if let Some(instance) = instance { + llfn_attrs_from_instance(bx.cx, instance, None, |place, attrs| { + attributes::apply_to_callsite(callsite, place, attrs) + }); + } } } diff --git a/compiler/rustc_codegen_llvm/src/attributes.rs b/compiler/rustc_codegen_llvm/src/attributes.rs index fde95104093e5..67a896483505e 100644 --- a/compiler/rustc_codegen_llvm/src/attributes.rs +++ b/compiler/rustc_codegen_llvm/src/attributes.rs @@ -14,8 +14,8 @@ use crate::context::CodegenCx; use crate::errors::{MissingFeatures, SanitizerMemtagRequiresMte, TargetFeatureDisableOrEnable}; use crate::llvm::AttributePlace::Function; use crate::llvm::{self, AllocKindFlags, Attribute, AttributeKind, AttributePlace, MemoryEffects}; +use crate::llvm_util; use crate::value::Value; -use crate::{attributes, llvm_util}; pub fn apply_to_llfn(llfn: &Value, idx: AttributePlace, attrs: &[&Attribute]) { if !attrs.is_empty() { @@ -324,13 +324,18 @@ fn create_alloc_family_attr(llcx: &llvm::Context) -> &llvm::Attribute { llvm::CreateAttrStringValue(llcx, "alloc-family", "__rust_alloc") } -/// Helper for `FnAbi::apply_attrs_llfn`: +/// Helper for `FnAbi::apply_attrs_*`: /// Composite function which sets LLVM attributes for function depending on its AST (`#[attribute]`) /// attributes. +/// +/// `apply_attrs` is called to apply the attributes, so this can be used both for declarations and +/// calls. However, some things are not represented as attributes and can only be set on +/// declarations, so `declare_llfn` should be `Some` if this is a declaration. pub fn llfn_attrs_from_instance<'ll, 'tcx>( cx: &CodegenCx<'ll, 'tcx>, - llfn: &'ll Value, instance: ty::Instance<'tcx>, + declare_llfn: Option<&'ll Value>, + apply_attrs: impl Fn(AttributePlace, &[&Attribute]), ) { let codegen_fn_attrs = cx.tcx.codegen_fn_attrs(instance.def_id()); @@ -440,7 +445,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>( to_add.push(create_alloc_family_attr(cx.llcx)); // apply to argument place instead of function let alloc_align = AttributeKind::AllocAlign.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::Argument(1), &[alloc_align]); + apply_attrs(AttributePlace::Argument(1), &[alloc_align]); to_add.push(llvm::CreateAllocSizeAttr(cx.llcx, 0)); let mut flags = AllocKindFlags::Alloc | AllocKindFlags::Aligned; if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR) { @@ -451,7 +456,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>( to_add.push(llvm::CreateAllocKindAttr(cx.llcx, flags)); // apply to return place instead of function (unlike all other attributes applied in this function) let no_alias = AttributeKind::NoAlias.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::ReturnValue, &[no_alias]); + apply_attrs(AttributePlace::ReturnValue, &[no_alias]); } if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::REALLOCATOR) { to_add.push(create_alloc_family_attr(cx.llcx)); @@ -461,26 +466,28 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>( )); // applies to argument place instead of function place let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]); + apply_attrs(AttributePlace::Argument(0), &[allocated_pointer]); // apply to argument place instead of function let alloc_align = AttributeKind::AllocAlign.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::Argument(2), &[alloc_align]); + apply_attrs(AttributePlace::Argument(2), &[alloc_align]); to_add.push(llvm::CreateAllocSizeAttr(cx.llcx, 3)); let no_alias = AttributeKind::NoAlias.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::ReturnValue, &[no_alias]); + apply_attrs(AttributePlace::ReturnValue, &[no_alias]); } if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::DEALLOCATOR) { to_add.push(create_alloc_family_attr(cx.llcx)); to_add.push(llvm::CreateAllocKindAttr(cx.llcx, AllocKindFlags::Free)); // applies to argument place instead of function place let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx); - attributes::apply_to_llfn(llfn, AttributePlace::Argument(0), &[allocated_pointer]); + apply_attrs(AttributePlace::Argument(0), &[allocated_pointer]); } if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::CMSE_NONSECURE_ENTRY) { to_add.push(llvm::CreateAttrString(cx.llcx, "cmse_nonsecure_entry")); } if let Some(align) = codegen_fn_attrs.alignment { - llvm::set_alignment(llfn, align); + if let Some(llfn) = declare_llfn { + llvm::set_alignment(llfn, align); + } } if let Some(backchain) = backchain_attr(cx) { to_add.push(backchain); @@ -551,7 +558,7 @@ pub fn llfn_attrs_from_instance<'ll, 'tcx>( to_add.push(llvm::CreateAttrStringValue(cx.llcx, "target-features", &target_features)); } - attributes::apply_to_llfn(llfn, Function, &to_add); + apply_attrs(Function, &to_add); } fn wasm_import_module(tcx: TyCtxt<'_>, id: DefId) -> Option<&String> { diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs index cc081f29e128e..bcbdd671d51f0 100644 --- a/compiler/rustc_codegen_llvm/src/builder.rs +++ b/compiler/rustc_codegen_llvm/src/builder.rs @@ -262,7 +262,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { ) }; if let Some(fn_abi) = fn_abi { - fn_abi.apply_attrs_callsite(self, invoke); + fn_abi.apply_attrs_callsite(self, invoke, instance); } invoke } @@ -1307,7 +1307,7 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> { ) }; if let Some(fn_abi) = fn_abi { - fn_abi.apply_attrs_callsite(self, call); + fn_abi.apply_attrs_callsite(self, call, instance); } call } @@ -1645,7 +1645,7 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> { ) }; if let Some(fn_abi) = fn_abi { - fn_abi.apply_attrs_callsite(self, callbr); + fn_abi.apply_attrs_callsite(self, callbr, instance); } callbr } diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs index 2aa349b278234..20336f0e61a15 100644 --- a/compiler/rustc_codegen_llvm/src/declare.rs +++ b/compiler/rustc_codegen_llvm/src/declare.rs @@ -12,8 +12,9 @@ //! * When in doubt, define. use itertools::Itertools; -use rustc_codegen_ssa::traits::TypeMembershipMethods; +use rustc_codegen_ssa::traits::{BaseTypeMethods, TypeMembershipMethods}; use rustc_data_structures::fx::FxIndexSet; +use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::ty::{Instance, Ty}; use rustc_sanitizers::{cfi, kcfi}; use smallvec::SmallVec; @@ -127,6 +128,43 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { ) -> &'ll Value { debug!("declare_rust_fn(name={:?}, fn_abi={:?})", name, fn_abi); + // FFI imports need to be treated specially: in Rust one can import the same function + // with different signatures, but LLVM can only import each function once. Within a + // codegen unit, whatever declaration we set up last will "win"; across codegen units, + // LLVM is doing some sort of unspecified merging as part of LTO. + // This is partially unsound due to LLVM bugs (https://github.com/llvm/llvm-project/issues/58976), + // but on the Rust side we try to ensure soundness by making the least amount of promises + // for these imports: no matter the signatures, we declare all functions as `void fn()`. + // If the same symbol is declared both as a function and a static, we just hope that + // doesn't lead to unsoundness (or LLVM crashes)... + let is_foreign_item = instance.is_some_and(|i| self.tcx.is_foreign_item(i.def_id())); + // If this is a Rust native allocator function, we want its attributes, so we do not + // treat it like the other FFI imports. If a user declares `__rust_alloc` we're going to have + // one import with the attributes and one with the default signature; that should hopefully be fine + // even if LLVM only sees the default one. + let is_rust_alloc_fn = instance.is_some_and(|i| { + let codegen_fn_flags = self.tcx.codegen_fn_attrs(i.def_id()).flags; + codegen_fn_flags.contains(CodegenFnAttrFlags::ALLOCATOR) + || codegen_fn_flags.contains(CodegenFnAttrFlags::ALLOCATOR_ZEROED) + || codegen_fn_flags.contains(CodegenFnAttrFlags::REALLOCATOR) + || codegen_fn_flags.contains(CodegenFnAttrFlags::DEALLOCATOR) + }); + // If this is calling an LLVM intrinsic, we must set the right signature or LLVM complains. + // These are also unstable to call so nobody but us can screw up their signature. + let is_llvm_builtin = name.starts_with("llvm."); + if is_foreign_item && !is_rust_alloc_fn && !is_llvm_builtin { + // This signatures is essentially irrelevant since we set all the attributes at the call + // site. + return declare_raw_fn( + self, + name, + llvm::CallConv::CCallConv, + llvm::UnnamedAddr::Global, + llvm::Visibility::Default, + self.type_func(&[], self.type_void()), + ); + } + // Function addresses in Rust are never significant, allowing functions to // be merged. let llfn = declare_raw_fn( diff --git a/tests/codegen/box-uninit-bytes.rs b/tests/codegen/box-uninit-bytes.rs index 63a6c7b841560..c4feed64a3121 100644 --- a/tests/codegen/box-uninit-bytes.rs +++ b/tests/codegen/box-uninit-bytes.rs @@ -8,7 +8,7 @@ use std::mem::MaybeUninit; pub fn box_uninitialized() -> Box> { // CHECK-LABEL: @box_uninitialized // CHECK-NOT: store - // CHECK-NOT: alloca + // CHECK-NOT: "alloca " // CHECK-NOT: memcpy // CHECK-NOT: memset Box::new(MaybeUninit::uninit()) @@ -19,7 +19,7 @@ pub fn box_uninitialized() -> Box> { pub fn box_uninitialized2() -> Box> { // CHECK-LABEL: @box_uninitialized2 // CHECK-NOT: store - // CHECK-NOT: alloca + // CHECK-NOT: "alloca "" // CHECK-NOT: memcpy // CHECK-NOT: memset Box::new(MaybeUninit::uninit()) @@ -32,7 +32,7 @@ pub struct LotsaPadding(usize); #[no_mangle] pub fn box_lotsa_padding() -> Box { // CHECK-LABEL: @box_lotsa_padding - // CHECK-NOT: alloca + // CHECK-NOT: "alloca " // CHECK-NOT: getelementptr // CHECK-NOT: memcpy // CHECK-NOT: memset diff --git a/tests/codegen/cffi/ffi-const.rs b/tests/codegen/cffi/ffi-const.rs index 564b8f7f8d8d2..d241642cb2838 100644 --- a/tests/codegen/cffi/ffi-const.rs +++ b/tests/codegen/cffi/ffi-const.rs @@ -2,15 +2,18 @@ #![crate_type = "lib"] #![feature(ffi_const)] +#[no_mangle] pub fn bar() { + // CHECK-LABEL: @bar + // CHECK: @foo + // CHECK-SAME: [[ATTRS:#[0-9]+]] unsafe { foo() } } extern "C" { - // CHECK-LABEL: declare{{.*}}void @foo() - // CHECK-SAME: [[ATTRS:#[0-9]+]] - // The attribute changed from `readnone` to `memory(none)` with LLVM 16.0. - // CHECK-DAG: attributes [[ATTRS]] = { {{.*}}{{readnone|memory\(none\)}}{{.*}} } #[ffi_const] pub fn foo(); } + +// The attribute changed from `readnone` to `memory(none)` with LLVM 16.0. +// CHECK: attributes [[ATTRS]] = { {{.*}}{{readnone|memory\(none\)}}{{.*}} } diff --git a/tests/codegen/cffi/ffi-pure.rs b/tests/codegen/cffi/ffi-pure.rs index 601509d5c90fe..17a39ba2690c2 100644 --- a/tests/codegen/cffi/ffi-pure.rs +++ b/tests/codegen/cffi/ffi-pure.rs @@ -2,15 +2,18 @@ #![crate_type = "lib"] #![feature(ffi_pure)] +#[no_mangle] pub fn bar() { + // CHECK-LABEL: @bar + // CHECK: @foo + // CHECK-SAME: [[ATTRS:#[0-9]+]] unsafe { foo() } } extern "C" { - // CHECK-LABEL: declare{{.*}}void @foo() - // CHECK-SAME: [[ATTRS:#[0-9]+]] - // The attribute changed from `readonly` to `memory(read)` with LLVM 16.0. - // CHECK-DAG: attributes [[ATTRS]] = { {{.*}}{{readonly|memory\(read\)}}{{.*}} } #[ffi_pure] pub fn foo(); } + +// The attribute changed from `readonly` to `memory(read)` with LLVM 16.0. +// CHECK: attributes [[ATTRS]] = { {{.*}}{{readonly|memory\(read\)}}{{.*}} } diff --git a/tests/codegen/issues/issue-111603.rs b/tests/codegen/issues/issue-111603.rs index 41bfb493ff580..c909846d4739d 100644 --- a/tests/codegen/issues/issue-111603.rs +++ b/tests/codegen/issues/issue-111603.rs @@ -12,7 +12,7 @@ pub fn new_from_array(x: u64) -> Arc<[u64]> { // CHECK: alloca // CHECK-SAME: [8000 x i8] - // CHECK-NOT: alloca + // CHECK-NOT: "alloca " let array = [x; 1000]; Arc::new(array) } diff --git a/tests/codegen/iter-repeat-n-trivial-drop.rs b/tests/codegen/iter-repeat-n-trivial-drop.rs index 7de224b92d853..f2abefd9c59ec 100644 --- a/tests/codegen/iter-repeat-n-trivial-drop.rs +++ b/tests/codegen/iter-repeat-n-trivial-drop.rs @@ -47,7 +47,7 @@ pub fn iter_repeat_n_next(it: &mut std::iter::RepeatN) -> Option Vec { - // CHECK: %[[ADDR:.+]] = tail call {{(noalias )?}}noundef dereferenceable_or_null(1234) ptr @__rust_alloc(i64 noundef 1234, i64 noundef 1) + // CHECK: %[[ADDR:.+]] = tail call {{(noalias )?}}noundef dereferenceable_or_null(1234) ptr @__rust_alloc(i64 noundef 1234, i64 {{(allocalign )?}}noundef 1) // CHECK: tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(1234) %[[ADDR]], i8 42, i64 1234, let n = 1234_usize; diff --git a/tests/codegen/no_builtins-at-crate.rs b/tests/codegen/no_builtins-at-crate.rs index ba1d31f60c390..234fb8e99842e 100644 --- a/tests/codegen/no_builtins-at-crate.rs +++ b/tests/codegen/no_builtins-at-crate.rs @@ -10,15 +10,13 @@ pub unsafe extern "C" fn __aeabi_memcpy(dest: *mut u8, src: *const u8, size: usize) { // CHECK: call // CHECK-SAME: @memcpy( + // CHECK-SAME: #2 memcpy(dest, src, size); } -// CHECK: declare -// CHECK-SAME: @memcpy -// CHECK-SAME: #0 extern "C" { pub fn memcpy(dest: *mut u8, src: *const u8, n: usize) -> *mut u8; } -// CHECK: attributes #0 +// CHECK: attributes #2 // CHECK-SAME: "no-builtins" diff --git a/tests/codegen/pic-relocation-model.rs b/tests/codegen/pic-relocation-model.rs index a1d1678a6bda5..54fbc2092d1c6 100644 --- a/tests/codegen/pic-relocation-model.rs +++ b/tests/codegen/pic-relocation-model.rs @@ -8,10 +8,7 @@ pub fn call_foreign_fn() -> u8 { unsafe { foreign_fn() } } -// (Allow but do not require `zeroext` here, because it is not worth effort to -// spell out which targets have it and which ones do not; see rust#97800.) - -// CHECK: declare{{( zeroext)?}} i8 @foreign_fn() +// CHECK: declare void @foreign_fn() extern "C" { fn foreign_fn() -> u8; } diff --git a/tests/codegen/pie-relocation-model.rs b/tests/codegen/pie-relocation-model.rs index b10af69345228..a678b4baa08df 100644 --- a/tests/codegen/pie-relocation-model.rs +++ b/tests/codegen/pie-relocation-model.rs @@ -13,7 +13,7 @@ pub fn call_foreign_fn() -> u8 { // External functions are still marked as non-dso_local, since we don't know if the symbol // is defined in the binary or in the shared library. -// CHECK: declare zeroext i8 @foreign_fn() +// CHECK: declare void @foreign_fn() extern "C" { fn foreign_fn() -> u8; } diff --git a/tests/codegen/unwind-extern-imports.rs b/tests/codegen/unwind-extern-imports.rs index dfae8aae64a51..5c6d8129d7c2f 100644 --- a/tests/codegen/unwind-extern-imports.rs +++ b/tests/codegen/unwind-extern-imports.rs @@ -3,9 +3,22 @@ #![crate_type = "lib"] +// The flag are only at the call sites, not at the declarations. +// CHECK-LABEL: @force_declare +#[no_mangle] +pub unsafe fn force_declare() { + // CHECK: call void @extern_fn() [[NOUNWIND_ATTR:#[0-9]+]] + extern_fn(); + // Call without attributes. + // FIXME: Make sure that there truly are no attributes! + // How can we match "end of line" with FileCheck? + // CHECK: call void @c_unwind_extern_fn() + c_unwind_extern_fn(); +} + extern "C" { - // CHECK: Function Attrs:{{.*}}nounwind - // CHECK-NEXT: declare{{.*}}void @extern_fn + // CHECK-NOT: nounwind + // CHECK: declare{{.*}}void @extern_fn fn extern_fn(); } @@ -15,7 +28,4 @@ extern "C-unwind" { fn c_unwind_extern_fn(); } -pub unsafe fn force_declare() { - extern_fn(); - c_unwind_extern_fn(); -} +// CHECK: attributes [[NOUNWIND_ATTR]] = { {{.*}}nounwind{{.*}} } diff --git a/tests/codegen/unwind-landingpad-cold.rs b/tests/codegen/unwind-landingpad-cold.rs index fa200bc300cf9..9d9b1bf07566a 100644 --- a/tests/codegen/unwind-landingpad-cold.rs +++ b/tests/codegen/unwind-landingpad-cold.rs @@ -8,7 +8,8 @@ // CHECK-LABEL: @check_cold // CHECK: {{(call|invoke) void .+}}drop_in_place{{.+}} [[ATTRIBUTES:#[0-9]+]] -// CHECK: attributes [[ATTRIBUTES]] = { cold } +// CHECK: attributes [[ATTRIBUTES]] +// CHECK-SAME: cold #[no_mangle] pub fn check_cold(f: fn(), x: Box) { // this may unwind