WIP: declare imported functions as globals without a type, and apply more attributes at the call site

RalfJung · RalfJung · commit da5ac2677df8 · 2024-08-05T12:14:35.000+02:00
diff --git a/compiler/rustc_ast_passes/src/feature_gate.rs b/compiler/rustc_ast_passes/src/feature_gate.rs
@@ -325,7 +325,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/attributes.rs b/compiler/rustc_codegen_llvm/src/attributes.rs
@@ -328,7 +328,7 @@ fn create_alloc_family_attr(llcx: &llvm::Context) -> &llvm::Attribute {
 
 /// Composite function which sets LLVM attributes for function depending on its AST (`#[attribute]`)
 /// attributes.
-pub fn from_fn_attrs<'ll, 'tcx>(
+pub fn llfn_attrs_from_fn<'ll, 'tcx>(
     cx: &CodegenCx<'ll, 'tcx>,
     llfn: &'ll Value,
     instance: ty::Instance<'tcx>,
@@ -410,48 +410,6 @@ pub fn from_fn_attrs<'ll, 'tcx>(
         // Need this for AArch64.
         to_add.push(llvm::CreateAttrStringValue(cx.llcx, "branch-target-enforcement", "false"));
     }
-    if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR)
-        || codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR_ZEROED)
-    {
-        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]);
-        to_add.push(llvm::CreateAllocSizeAttr(cx.llcx, 0));
-        let mut flags = AllocKindFlags::Alloc | AllocKindFlags::Aligned;
-        if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR) {
-            flags |= AllocKindFlags::Uninitialized;
-        } else {
-            flags |= AllocKindFlags::Zeroed;
-        }
-        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]);
-    }
-    if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::REALLOCATOR) {
-        to_add.push(create_alloc_family_attr(cx.llcx));
-        to_add.push(llvm::CreateAllocKindAttr(
-            cx.llcx,
-            AllocKindFlags::Realloc | AllocKindFlags::Aligned,
-        ));
-        // 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 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]);
-        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]);
-    }
-    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]);
-    }
     if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::CMSE_NONSECURE_ENTRY) {
         to_add.push(llvm::CreateAttrString(cx.llcx, "cmse_nonsecure_entry"));
     }
@@ -530,6 +488,64 @@ pub fn from_fn_attrs<'ll, 'tcx>(
     attributes::apply_to_llfn(llfn, Function, &to_add);
 }
 
+pub fn callsite_attrs_from_fn<'ll, 'tcx>(
+    cx: &CodegenCx<'ll, 'tcx>,
+    callsite: &'ll Value,
+    instance: ty::Instance<'tcx>,
+) {
+    let codegen_fn_attrs = cx.tcx.codegen_fn_attrs(instance.def_id());
+
+    let mut to_add = SmallVec::<[_; 16]>::new();
+
+    if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR)
+        || codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::ALLOCATOR_ZEROED)
+    {
+        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_callsite(callsite, 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) {
+            flags |= AllocKindFlags::Uninitialized;
+        } else {
+            flags |= AllocKindFlags::Zeroed;
+        }
+        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_callsite(callsite, AttributePlace::ReturnValue, &[no_alias]);
+    }
+    if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::REALLOCATOR) {
+        to_add.push(create_alloc_family_attr(cx.llcx));
+        to_add.push(llvm::CreateAllocKindAttr(
+            cx.llcx,
+            AllocKindFlags::Realloc | AllocKindFlags::Aligned,
+        ));
+        // applies to argument place instead of function place
+        let allocated_pointer = AttributeKind::AllocatedPointer.create_attr(cx.llcx);
+        //attributes::apply_to_callsite(callsite, AttributePlace::Argument(0), &[allocated_pointer]);
+        // apply to argument place instead of function
+        let alloc_align = AttributeKind::AllocAlign.create_attr(cx.llcx);
+        //attributes::apply_to_callsite(callsite, 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_callsite(callsite, 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_callsite(callsite, AttributePlace::Argument(0), &[allocated_pointer]);
+    }
+
+    if !to_add.is_empty() {
+        eprintln!("applying attributes to call of {instance:?}");
+    }
+    attributes::apply_to_callsite(callsite, Function, &to_add);
+}
+
 fn wasm_import_module(tcx: TyCtxt<'_>, id: DefId) -> Option<&String> {
     tcx.wasm_import_module_map(id.krate).get(&id)
 }
diff --git a/compiler/rustc_codegen_llvm/src/builder.rs b/compiler/rustc_codegen_llvm/src/builder.rs
@@ -262,6 +262,9 @@ 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);
         }
+        if let Some(instance) = instance {
+            attributes::callsite_attrs_from_fn(self.cx, invoke, instance);
+        }
         invoke
     }
 
@@ -1287,6 +1290,9 @@ 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);
         }
+        if let Some(instance) = instance {
+            attributes::callsite_attrs_from_fn(self.cx, call, instance);
+        }
         call
     }
 
@@ -1615,6 +1621,9 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
         if let Some(fn_abi) = fn_abi {
             fn_abi.apply_attrs_callsite(self, callbr);
         }
+        if let Some(instance) = instance {
+            attributes::callsite_attrs_from_fn(self.cx, callbr, instance);
+        }
         callbr
     }
 
diff --git a/compiler/rustc_codegen_llvm/src/callee.rs b/compiler/rustc_codegen_llvm/src/callee.rs
@@ -4,12 +4,13 @@
 //! and methods are represented as just a fn ptr and not a full
 //! closure.
 
-use crate::attributes;
 use crate::common;
 use crate::context::CodegenCx;
+use crate::declare::declare_as_global;
 use crate::llvm;
 use crate::value::Value;
 
+use rustc_codegen_ssa::traits::BaseTypeMethods;
 use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt};
 use rustc_middle::ty::{self, Instance, TypeVisitableExt};
 use tracing::debug;
@@ -47,40 +48,51 @@ pub fn get_fn<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>, instance: Instance<'tcx>) ->
         llfn
     } else {
         let instance_def_id = instance.def_id();
-        let llfn = if tcx.sess.target.arch == "x86"
-            && let Some(dllimport) = common::get_dllimport(tcx, instance_def_id, sym)
-        {
-            // Fix for https://github.com/rust-lang/rust/issues/104453
-            // On x86 Windows, LLVM uses 'L' as the prefix for any private
-            // global symbols, so when we create an undecorated function symbol
-            // that begins with an 'L' LLVM misinterprets that as a private
-            // global symbol that it created and so fails the compilation at a
-            // later stage since such a symbol must have a definition.
-            //
-            // To avoid this, we set the Storage Class to "DllImport" so that
-            // LLVM will prefix the name with `__imp_`. Ideally, we'd like the
-            // existing logic below to set the Storage Class, but it has an
-            // exemption for MinGW for backwards compatibility.
-            let llfn = cx.declare_fn(
-                &common::i686_decorated_name(
-                    dllimport,
-                    common::is_mingw_gnu_toolchain(&tcx.sess.target),
-                    true,
-                ),
-                fn_abi,
-                Some(instance),
-            );
-            unsafe {
-                llvm::LLVMSetDLLStorageClass(llfn, llvm::DLLStorageClass::DllImport);
+        let llfn = if declare_as_global(tcx, instance, sym) {
+            // If this is a foreign function, make the declaration not promise *anything*.
+            // The same foreign function could be declared multiple times in the same compilation
+            // unit, and the last declaration will overwrite the previous ones, leading
+            // to "action at a distance" -- including UB, if the last declaration adds attributes
+            // that not all call sites are prepared for.
+
+            // Declare this as an empty array of `i8`.
+            let ty = cx.type_array(cx.type_i8(), 0);
+
+            if tcx.sess.target.arch == "x86"
+                && let Some(dllimport) = common::get_dllimport(tcx, instance_def_id, sym)
+            {
+                // Fix for https://github.com/rust-lang/rust/issues/104453
+                // On x86 Windows, LLVM uses 'L' as the prefix for any private
+                // global symbols, so when we create an undecorated function symbol
+                // that begins with an 'L' LLVM misinterprets that as a private
+                // global symbol that it created and so fails the compilation at a
+                // later stage since such a symbol must have a definition.
+                //
+                // To avoid this, we set the Storage Class to "DllImport" so that
+                // LLVM will prefix the name with `__imp_`. Ideally, we'd like the
+                // existing logic below to set the Storage Class, but it has an
+                // exemption for MinGW for backwards compatibility.
+                let llsym = cx.declare_global(
+                    &common::i686_decorated_name(
+                        dllimport,
+                        common::is_mingw_gnu_toolchain(&tcx.sess.target),
+                        true,
+                    ),
+                    ty,
+                );
+                unsafe {
+                    llvm::LLVMSetDLLStorageClass(llsym, llvm::DLLStorageClass::DllImport);
+                }
+                llsym
+            } else {
+                cx.declare_global(sym, ty)
             }
-            llfn
         } else {
-            cx.declare_fn(sym, fn_abi, Some(instance))
+            let llfn = cx.declare_fn(sym, fn_abi, Some(instance));
+            llfn
         };
         debug!("get_fn: not casting pointer!");
 
-        attributes::from_fn_attrs(cx, llfn, instance);
-
         // Apply an appropriate linkage/visibility value to our item that we
         // just declared.
         //
diff --git a/compiler/rustc_codegen_llvm/src/declare.rs b/compiler/rustc_codegen_llvm/src/declare.rs
@@ -21,7 +21,7 @@ use crate::value::Value;
 use itertools::Itertools;
 use rustc_codegen_ssa::traits::TypeMembershipMethods;
 use rustc_data_structures::fx::FxIndexSet;
-use rustc_middle::ty::{Instance, Ty};
+use rustc_middle::ty::{Instance, Ty, TyCtxt};
 use rustc_sanitizers::{cfi, kcfi};
 use smallvec::SmallVec;
 use tracing::debug;
@@ -60,6 +60,18 @@ fn declare_raw_fn<'ll>(
     llfn
 }
 
+/// Some functions are not declared as functions but as global names without any meaningful type.
+/// This is to deal with the fact that Rust can import the same external function with different
+/// signatures into the same compilation unit, but LLVM cannot.
+pub fn declare_as_global<'tcx>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, name: &str) -> bool {
+    tcx.is_foreign_item(instance.def_id()) && {
+        // If this is calling an LLVM intrinsic, we can't use the `global` trick.
+        // These are also unstable to call so nobody but us can screw up their signature.
+        let is_llvm_intrinsic = name.starts_with("llvm.");
+        !is_llvm_intrinsic
+    }
+}
+
 impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     /// Declare a global value.
     ///
@@ -127,6 +139,15 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
     ) -> &'ll Value {
         debug!("declare_rust_fn(name={:?}, fn_abi={:?})", name, fn_abi);
 
+        // If this is a foreign function, it should have been declared in a different way.
+        // We can't declare these as "functions" because they can be imported multiple times
+        // with different signatures, and any signature we tell LLVM might be wrong for
+        // call sites that use another signature.
+        assert!(
+            !instance.is_some_and(|i| declare_as_global(self.tcx, i, name)),
+            "{name} is a foreign function, it should be declared as a global",
+        );
+
         // Function addresses in Rust are never significant, allowing functions to
         // be merged.
         let llfn = declare_raw_fn(
@@ -138,6 +159,9 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
             fn_abi.llvm_type(self),
         );
         fn_abi.apply_attrs_llfn(self, llfn);
+        if let Some(instance) = instance {
+            attributes::llfn_attrs_from_fn(self, llfn, instance);
+        }
 
         if self.tcx.sess.is_sanitizer_cfi_enabled() {
             if let Some(instance) = instance {
diff --git a/compiler/rustc_codegen_llvm/src/mono_item.rs b/compiler/rustc_codegen_llvm/src/mono_item.rs
@@ -1,4 +1,3 @@
-use crate::attributes;
 use crate::base;
 use crate::context::CodegenCx;
 use crate::errors::SymbolAlreadyDefined;
@@ -60,6 +59,21 @@ impl<'tcx> PreDefineMethods<'tcx> for CodegenCx<'_, 'tcx> {
     ) {
         assert!(!instance.args.has_infer());
 
+        // If this is a foreign function, make the declaration not promise *anything*.
+        // The same foreign function could be declared multiple times in the same compilation
+        // unit, and the last declaration will overwrite the previous ones, leading
+        // to "action at a distance" -- including UB, if the last declaration adds attributes
+        // that not all call sites are prepared for.
+        let is_foreign_fn = self.tcx.is_foreign_item(instance.def_id());
+        if is_foreign_fn {
+            eprintln!("special treatment for extern fn {}", symbol_name);
+            // Declare this as an empty array of `i8`.
+            let ty = self.type_array(self.type_i8(), 0);
+            let lldecl = self.declare_global(symbol_name, ty);
+            self.instances.borrow_mut().insert(instance, lldecl);
+            return;
+        }
+
         let fn_abi = self.fn_abi_of_instance(instance, ty::List::empty());
         let lldecl = self.declare_fn(symbol_name, fn_abi, Some(instance));
         unsafe { llvm::LLVMRustSetLinkage(lldecl, base::linkage_to_llvm(linkage)) };
@@ -88,8 +102,6 @@ impl<'tcx> PreDefineMethods<'tcx> for CodegenCx<'_, 'tcx> {
 
         debug!("predefine_fn: instance = {:?}", instance);
 
-        attributes::from_fn_attrs(self, lldecl, instance);
-
         unsafe {
             if self.should_assume_dso_local(lldecl, false) {
                 llvm::LLVMRustSetDSOLocal(lldecl, true);
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 or even types 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: @malloc = external global [0 x i8]
+    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/pic-relocation-model.rs b/tests/codegen/pic-relocation-model.rs
@@ -2,18 +2,15 @@
 
 #![crate_type = "rlib"]
 
+// CHECK: @foreign_fn = external global
+extern "C" {
+    fn foreign_fn() -> u8;
+}
+
 // CHECK: define i8 @call_foreign_fn()
 #[no_mangle]
 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()
-extern "C" {
-    fn foreign_fn() -> u8;
-}
-
 // CHECK: !{i32 {{[78]}}, !"PIC Level", i32 2}
diff --git a/tests/codegen/pie-relocation-model.rs b/tests/codegen/pie-relocation-model.rs

Original file line number	Diff line number	Diff line change
`@@ -325,7 +325,7 @@ impl<'a> Visitor<'a> for PostExpansionVisitor<'a> {`
`325`	`325`	`&self,`
`326`	`326`	`link_llvm_intrinsics,`
`327`	`327`	`i.span,`
`328`		`- "linking to LLVM intrinsics is experimental"`
	`328`	`+ "linking to LLVM intrinsics is an internal feature reserved for the standard library"`
`329`	`329`	`);`
`330`	`330`	`}`
`331`	`331`	`}`
Original file line number	Diff line number	Diff line change
`@@ -262,6 +262,9 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {`
`262`	`262`	`if let Some(fn_abi) = fn_abi {`
`263`	`263`	`fn_abi.apply_attrs_callsite(self, invoke);`
`264`	`264`	`}`
	`265`	`+ if let Some(instance) = instance {`
	`266`	`+ attributes::callsite_attrs_from_fn(self.cx, invoke, instance);`
	`267`	`+ }`
`265`	`268`	`invoke`
`266`	`269`	`}`
`267`	`270`
`@@ -1287,6 +1290,9 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {`
`1287`	`1290`	`if let Some(fn_abi) = fn_abi {`
`1288`	`1291`	`fn_abi.apply_attrs_callsite(self, call);`
`1289`	`1292`	`}`
	`1293`	`+ if let Some(instance) = instance {`
	`1294`	`+ attributes::callsite_attrs_from_fn(self.cx, call, instance);`
	`1295`	`+ }`
`1290`	`1296`	`call`
`1291`	`1297`	`}`
`1292`	`1298`
`@@ -1615,6 +1621,9 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {`
`1615`	`1621`	`if let Some(fn_abi) = fn_abi {`
`1616`	`1622`	`fn_abi.apply_attrs_callsite(self, callbr);`
`1617`	`1623`	`}`
	`1624`	`+ if let Some(instance) = instance {`
	`1625`	`+ attributes::callsite_attrs_from_fn(self.cx, callbr, instance);`
	`1626`	`+ }`
`1618`	`1627`	`callbr`
`1619`	`1628`	`}`
`1620`	`1629`