Auto merge of #32742 - eddyb:cast-fns, r=dotdash

trans: don't declare symbols that were already imported.

Fixes #32740 by checking for a declaration before attempting a new one.
Before, `LLVMGetOrInsertFunction` was called for a existing import, but with a different type.
The returned value was a cast function pointer instead of a declaration, and we gave this
value to `llvm::SetFunctionCallConv` & friends , which triggered an LLVM assertion.

3 files changed, 51 insertions, 21 deletions

diff --git a/src/librustc_trans/callee.rs b/src/librustc_trans/callee.rs
index a323d63adae..6b0945b2bb2 100644
--- a/src/librustc_trans/callee.rs
+++ b/src/librustc_trans/callee.rs
@@ -541,14 +541,6 @@ fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
         }
     };
 
-    let llfn = declare::declare_fn(ccx, &sym, ty);
-    attributes::from_fn_attrs(ccx, attrs, llfn);
-    if let Some(id) = local_item {
-        // FIXME(eddyb) Doubt all extern fn should allow unwinding.
-        attributes::unwind(llfn, true);
-        ccx.item_symbols().borrow_mut().insert(id, sym);
-    }
-
     // This is subtle and surprising, but sometimes we have to bitcast
     // the resulting fn pointer.  The reason has to do with external
     // functions.  If you have two crates that both bind the same C
@@ -572,12 +564,32 @@ fn get_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     // This can occur on either a crate-local or crate-external
     // reference. It also occurs when testing libcore and in some
     // other weird situations. Annoying.
+
     let llptrty = type_of::type_of(ccx, fn_ptr_ty);
-    let llfn = if common::val_ty(llfn) != llptrty {
-        debug!("get_fn: casting {:?} to {:?}", llfn, llptrty);
-        consts::ptrcast(llfn, llptrty)
+    let llfn = if let Some(llfn) = declare::get_declared_value(ccx, &sym) {
+        if common::val_ty(llfn) != llptrty {
+            if local_item.is_some() {
+                bug!("symbol `{}` previously declared as {:?}, now wanted as {:?}",
+                     sym, Value(llfn), llptrty);
+            }
+            debug!("get_fn: casting {:?} to {:?}", llfn, llptrty);
+            consts::ptrcast(llfn, llptrty)
+        } else {
+            debug!("get_fn: not casting pointer!");
+            llfn
+        }
     } else {
+        let llfn = declare::declare_fn(ccx, &sym, ty);
+        assert_eq!(common::val_ty(llfn), llptrty);
         debug!("get_fn: not casting pointer!");
+
+        attributes::from_fn_attrs(ccx, attrs, llfn);
+        if let Some(id) = local_item {
+            // FIXME(eddyb) Doubt all extern fn should allow unwinding.
+            attributes::unwind(llfn, true);
+            ccx.item_symbols().borrow_mut().insert(id, sym);
+        }
+
         llfn
     };
 
diff --git a/src/librustc_trans/declare.rs b/src/librustc_trans/declare.rs
index 83af511f087..eb520fe744a 100644
--- a/src/librustc_trans/declare.rs
+++ b/src/librustc_trans/declare.rs
@@ -26,6 +26,7 @@ use abi::{Abi, FnType};
 use attributes;
 use context::CrateContext;
 use type_::Type;
+use value::Value;
 
 use std::ffi::CString;
 
@@ -146,27 +147,33 @@ pub fn define_internal_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
 }
 
 
-/// Get defined or externally defined (AvailableExternally linkage) value by
-/// name.
-pub fn get_defined_value(ccx: &CrateContext, name: &str) -> Option<ValueRef> {
-    debug!("get_defined_value(name={:?})", name);
+/// Get declared value by name.
+pub fn get_declared_value(ccx: &CrateContext, name: &str) -> Option<ValueRef> {
+    debug!("get_declared_value(name={:?})", name);
     let namebuf = CString::new(name).unwrap_or_else(|_|{
         bug!("name {:?} contains an interior null byte", name)
     });
     let val = unsafe { llvm::LLVMGetNamedValue(ccx.llmod(), namebuf.as_ptr()) };
     if val.is_null() {
-        debug!("get_defined_value: {:?} value is null", name);
+        debug!("get_declared_value: {:?} value is null", name);
         None
     } else {
+        debug!("get_declared_value: {:?} => {:?}", name, Value(val));
+        Some(val)
+    }
+}
+
+/// Get defined or externally defined (AvailableExternally linkage) value by
+/// name.
+pub fn get_defined_value(ccx: &CrateContext, name: &str) -> Option<ValueRef> {
+    get_declared_value(ccx, name).and_then(|val|{
         let declaration = unsafe {
             llvm::LLVMIsDeclaration(val) != 0
         };
-        debug!("get_defined_value: found {:?} value (declaration: {})",
-                name, declaration);
         if !declaration {
             Some(val)
         } else {
             None
         }
-    }
+    })
 }
diff --git a/src/test/run-pass/foreign-dupe.rs b/src/test/run-pass/foreign-dupe.rs
index 11de5ac70f4..6c393ce99e3 100644
--- a/src/test/run-pass/foreign-dupe.rs
+++ b/src/test/run-pass/foreign-dupe.rs
@@ -31,9 +31,20 @@ mod rustrt2 {
     }
 }
 
+mod rustrt3 {
+    // Different type, but same ABI (on all supported platforms).
+    // Ensures that we don't ICE or trigger LLVM asserts when
+    // importing the same symbol under different types.
+    // See https://github.com/rust-lang/rust/issues/32740.
+    extern {
+        pub fn rust_get_test_int() -> *const u8;
+    }
+}
+
 pub fn main() {
     unsafe {
-        rustrt1::rust_get_test_int();
-        rustrt2::rust_get_test_int();
+        let x = rustrt1::rust_get_test_int();
+        assert_eq!(x, rustrt2::rust_get_test_int());
+        assert_eq!(x as *const _, rustrt3::rust_get_test_int());
     }
 }