codegen: do not set attributes on foreign function imports

RalfJung · RalfJung · commit e72b11a79b8c · 2024-07-26T21:59:17.000+02:00
diff --git a/compiler/rustc_codegen_llvm/src/abi.rs b/compiler/rustc_codegen_llvm/src/abi.rs
@@ -27,7 +27,14 @@ use smallvec::SmallVec;
 use std::cmp;
 
 pub trait ArgAttributesExt {
-    fn apply_attrs_to_llfn(&self, idx: AttributePlace, cx: &CodegenCx<'_, '_>, llfn: &Value);
+    fn apply_attrs_to_llfn(
+        &self,
+        idx: AttributePlace,
+        cx: &CodegenCx<'_, '_>,
+        llfn: &Value,
+        is_foreign_fn: bool,
+        force_set_align: bool,
+    );
     fn apply_attrs_to_callsite(
         &self,
         idx: AttributePlace,
@@ -47,7 +54,16 @@ const OPTIMIZATION_ATTRIBUTES: [(ArgAttribute, llvm::AttributeKind); 5] = [
     (ArgAttribute::NoUndef, llvm::AttributeKind::NoUndef),
 ];
 
-fn get_attrs<'ll>(this: &ArgAttributes, cx: &CodegenCx<'ll, '_>) -> SmallVec<[&'ll Attribute; 8]> {
+/// `is_foreign_fn_decl` indicates whether the attributes should be computed for a foreign function declaration,
+/// where we emit non-essential attributes.
+/// See <https://github.com/rust-lang/rust/issues/46188> for background and discussion.
+/// `force_set_align` indicates whether the alignment is ABI-relevant.
+fn get_attrs<'ll>(
+    this: &ArgAttributes,
+    cx: &CodegenCx<'ll, '_>,
+    is_foreign_fn_decl: bool,
+    force_set_align: bool,
+) -> SmallVec<[&'ll Attribute; 8]> {
     let mut regular = this.regular;
 
     let mut attrs = SmallVec::new();
@@ -58,15 +74,31 @@ fn get_attrs<'ll>(this: &ArgAttributes, cx: &CodegenCx<'ll, '_>) -> SmallVec<[&'
             attrs.push(llattr.create_attr(cx.llcx));
         }
     }
-    if let Some(align) = this.pointee_align {
-        attrs.push(llvm::CreateAlignmentAttr(cx.llcx, align.bytes()));
+    if force_set_align {
+        if let Some(align) = this.pointee_align {
+            attrs.push(llvm::CreateAlignmentAttr(cx.llcx, align.bytes()));
+        }
     }
     match this.arg_ext {
         ArgExtension::None => {}
         ArgExtension::Zext => attrs.push(llvm::AttributeKind::ZExt.create_attr(cx.llcx)),
         ArgExtension::Sext => attrs.push(llvm::AttributeKind::SExt.create_attr(cx.llcx)),
     }
 
+    // For foreign function declarations, this is it.
+    if is_foreign_fn_decl {
+        return attrs;
+    }
+
+    // If we didn't add alignment yet, do it now.
+    // HACK: this should really be inside the "only if optimizations" below,
+    // but to avoid tedious codegen test changes, we have it here.
+    if !force_set_align {
+        if let Some(align) = this.pointee_align {
+            attrs.push(llvm::CreateAlignmentAttr(cx.llcx, align.bytes()));
+        }
+    }
+
     // Only apply remaining attributes when optimizing
     if cx.sess().opts.optimize != config::OptLevel::No {
         let deref = this.pointee_size.bytes();
@@ -96,8 +128,15 @@ fn get_attrs<'ll>(this: &ArgAttributes, cx: &CodegenCx<'ll, '_>) -> SmallVec<[&'
 }
 
 impl ArgAttributesExt for ArgAttributes {
-    fn apply_attrs_to_llfn(&self, idx: AttributePlace, cx: &CodegenCx<'_, '_>, llfn: &Value) {
-        let attrs = get_attrs(self, cx);
+    fn apply_attrs_to_llfn(
+        &self,
+        idx: AttributePlace,
+        cx: &CodegenCx<'_, '_>,
+        llfn: &Value,
+        is_foreign_fn: bool,
+        force_set_align: bool,
+    ) {
+        let attrs = get_attrs(self, cx, is_foreign_fn, force_set_align);
         attributes::apply_to_llfn(llfn, idx, &attrs);
     }
 
@@ -107,7 +146,10 @@ impl ArgAttributesExt for ArgAttributes {
         cx: &CodegenCx<'_, '_>,
         callsite: &Value,
     ) {
-        let attrs = get_attrs(self, cx);
+        // It's not a declaration at all, so in particular not a foreign function declaration.
+        // We also don't need to force setting the alignment.
+        let attrs =
+            get_attrs(self, cx, /*is_foreign_fn_decl*/ false, /*force_set_align*/ false);
         attributes::apply_to_callsite(callsite, idx, &attrs);
     }
 }
@@ -310,7 +352,7 @@ pub trait FnAbiLlvmExt<'ll, 'tcx> {
     fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type;
     fn ptr_to_llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type;
     fn llvm_cconv(&self) -> llvm::CallConv;
-    fn apply_attrs_llfn(&self, cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value);
+    fn apply_attrs_llfn(&self, cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, is_foreign_fn: bool);
     fn apply_attrs_callsite(&self, bx: &mut Builder<'_, 'll, 'tcx>, callsite: &'ll Value);
 }
 
@@ -396,32 +438,46 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
         self.conv.into()
     }
 
-    fn apply_attrs_llfn(&self, cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value) {
+    fn apply_attrs_llfn(&self, cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, is_foreign_fn: bool) {
         let mut func_attrs = SmallVec::<[_; 3]>::new();
-        if self.ret.layout.abi.is_uninhabited() {
-            func_attrs.push(llvm::AttributeKind::NoReturn.create_attr(cx.llcx));
-        }
-        if !self.can_unwind {
-            func_attrs.push(llvm::AttributeKind::NoUnwind.create_attr(cx.llcx));
+        if !is_foreign_fn {
+            if self.ret.layout.abi.is_uninhabited() {
+                func_attrs.push(llvm::AttributeKind::NoReturn.create_attr(cx.llcx));
+            }
+            if !self.can_unwind {
+                func_attrs.push(llvm::AttributeKind::NoUnwind.create_attr(cx.llcx));
+            }
         }
         if let Conv::RiscvInterrupt { kind } = self.conv {
             func_attrs.push(llvm::CreateAttrStringValue(cx.llcx, "interrupt", kind.as_str()));
         }
         attributes::apply_to_llfn(llfn, llvm::AttributePlace::Function, &{ func_attrs });
 
         let mut i = 0;
-        let mut apply = |attrs: &ArgAttributes| {
-            attrs.apply_attrs_to_llfn(llvm::AttributePlace::Argument(i), cx, llfn);
+        let mut apply_next_arg = |attrs: &ArgAttributes, force_set_align: bool| {
+            attrs.apply_attrs_to_llfn(
+                llvm::AttributePlace::Argument(i),
+                cx,
+                llfn,
+                is_foreign_fn,
+                force_set_align,
+            );
             i += 1;
             i - 1
         };
         match &self.ret.mode {
             PassMode::Direct(attrs) => {
-                attrs.apply_attrs_to_llfn(llvm::AttributePlace::ReturnValue, cx, llfn);
+                attrs.apply_attrs_to_llfn(
+                    llvm::AttributePlace::ReturnValue,
+                    cx,
+                    llfn,
+                    is_foreign_fn,
+                    /*force_set_align*/ false,
+                );
             }
             PassMode::Indirect { attrs, meta_attrs: _, on_stack } => {
                 assert!(!on_stack);
-                let i = apply(attrs);
+                let i = apply_next_arg(attrs, /*force_set_align*/ false);
                 let sret = llvm::CreateStructRetAttr(
                     cx.llcx,
                     cx.type_array(cx.type_i8(), self.ret.layout.size.bytes()),
@@ -441,15 +497,21 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
                 }
             }
             PassMode::Cast { cast, pad_i32: _ } => {
-                cast.attrs.apply_attrs_to_llfn(llvm::AttributePlace::ReturnValue, cx, llfn);
+                cast.attrs.apply_attrs_to_llfn(
+                    llvm::AttributePlace::ReturnValue,
+                    cx,
+                    llfn,
+                    is_foreign_fn,
+                    /*force_set_align*/ false,
+                );
             }
             _ => {}
         }
         for arg in self.args.iter() {
             match &arg.mode {
                 PassMode::Ignore => {}
                 PassMode::Indirect { attrs, meta_attrs: None, on_stack: true } => {
-                    let i = apply(attrs);
+                    let i = apply_next_arg(attrs, /*force_set_align*/ true);
                     let byval = llvm::CreateByValAttr(
                         cx.llcx,
                         cx.type_array(cx.type_i8(), arg.layout.size.bytes()),
@@ -458,22 +520,22 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
                 }
                 PassMode::Direct(attrs)
                 | PassMode::Indirect { attrs, meta_attrs: None, on_stack: false } => {
-                    apply(attrs);
+                    apply_next_arg(attrs, /*force_set_align*/ false);
                 }
                 PassMode::Indirect { attrs, meta_attrs: Some(meta_attrs), on_stack } => {
                     assert!(!on_stack);
-                    apply(attrs);
-                    apply(meta_attrs);
+                    apply_next_arg(attrs, /*force_set_align*/ false);
+                    apply_next_arg(meta_attrs, /*force_set_align*/ false);
                 }
                 PassMode::Pair(a, b) => {
-                    apply(a);
-                    apply(b);
+                    apply_next_arg(a, /*force_set_align*/ false);
+                    apply_next_arg(b, /*force_set_align*/ false);
                 }
                 PassMode::Cast { cast, pad_i32 } => {
                     if *pad_i32 {
-                        apply(&ArgAttributes::new());
+                        apply_next_arg(&ArgAttributes::new(), /*force_set_align*/ false);
                     }
-                    apply(&cast.attrs);
+                    apply_next_arg(&cast.attrs, /*force_set_align*/ false);
                 }
             }
         }
diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs
@@ -137,7 +137,12 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
             llvm::Visibility::Default,
             fn_abi.llvm_type(self),
         );
-        fn_abi.apply_attrs_llfn(self, llfn);
+        // If this is for a foreign instance, do *not* add the attributes.
+        // There will be no body attached to this, and having attributes attached to a declaration
+        // means that if there are multiple declarations in different modules, one of them will win
+        // and be used for *all* modules, even though it may never be called!
+        let is_foreign_fn = instance.is_some_and(|i| self.tcx.is_foreign_item(i.def_id()));
+        fn_abi.apply_attrs_llfn(self, llfn, is_foreign_fn);
 
         if self.tcx.sess.is_sanitizer_cfi_enabled() {
             if let Some(instance) = instance {
diff --git a/tests/codegen/align-byval.rs b/tests/codegen/align-byval.rs
@@ -223,7 +223,6 @@ extern "C" {
 
     // x86_64-windows: declare void @natural_align_2(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 2{{.*}})
 
     // i686-linux: declare void @natural_align_2({{.*}}byval([34 x i8]) align 4{{.*}})
 
@@ -238,7 +237,6 @@ extern "C" {
 
     // x86_64-windows: declare void @force_align_4(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 4{{.*}})
 
     // i686-linux: declare void @force_align_4({{.*}}byval([20 x i8]) align 4{{.*}})
 
@@ -253,7 +251,6 @@ extern "C" {
 
     // x86_64-windows: declare void @natural_align_8(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 8{{.*}})
 
     // i686-linux: declare void @natural_align_8({{.*}}byval([24 x i8]) align 4{{.*}})
 
@@ -268,13 +265,11 @@ extern "C" {
 
     // x86_64-windows: declare void @force_align_8(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 8{{.*}})
 
     // i686-linux: declare void @force_align_8({{.*}}byval([24 x i8]) align 4{{.*}})
 
     // i686-windows: declare void @force_align_8(
     // i686-windows-NOT: byval
-    // i686-windows-SAME: align 8{{.*}})
     fn force_align_8(x: ForceAlign8);
 
     // m68k: declare void @lower_fa8({{.*}}byval([24 x i8]) align 4{{.*}})
@@ -285,7 +280,6 @@ extern "C" {
 
     // x86_64-windows: declare void @lower_fa8(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 8{{.*}})
 
     // i686-linux: declare void @lower_fa8({{.*}}byval([24 x i8]) align 4{{.*}})
 
@@ -300,13 +294,11 @@ extern "C" {
 
     // x86_64-windows: declare void @wrapped_fa8(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 8{{.*}})
 
     // i686-linux: declare void @wrapped_fa8({{.*}}byval([24 x i8]) align 4{{.*}})
 
     // i686-windows: declare void @wrapped_fa8(
     // i686-windows-NOT: byval
-    // i686-windows-SAME: align 8{{.*}})
     fn wrapped_fa8(x: WrappedFA8);
 
     // m68k: declare void @transparent_fa8({{.*}}byval([24 x i8]) align 8{{.*}})
@@ -317,13 +309,11 @@ extern "C" {
 
     // x86_64-windows: declare void @transparent_fa8(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 8{{.*}})
 
     // i686-linux: declare void @transparent_fa8({{.*}}byval([24 x i8]) align 4{{.*}})
 
     // i686-windows: declare void @transparent_fa8(
     // i686-windows-NOT: byval
-    // i686-windows-SAME: align 8{{.*}})
     fn transparent_fa8(x: TransparentFA8);
 
     // m68k: declare void @force_align_16({{.*}}byval([80 x i8]) align 16{{.*}})
@@ -334,12 +324,10 @@ extern "C" {
 
     // x86_64-windows: declare void @force_align_16(
     // x86_64-windows-NOT: byval
-    // x86_64-windows-SAME: align 16{{.*}})
 
     // i686-linux: declare void @force_align_16({{.*}}byval([80 x i8]) align 4{{.*}})
 
     // i686-windows: declare void @force_align_16(
     // i686-windows-NOT: byval
-    // i686-windows-SAME: align 16{{.*}})
     fn force_align_16(x: ForceAlign16);
 }
diff --git a/tests/codegen/box-uninit-bytes.rs b/tests/codegen/box-uninit-bytes.rs
@@ -41,6 +41,6 @@ pub fn box_lotsa_padding() -> Box<LotsaPadding> {
 
 // Hide the `allocalign` attribute in the declaration of __rust_alloc
 // from the CHECK-NOT above, and also verify the attributes got set reasonably.
-// CHECK: declare {{(dso_local )?}}noalias noundef ptr @__rust_alloc(i{{[0-9]+}} noundef, i{{[0-9]+}} allocalign noundef) unnamed_addr [[RUST_ALLOC_ATTRS:#[0-9]+]]
+// CHECK: declare {{(dso_local )?}}noalias ptr @__rust_alloc(i{{[0-9]+}}, i{{[0-9]+}} allocalign) unnamed_addr [[RUST_ALLOC_ATTRS:#[0-9]+]]
 
 // CHECK-DAG: attributes [[RUST_ALLOC_ATTRS]] = { {{.*}} allockind("alloc,uninitialized,aligned") allocsize(0) {{(uwtable )?}}"alloc-family"="__rust_alloc" {{.*}} }
diff --git a/tests/codegen/declare-no-attrs.rs b/tests/codegen/declare-no-attrs.rs
@@ -0,0 +1,21 @@
+//@ compile-flags: -C opt-level=1
+
+#![no_builtins]
+#![crate_type = "lib"]
+
+// Make sure that there are no attributes attached to this delcaration.
+// If there were, they could be used even for other call sites using a different
+// item importing the same function!
+// See <https://github.com/rust-lang/rust/issues/46188> for details.
+#[allow(improper_ctypes)]
+extern "C" {
+    // CHECK: declare ptr @malloc(i64)
+    pub fn malloc(x: u64) -> &'static mut ();
+}
+
+// `malloc` needs to be referenced or else it will disappear entirely.
+#[no_mangle]
+pub fn use_it() -> usize {
+    let m: unsafe extern "C" fn(_) -> _ = malloc;
+    malloc as *const () as usize
+}
diff --git a/tests/codegen/no_builtins-at-crate.rs b/tests/codegen/no_builtins-at-crate.rs
@@ -15,10 +15,12 @@ pub unsafe extern "C" fn __aeabi_memcpy(dest: *mut u8, src: *const u8, size: usi
 
 // CHECK: declare
 // CHECK-SAME: @memcpy
-// CHECK-SAME: #0
+// CHECK-SAME: #1
 extern "C" {
     pub fn memcpy(dest: *mut u8, src: *const u8, n: usize) -> *mut u8;
 }
 
 // CHECK: attributes #0
 // CHECK-SAME: "no-builtins"
+// CHECK: attributes #1
+// CHECK-SAME: "no-builtins"
diff --git a/tests/codegen/unwind-extern-imports.rs b/tests/codegen/unwind-extern-imports.rs
@@ -4,18 +4,25 @@
 #![crate_type = "lib"]
 
 extern "C" {
-    // CHECK: Function Attrs:{{.*}}nounwind
-    // CHECK-NEXT: declare{{.*}}void @extern_fn
     fn extern_fn();
 }
 
 extern "C-unwind" {
-    // CHECK-NOT: nounwind
-    // CHECK: declare{{.*}}void @c_unwind_extern_fn
     fn c_unwind_extern_fn();
 }
 
+// The attributes are at the call sites, not the declaration.
+
+// CHECK-LABEL: @force_declare
+#[no_mangle]
 pub unsafe fn force_declare() {
+    // Attributes with `nounwind` here (also see check below).
+    // CHECK: call void @extern_fn() #1
     extern_fn();
+    // No attributes here.
+    // CHECK: call void @c_unwind_extern_fn()
     c_unwind_extern_fn();
 }
+
+// CHECK: attributes #1
+// CHECK-SAME: nounwind
diff --git a/tests/codegen/vec-calloc.rs b/tests/codegen/vec-calloc.rs
