Use function shims to make sure EII works on apple targets

compiler/rustc_codegen_llvm/src/llvm/mod.rs

@@ -284,6 +284,10 @@ pub(crate) fn set_comdat(llmod: &Module, llglobal: &Value, name: &CStr) {
     }
 }
 
+pub(crate) fn count_params(llfn: &Value) -> c_uint {
+    LLVMCountParams(llfn)
+}
+
 /// Safe wrapper around `LLVMGetParam`, because segfaults are no fun.
 pub(crate) fn get_param(llfn: &Value, index: c_uint) -> &Value {
     unsafe {

compiler/rustc_codegen_llvm/src/mono_item.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::ffi::CString;
 
 use rustc_abi::AddressSpace;
@@ -6,13 +7,17 @@ use rustc_hir::attrs::Linkage;
 use rustc_hir::def::DefKind;
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
 use rustc_middle::bug;
+use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
 use rustc_middle::mir::mono::Visibility;
 use rustc_middle::ty::layout::{FnAbiOf, HasTypingEnv, LayoutOf};
-use rustc_middle::ty::{self, Instance, TypeVisitableExt};
+use rustc_middle::ty::{self, Instance, Ty, TypeVisitableExt};
 use rustc_session::config::CrateType;
+use rustc_target::callconv::FnAbi;
 use rustc_target::spec::{Arch, RelocModel};
 use tracing::debug;
 
+use crate::abi::FnAbiLlvmExt;
+use crate::builder::Builder;
 use crate::context::CodegenCx;
 use crate::errors::SymbolAlreadyDefined;
 use crate::type_of::LayoutLlvmExt;
@@ -45,7 +50,7 @@ impl<'tcx> PreDefineCodegenMethods<'tcx> for CodegenCx<'_, 'tcx> {
         self.assume_dso_local(g, false);
 
         let attrs = self.tcx.codegen_instance_attrs(instance.def);
-        self.add_aliases(g, &attrs.foreign_item_symbol_aliases);
+        self.add_static_aliases(g, &attrs.foreign_item_symbol_aliases);
 
         self.instances.borrow_mut().insert(instance, g);
     }
@@ -59,11 +64,29 @@ impl<'tcx> PreDefineCodegenMethods<'tcx> for CodegenCx<'_, 'tcx> {
     ) {
         assert!(!instance.args.has_infer());
 
-        let fn_abi = self.fn_abi_of_instance(instance, ty::List::empty());
+        let attrs = self.tcx.codegen_instance_attrs(instance.def);
+
+        let lldecl =
+            self.predefine_without_aliases(instance, &attrs, linkage, visibility, symbol_name);
+        self.add_function_aliases(instance, lldecl, &attrs, &attrs.foreign_item_symbol_aliases);
+
+        self.instances.borrow_mut().insert(instance, lldecl);
+    }
+}
+
+impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
+    fn predefine_without_aliases(
+        &self,
+        instance: Instance<'tcx>,
+        attrs: &Cow<'_, CodegenFnAttrs>,
+        linkage: Linkage,
+        visibility: Visibility,
+        symbol_name: &str,
+    ) -> &'ll llvm::Value {
+        let fn_abi: &FnAbi<'tcx, Ty<'tcx>> = self.fn_abi_of_instance(instance, ty::List::empty());
         let lldecl = self.declare_fn(symbol_name, fn_abi, Some(instance));
         llvm::set_linkage(lldecl, base::linkage_to_llvm(linkage));
-        let attrs = self.tcx.codegen_instance_attrs(instance.def);
-        base::set_link_section(lldecl, &attrs);
+        base::set_link_section(lldecl, attrs);
         if (linkage == Linkage::LinkOnceODR || linkage == Linkage::WeakODR)
             && self.tcx.sess.target.supports_comdat()
         {
@@ -84,20 +107,45 @@ impl<'tcx> PreDefineCodegenMethods<'tcx> for CodegenCx<'_, 'tcx> {
 
         self.assume_dso_local(lldecl, false);
 
-        self.add_aliases(lldecl, &attrs.foreign_item_symbol_aliases);
-
-        self.instances.borrow_mut().insert(instance, lldecl);
+        lldecl
     }
-}
 
-impl CodegenCx<'_, '_> {
-    fn add_aliases(&self, aliasee: &llvm::Value, aliases: &[(DefId, Linkage, Visibility)]) {
+    /// LLVM has the concept of an `alias`.
+    /// We need this for the "externally implementable items" feature,
+    /// though it's generally useful.
+    ///
+    /// On macos, though this might be a more general problem, function symbols
+    /// have a fixed target architecture. This is necessary, since macos binaries
+    /// may contain code for both ARM and x86 macs.
+    ///
+    /// LLVM *can* add attributes for target architecture to function symbols,
+    /// cannot do so for statics, but importantly, also cannot for aliases
+    /// *even* when aliases may refer to a function symbol.
+    ///
+    /// This is not a problem: instead of using LLVM aliases, we can just generate
+    /// a new function symbol (with target architecture!) which effectively comes down to:
+    ///
+    /// ```ignore (illustrative example)
+    /// fn alias_name(...args) {
+    ///     original_name(...args)
+    /// }
+    /// ```
+    ///
+    /// That's also an alias.
+    ///
+    /// This does mean that the alias symbol has a different address than the original symbol
+    /// (assuming no optimizations by LLVM occur). This is unacceptable for statics.
+    /// So for statics we do want to use LLVM aliases, which is fine,
+    /// since for those we don't care about target architecture anyway.
+    ///
+    /// So, this function is for static aliases. See [`add_function_aliases`](Self::add_function_aliases) for the alternative.
+    fn add_static_aliases(&self, aliasee: &llvm::Value, aliases: &[(DefId, Linkage, Visibility)]) {
         let ty = self.get_type_of_global(aliasee);
 
         for (alias, linkage, visibility) in aliases {
             let symbol_name = self.tcx.symbol_name(Instance::mono(self.tcx, *alias));
+            tracing::debug!("STATIC ALIAS: {alias:?} {linkage:?} {visibility:?}");
 
-            tracing::debug!("ALIAS: {alias:?} {linkage:?} {visibility:?}");
             let lldecl = llvm::add_alias(
                 self.llmod,
                 ty,
@@ -111,6 +159,54 @@ impl CodegenCx<'_, '_> {
         }
     }
 
+    /// See [`add_static_aliases`](Self::add_static_aliases) for docs.
+    fn add_function_aliases(
+        &self,
+        aliasee_instance: Instance<'tcx>,
+        aliasee: &'ll llvm::Value,
+        attrs: &Cow<'_, CodegenFnAttrs>,
+        aliases: &[(DefId, Linkage, Visibility)],
+    ) {
+        for (alias, linkage, visibility) in aliases {
+            let symbol_name = self.tcx.symbol_name(Instance::mono(self.tcx, *alias));
+            tracing::debug!("FUNCTION ALIAS: {alias:?} {linkage:?} {visibility:?}");
+
+            // predefine another copy of the original instance
+            // with a new symbol name
+            let alias_lldecl = self.predefine_without_aliases(
+                aliasee_instance,
+                attrs,
+                *linkage,
+                *visibility,
+                symbol_name.name,
+            );
+
+            let fn_abi: &FnAbi<'tcx, Ty<'tcx>> =
+                self.fn_abi_of_instance(aliasee_instance, ty::List::empty());
+
+            // both the alias and the aliasee have the same ty
+            let fn_ty = fn_abi.llvm_type(self);
+            let start_llbb = Builder::append_block(self, alias_lldecl, "start");
+            let mut start_bx = Builder::build(self, start_llbb);
+
+            let num_params = llvm::count_params(alias_lldecl);
+            let mut args = Vec::with_capacity(num_params as usize);
+            for index in 0..num_params {
+                args.push(llvm::get_param(alias_lldecl, index));
+            }
+
+            start_bx.tail_call(
+                fn_ty,
+                Some(attrs),
+                fn_abi,
+                aliasee,
+                &args,
+                None,
+                Some(aliasee_instance),
+            );
+        }
+    }
+
     /// A definition or declaration can be assumed to be local to a group of
     /// libraries that form a single DSO or executable.
     /// Marks the local as DSO if so.

tests/ui/eii/default/auxiliary/decl_with_default_panics.rs

@@ -0,0 +1,10 @@
+//@ no-prefer-dynamic
+//@ needs-unwind
+//@ exec-env:RUST_BACKTRACE=1
+#![crate_type = "rlib"]
+#![feature(extern_item_impls)]
+
+#[eii(eii1)]
+pub fn decl1(x: u64) {
+    panic!("{}", x);
+}

tests/ui/eii/default/call_default.rs

@@ -5,16 +5,6 @@
 //@ ignore-backends: gcc
 // FIXME: linking on windows (speciifcally mingw) not yet supported, see tracking issue #125418
 //@ ignore-windows
-// Functions can have target-cpu applied. On apple-darwin this is super important,
-// since you can have binaries which mix x86 and aarch64 code that are compatible
-// with both architectures. So we can't just reject target_cpu on EIIs since apple
-// puts them on by default. The problem: we generate aliases. And aliases cannot
-// get target_cpu applied to them. So, instead we should, in the case of functions,
-// generate a shim function. For statics aliases should keep working in theory.
-// In fact, aliases are only necessary for statics. For functions we could just
-//  always generate a shim and a previous version of EII did so but I was sad
-// that that'd never support statics.
-//@ ignore-macos
 // Tests EIIs with default implementations.
 // When there's no explicit declaration, the default should be called from the declaring crate.
 #![feature(extern_item_impls)]

tests/ui/eii/default/call_default_panics.rs

@@ -0,0 +1,35 @@
+//@ no-prefer-dynamic
+//@ aux-build: decl_with_default_panics.rs
+//@ edition: 2021
+//@ run-pass
+//@ needs-unwind
+//@ exec-env:RUST_BACKTRACE=1
+//@ ignore-backends: gcc
+// FIXME: linking on windows (speciifcally mingw) not yet supported, see tracking issue #125418
+//@ ignore-windows
+// A small test to make sure that unwinding works properly.
+//
+// Functions can have target-cpu applied. On apple-darwin this is super important,
+// since you can have binaries which mix x86 and aarch64 code that are compatible
+// with both architectures. So we can't just reject target_cpu on EIIs since apple
+// puts them on by default. The problem: we generate aliases. And aliases cannot
+// get target_cpu applied to them. So, instead we should, in the case of functions,
+// generate a shim function. For statics aliases should keep working.
+// However, to make this work properly,
+// on LLVM we generate shim functions instead of function aliases.
+// Little extra functions that look like
+// ```
+// function alias_symbol(*args) {return (tailcall) aliasee(*args);}
+// ```
+// This is a simple test to make sure that we can unwind through these,
+// and that this wrapper function effectively doesn't show up in the trace.
+#![feature(extern_item_impls)]
+
+extern crate decl_with_default_panics;
+
+fn main() {
+    let result = std::panic::catch_unwind(|| {
+        decl_with_default_panics::decl1(10);
+    });
+    assert!(result.is_err());
+}

tests/ui/eii/default/call_default_panics.run.stderr

@@ -0,0 +1,10 @@
+
+thread 'main' ($TID) panicked at $DIR/auxiliary/decl_with_default_panics.rs:14:5:
+10
+stack backtrace:
+   0: __rustc::rust_begin_unwind
+   1: core::panicking::panic_fmt
+   2: core::panicking::panic_display::<u64>
+   3: decl_with_default_panics::_::decl1
+   4: call_default_panics::main
+note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.