[RFC] Try to ensure we don’t emit duplicate symbols

src/librustc_llvm/lib.rs

@@ -925,6 +925,7 @@ extern {
     pub fn LLVMSetThreadLocal(GlobalVar: ValueRef, IsThreadLocal: Bool);
     pub fn LLVMIsGlobalConstant(GlobalVar: ValueRef) -> Bool;
     pub fn LLVMSetGlobalConstant(GlobalVar: ValueRef, IsConstant: Bool);
+    pub fn LLVMGetNamedValue(M: ModuleRef, Name: *const c_char) -> ValueRef;
 
     /* Operations on aliases */
     pub fn LLVMAddAlias(M: ModuleRef,

src/librustc_trans/trans/base.rs

@@ -185,6 +185,7 @@ impl<'a, 'tcx> Drop for StatRecorder<'a, 'tcx> {
 // only use this for foreign function ABIs and glue, use `decl_rust_fn` for Rust functions
 pub fn decl_fn(ccx: &CrateContext, name: &str, cc: llvm::CallConv,
                ty: Type, output: ty::FnOutput) -> ValueRef {
+    ccx.assert_unique_symbol(name.to_string());
 
     let buf = CString::new(name).unwrap();
     let llfn: ValueRef = unsafe {
@@ -482,16 +483,6 @@ pub fn unset_split_stack(f: ValueRef) {
     }
 }
 
-// Double-check that we never ask LLVM to declare the same symbol twice. It
-// silently mangles such symbols, breaking our linkage model.
-pub fn note_unique_llvm_symbol(ccx: &CrateContext, sym: String) {
-    if ccx.all_llvm_symbols().borrow().contains(&sym) {
-        ccx.sess().bug(&format!("duplicate LLVM symbol: {}", sym));
-    }
-    ccx.all_llvm_symbols().borrow_mut().insert(sym);
-}
-
-
 pub fn get_res_dtor<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                               did: ast::DefId,
                               t: Ty<'tcx>,
@@ -1751,9 +1742,9 @@ pub fn build_return_block<'blk, 'tcx>(fcx: &FunctionContext<'blk, 'tcx>,
     }
 }
 
-// trans_closure: Builds an LLVM function out of a source function.
-// If the function closes over its environment a closure will be
-// returned.
+/// Builds an LLVM function out of a source function.
+///
+/// If the function closes over its environment a closure will be returned.
 pub fn trans_closure<'a, 'b, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                                    decl: &ast::FnDecl,
                                    body: &ast::Block,
@@ -1901,8 +1892,7 @@ pub fn trans_closure<'a, 'b, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
     finish_fn(&fcx, bcx, output_type, ret_debug_loc);
 }
 
-// trans_fn: creates an LLVM function corresponding to a source language
-// function.
+/// Creates an LLVM function corresponding to a source language function.
 pub fn trans_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                           decl: &ast::FnDecl,
                           body: &ast::Block,
@@ -2710,10 +2700,9 @@ fn exported_name<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, id: ast::NodeId,
         None => {}
     }
 
-    match attr::first_attr_value_str_by_name(attrs, "export_name") {
+    match attr::find_export_name_attr(ccx.sess().diagnostic(), attrs) {
         // Use provided name
         Some(name) => name.to_string(),
-
         _ => ccx.tcx().map.with_path(id, |path| {
             if attr::contains_name(attrs, "no_mangle") {
                 // Don't mangle
@@ -2752,15 +2741,28 @@ pub fn get_item_val(ccx: &CrateContext, id: ast::NodeId) -> ValueRef {
 
             let v = match i.node {
                 ast::ItemStatic(_, _, ref expr) => {
-                    // If this static came from an external crate, then
-                    // we need to get the symbol from csearch instead of
-                    // using the current crate's name/version
-                    // information in the hash of the symbol
+                    // If this static came from an external crate, then we need to get the symbol
+                    // from csearch instead of using the current crate's name/version information
+                    // in the hash of the symbol
                     let sym = sym();
-                    debug!("making {}", sym);
+                    debug!("making static {}", sym);
+
+                    // Check whether there’s no symbols with the same name yet. In case there are
+                    // any, we will suffer from the *extern declaration* being renamed if we
+                    // register this static with the same name too.
+                    // FIXME: this doesn’t work reliably anyway.
+                    if ccx.symbol_value(sym.to_string()).is_some() {
+                        ccx.sess().span_fatal(i.span, &format!("symbol {} is already declared",
+                                                               sym));
+                    } else if contains_null(&sym[..]) {
+                        // TODO: Should it be a bug? Sounds like prime example for checking
+                        // in parser and attribute getters.
+                        ccx.sess().span_fatal(i.span, &format!("Illegal null byte in export_name \
+                                                               value: `{}`", sym));
+                    }
 
-                    // We need the translated value here, because for enums the
-                    // LLVM type is not fully determined by the Rust type.
+                    // We need the translated value here, because for enums the LLVM type is not
+                    // fully determined by the Rust type.
                     let empty_substs = ccx.tcx().mk_substs(Substs::trans_empty());
                     let (v, ty) = consts::const_expr(ccx, &**expr, empty_substs);
                     ccx.static_values().borrow_mut().insert(id, v);
@@ -2772,11 +2774,7 @@ pub fn get_item_val(ccx: &CrateContext, id: ast::NodeId) -> ValueRef {
                         } else {
                             llvm::LLVMTypeOf(v)
                         };
-                        if contains_null(&sym[..]) {
-                            ccx.sess().fatal(
-                                &format!("Illegal null byte in export_name \
-                                         value: `{}`", sym));
-                        }
+
                         let buf = CString::new(sym.clone()).unwrap();
                         let g = llvm::LLVMAddGlobal(ccx.llmod(), llty,
                                                     buf.as_ptr());
@@ -2792,6 +2790,11 @@ pub fn get_item_val(ccx: &CrateContext, id: ast::NodeId) -> ValueRef {
 
                 ast::ItemFn(_, _, abi, _, _) => {
                     let sym = sym();
+                    if ccx.symbol_value(sym.to_string()).is_some() {
+                        ccx.sess().span_fatal(i.span, &format!("symbol {} is already declared",
+                                                               sym));
+                    }
+
                     let llfn = if abi == Rust {
                         register_fn(ccx, i.span, sym, i.id, ty)
                     } else {
@@ -2936,6 +2939,10 @@ fn register_method(ccx: &CrateContext, id: ast::NodeId,
     let mty = ty::node_id_to_type(ccx.tcx(), id);
 
     let sym = exported_name(ccx, id, mty, &m.attrs);
+    if ccx.symbol_value(sym.to_string()).is_some() {
+        ccx.sess().span_fatal(m.span, &format!("symbol {} is already declared", sym));
+    }
+
 
     if let ty::ty_bare_fn(_, ref f) = mty.sty {
         let llfn = if f.abi == Rust || f.abi == RustCall {

src/librustc_trans/trans/context.rs

@@ -37,6 +37,8 @@ use std::rc::Rc;
 use syntax::ast;
 use syntax::parse::token::InternedString;
 
+use libc::c_uint;
+
 pub struct Stats {
     pub n_static_tydescs: Cell<uint>,
     pub n_glues_created: Cell<uint>,
@@ -136,7 +138,6 @@ pub struct LocalCrateContext<'tcx> {
     llsizingtypes: RefCell<FnvHashMap<Ty<'tcx>, Type>>,
     adt_reprs: RefCell<FnvHashMap<Ty<'tcx>, Rc<adt::Repr<'tcx>>>>,
     type_hashcodes: RefCell<FnvHashMap<Ty<'tcx>, String>>,
-    all_llvm_symbols: RefCell<FnvHashSet<String>>,
     int_type: Type,
     opaque_vec_type: Type,
     builder: BuilderRef_res,
@@ -415,7 +416,6 @@ impl<'tcx> LocalCrateContext<'tcx> {
                 llsizingtypes: RefCell::new(FnvHashMap()),
                 adt_reprs: RefCell::new(FnvHashMap()),
                 type_hashcodes: RefCell::new(FnvHashMap()),
-                all_llvm_symbols: RefCell::new(FnvHashSet()),
                 int_type: Type::from_ref(ptr::null_mut()),
                 opaque_vec_type: Type::from_ref(ptr::null_mut()),
                 builder: BuilderRef_res(llvm::LLVMCreateBuilderInContext(llcx)),
@@ -670,10 +670,6 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
         &self.local.type_hashcodes
     }
 
-    pub fn all_llvm_symbols<'a>(&'a self) -> &'a RefCell<FnvHashSet<String>> {
-        &self.local.all_llvm_symbols
-    }
-
     pub fn stats<'a>(&'a self) -> &'a Stats {
         &self.shared.stats
     }
@@ -743,6 +739,33 @@ impl<'b, 'tcx> CrateContext<'b, 'tcx> {
             &format!("the type `{}` is too big for the current architecture",
                     obj.repr(self.tcx())))
     }
+
+    /* FIXME?: not sure about placement of these functions */
+    /// Double-check that we never ask LLVM to declare the same symbol twice. LLVM silently mangles
+    /// such symbols if they have Internal linkage, breaking our linkage model.
+    // FIXME: this function needs to be used more. Much more.
+    pub fn assert_unique_symbol<'a>(&'a self, sym: String) {
+        if let Some(v) = self.symbol_value(sym.clone()) {
+            if (unsafe { llvm::LLVMGetLinkage(v) }) == llvm::InternalLinkage as c_uint {
+                self.sess().bug(&format!("duplicate LLVM symbol: {}", sym));
+            }
+        }
+    }
+
+    /// Get LLVM value by symbol.
+    pub fn symbol_value<'a>(&'a self, sym: String) -> Option<ValueRef> {
+        let buf = CString::new(sym.clone()).unwrap();
+        let val = unsafe { llvm::LLVMGetNamedValue(self.llmod(), buf.as_ptr()) };
+        if val.is_null() { None } else { Some(val) }
+    }
+
+    /// Returns whether the symbol is defined as opposed to being just declared or not existing at
+    /// all.
+    pub fn symbol_defined<'a>(&'a self, sym: String) -> bool {
+        self.symbol_value(sym).map_or(false, |v| {
+            unsafe { llvm::LLVMIsDeclaration(v) != 0 }
+        })
+    }
 }
 
 fn declare_intrinsic(ccx: &CrateContext, key: & &'static str) -> Option<ValueRef> {

src/librustc_trans/trans/foreign.rs

@@ -111,6 +111,7 @@ pub fn register_static(ccx: &CrateContext,
     let llty = type_of::type_of(ccx, ty);
 
     let ident = link_name(foreign_item);
+    ccx.assert_unique_symbol(ident.to_string());
     match attr::first_attr_value_str_by_name(&foreign_item.attrs,
                                              "linkage") {
         // If this is a static with a linkage specified, then we need to handle
@@ -622,6 +623,11 @@ pub fn trans_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
                                       llwrapfn: ValueRef,
                                       tys: &ForeignTypes<'tcx>,
                                       t: Ty<'tcx>) {
+        if llvm::LLVMCountBasicBlocks(llwrapfn) != 0 {
+            ccx.sess().bug("wrapping a function inside non-empty wrapper, most likely cause is \
+                           multiple functions being wrapped");
+        }
+
         let _icx = push_ctxt(
             "foreign::trans_rust_fn_with_foreign_abi::build_wrap_fn");
         let tcx = ccx.tcx();
@@ -641,7 +647,6 @@ pub fn trans_rust_fn_with_foreign_abi<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
         //         foo0(&r, NULL, i);
         //         return r;
         //     }
-
         let ptr = "the block\0".as_ptr();
         let the_block = llvm::LLVMAppendBasicBlockInContext(ccx.llcx(), llwrapfn,
                                                             ptr as *const _);

src/librustc_trans/trans/glue.rs

@@ -518,7 +518,7 @@ pub fn declare_tydesc<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>)
         llvm::LLVMAddGlobal(ccx.llmod(), ccx.tydesc_type().to_ref(),
                             buf.as_ptr())
     };
-    note_unique_llvm_symbol(ccx, name);
+    ccx.assert_unique_symbol(name.to_string());
 
     let ty_name = token::intern_and_get_ident(
         &ppaux::ty_to_string(ccx.tcx(), t));
@@ -542,7 +542,6 @@ fn declare_generic_glue<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>,
         t,
         &format!("glue_{}", name));
     let llfn = decl_cdecl_fn(ccx, &fn_nm[..], llfnty, ty::mk_nil(ccx.tcx()));
-    note_unique_llvm_symbol(ccx, fn_nm.clone());
     return (fn_nm, llfn);
 }
 

src/libsyntax/attr.rs

@@ -283,6 +283,23 @@ pub fn find_crate_name(attrs: &[Attribute]) -> Option<InternedString> {
     first_attr_value_str_by_name(attrs, "crate_name")
 }
 
+/// Find the value of #[export_name=*] attribute and check its validity.
+pub fn find_export_name_attr(diag: &SpanHandler, attrs: &[Attribute]) -> Option<InternedString> {
+    attrs.iter().fold(None, |ia,attr| {
+        if attr.check_name("export_name") {
+            if let s@Some(_) = attr.value_str() {
+                s
+            } else {
+                diag.span_err(attr.span, "export_name attribute has invalid format");
+                diag.handler.help("use #[export_name=\"*\"]");
+                None
+            }
+        } else {
+            ia
+        }
+    })
+}
+
 #[derive(Copy, PartialEq)]
 pub enum InlineAttr {
     InlineNone,

src/rustllvm/RustWrapper.cpp

@@ -94,6 +94,11 @@ extern "C" void LLVMRustPrintPassTimings() {
   TimerGroup::printAll(OS);
 }
 
+extern "C" LLVMValueRef LLVMGetNamedValue(LLVMModuleRef M,
+                                          const char* Name) {
+    return wrap(unwrap(M)->getNamedValue(Name));
+}
+
 extern "C" LLVMValueRef LLVMGetOrInsertFunction(LLVMModuleRef M,
                                                 const char* Name,
                                                 LLVMTypeRef FunctionTy) {

src/test/compile-fail/dupe-symbols-1.rs

@@ -0,0 +1,20 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+#![crate_type="rlib"]
+#![allow(warnings)]
+
+#[export_name="fail"]
+pub fn a() {
+}
+
+#[export_name="fail"]
+pub fn b() {
+//~^ symbol fail is already declared
+}

src/test/compile-fail/dupe-symbols-2.rs

@@ -0,0 +1,24 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+#![crate_type="rlib"]
+#![allow(warnings)]
+
+mod a {
+    #[no_mangle]
+    pub extern fn fail() {
+    }
+}
+
+mod b {
+    #[no_mangle]
+    pub extern fn fail() {
+    //~^ symbol fail is already declared
+    }
+}

src/test/compile-fail/dupe-symbols-3.rs

@@ -0,0 +1,20 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+#![crate_type="rlib"]
+#![allow(warnings)]
+
+#[export_name="fail"]
+pub fn a() {
+}
+
+#[no_mangle]
+pub fn fail() {
+//~^ symbol fail is already declared
+}

src/test/compile-fail/dupe-symbols-4.rs

@@ -0,0 +1,30 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+#![crate_type="rlib"]
+#![allow(warnings)]
+
+
+pub trait A {
+    fn fail(self);
+}
+
+struct B;
+struct C;
+
+impl A for B {
+    #[no_mangle]
+    fn fail(self) {}
+}
+
+impl A for C {
+    #[no_mangle]
+    fn fail(self) {}
+    //~^ symbol fail is already declared
+}