Auto merge of rust-lang#128247 - RalfJung:import-attribute-clash, r=<…

…try>

codegen: do not set attributes on foreign function imports

Currently we are setting all the usual attributes when translating an FFI import, like
```rust
    extern {
        pub fn myfun(x: NonZeroI32) -> &'static mut ();
    }
```
into LLVM. However, if the same function is imported multiple times as different items in Rust, that all becomes a single LLVM declaration. The attributes set on any one of them may then apply to calls done via other imported items. For instance, the import above, *even if it is never called*, can introduce UB if there is another part of the program that also imports `myfun`, calls it via that import, and passes 0 or `myfun` returns NULL.

To fix that, this PR changes the function declarations we emit to all look like `declare `@fn()`.` The one exception are Rust's own allocator functions, which have a bunch of attributes that LLVM currently only honors in the declaration, not the call site -- though with llvm/llvm-project@1953629 we can maybe avoid that in the future.

The hope is that this fixes rust-lang#46188. The only cases I can still think of where we emit different declarations with the same name and one of them "wins" are:
- Rust's native allocation functions. Even then, the "wrong" declarations will just have no argument and return value so I don't think this should cause problems.
- Two `extern static` with the same name but different type, or an `extern static` with the same name as an imported function. Again I doubt the "type of the static" will lead LLVM to deduce things about the function or vice versa, so probably the worst that can happen is LLVM crashes. (And of course if this static actually ends up resolving to a function, that's UB unless the static has size 0. And conversely, if it resolves to a static instead of code then calling the function is UB.)

Fixes rust-lang/miri#3581 by making this not UB.

Cc `@nikic`

try-job: x86_64-msvc
try-job: i686-msvc

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

compiler/rustc_codegen_llvm/src/abi.rs

@@ -321,7 +321,12 @@ pub(crate) 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<ty::Instance<'tcx>>,
+    );
 }
 
 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<ty::Instance<'tcx>>,
+    ) {
         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,18 @@ 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.
+        // However, only do that for calls to imported functions: all the others have these
+        // attributes applied already to the declaration, so we can save some work by not also
+        // applying them here (and this really shows in perf).
+        if let Some(instance) = instance
+            && bx.tcx.is_foreign_item(instance.def_id())
+        {
+            llfn_attrs_from_instance(bx.cx, instance, None, |place, attrs| {
+                attributes::apply_to_callsite(callsite, place, attrs)
+            });
+        }
     }
 }
 

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(crate) 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(crate) 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());
 
@@ -445,7 +450,7 @@ pub(crate) 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) {
@@ -456,7 +461,7 @@ pub(crate) 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));
@@ -466,26 +471,28 @@ pub(crate) 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);
@@ -503,24 +510,27 @@ pub(crate) fn llfn_attrs_from_instance<'ll, 'tcx>(
     let function_features =
         codegen_fn_attrs.target_features.iter().map(|f| f.name.as_str()).collect::<Vec<&str>>();
 
-    if let Some(f) = llvm_util::check_tied_features(
-        cx.tcx.sess,
-        &function_features.iter().map(|f| (*f, true)).collect(),
-    ) {
-        let span = cx
-            .tcx
-            .get_attrs(instance.def_id(), sym::target_feature)
-            .next()
-            .map_or_else(|| cx.tcx.def_span(instance.def_id()), |a| a.span);
-        cx.tcx
-            .dcx()
-            .create_err(TargetFeatureDisableOrEnable {
-                features: f,
-                span: Some(span),
-                missing_features: Some(MissingFeatures),
-            })
-            .emit();
-        return;
+    // HACK: Avoid emitting the lint twice.
+    if declare_llfn.is_some() {
+        if let Some(f) = llvm_util::check_tied_features(
+            cx.tcx.sess,
+            &function_features.iter().map(|f| (*f, true)).collect(),
+        ) {
+            let span = cx
+                .tcx
+                .get_attrs(instance.def_id(), sym::target_feature)
+                .next()
+                .map_or_else(|| cx.tcx.def_span(instance.def_id()), |a| a.span);
+            cx.tcx
+                .dcx()
+                .create_err(TargetFeatureDisableOrEnable {
+                    features: f,
+                    span: Some(span),
+                    missing_features: Some(MissingFeatures),
+                })
+                .emit();
+            return;
+        }
     }
 
     let function_features = function_features
@@ -562,7 +572,7 @@ pub(crate) 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> {

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
     }
@@ -1657,7 +1657,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
     }

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,42 @@ 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 `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 {
+            return declare_raw_fn(
+                self,
+                name,
+                llvm::CallConv::CCallConv,
+                llvm::UnnamedAddr::Global,
+                llvm::Visibility::Default,
+                // The function type for `fn()`.
+                self.type_func(&[], self.type_void()),
+            );
+        }
+
         // Function addresses in Rust are never significant, allowing functions to
         // be merged.
         let llfn = declare_raw_fn(

tests/codegen/aarch64-struct-align-128.rs

@@ -39,12 +39,17 @@ pub struct Wrapped8 {
 }
 
 extern "C" {
-    // linux:  declare void @test_8([2 x i64], [2 x i64], [2 x i64])
-    // darwin: declare void @test_8([2 x i64], [2 x i64], [2 x i64])
-    // win:    declare void @test_8([2 x i64], [2 x i64], [2 x i64])
     fn test_8(a: Align8, b: Transparent8, c: Wrapped8);
 }
 
+#[no_mangle]
+fn call_test_8(a: Align8, b: Transparent8, c: Wrapped8) {
+    // linux:  call void @test_8([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    // darwin: call void @test_8([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    // win:    call void @test_8([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    unsafe { test_8(a, b, c) }
+}
+
 // Passed as `i128`, since it's an aggregate with size <= 128 bits, align = 128 bits.
 // EXCEPT on Linux, where there's a special case to use its unadjusted alignment,
 // making it the same as `Align8`, so it's be passed as `[i64 x 2]`.
@@ -69,12 +74,17 @@ pub struct Wrapped16 {
 }
 
 extern "C" {
-    // linux:  declare void @test_16([2 x i64], [2 x i64], i128)
-    // darwin: declare void @test_16(i128, i128, i128)
-    // win:    declare void @test_16(i128, i128, i128)
     fn test_16(a: Align16, b: Transparent16, c: Wrapped16);
 }
 
+#[no_mangle]
+fn call_test_16(a: Align16, b: Transparent16, c: Wrapped16) {
+    // linux:  call void @test_16([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, i128 {{%.*}})
+    // darwin: call void @test_16(i128 {{%.*}}, i128 {{%.*}}, i128 {{%.*}})
+    // win:    call void @test_16(i128 {{%.*}}, i128 {{%.*}}, i128 {{%.*}})
+    unsafe { test_16(a, b, c) }
+}
+
 // Passed as `i128`, since it's an aggregate with size <= 128 bits, align = 128 bits.
 #[repr(C)]
 pub struct I128 {
@@ -94,12 +104,17 @@ pub struct WrappedI128 {
 }
 
 extern "C" {
-    // linux:  declare void @test_i128(i128, i128, i128)
-    // darwin: declare void @test_i128(i128, i128, i128)
-    // win:    declare void @test_i128(i128, i128, i128)
     fn test_i128(a: I128, b: TransparentI128, c: WrappedI128);
 }
 
+#[no_mangle]
+fn call_test_i128(a: I128, b: TransparentI128, c: WrappedI128) {
+    // linux:  call void @test_i128(i128 {{%.*}}, i128 {{%.*}}, i128 {{%.*}})
+    // darwin: call void @test_i128(i128 {{%.*}}, i128 {{%.*}}, i128 {{%.*}})
+    // win:    call void @test_i128(i128 {{%.*}}, i128 {{%.*}}, i128 {{%.*}})
+    unsafe { test_i128(a, b, c) }
+}
+
 // Passed as `[2 x i64]`, since it's an aggregate with size <= 128 bits, align < 128 bits.
 // Note that the Linux special case does not apply, because packing is not considered "adjustment".
 #[repr(C)]
@@ -121,12 +136,17 @@ pub struct WrappedPacked {
 }
 
 extern "C" {
-    // linux:  declare void @test_packed([2 x i64], [2 x i64], [2 x i64])
-    // darwin: declare void @test_packed([2 x i64], [2 x i64], [2 x i64])
-    // win:    declare void @test_packed([2 x i64], [2 x i64], [2 x i64])
     fn test_packed(a: Packed, b: TransparentPacked, c: WrappedPacked);
 }
 
+#[no_mangle]
+fn call_test_packed(a: Packed, b: TransparentPacked, c: WrappedPacked) {
+    // linux:  call void @test_packed([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    // darwin: call void @test_packed([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    // win:    call void @test_packed([2 x i64] {{%.*}}, [2 x i64] {{%.*}}, [2 x i64] {{%.*}})
+    unsafe { test_packed(a, b, c) }
+}
+
 pub unsafe fn main(
     a1: Align8,
     a2: Transparent8,
@@ -141,8 +161,8 @@ pub unsafe fn main(
     d2: TransparentPacked,
     d3: WrappedPacked,
 ) {
-    test_8(a1, a2, a3);
-    test_16(b1, b2, b3);
-    test_i128(c1, c2, c3);
-    test_packed(d1, d2, d3);
+    call_test_8(a1, a2, a3);
+    call_test_16(b1, b2, b3);
+    call_test_i128(c1, c2, c3);
+    call_test_packed(d1, d2, d3);
 }