Skip to content

Commit

Permalink
Mangle exported names using node IDs rather than types
Browse files Browse the repository at this point in the history 
Use node IDs rather than types to ensure exported names are unique.
duplicate symbol. Closes #2074.
  • Loading branch information
@catamorphism
catamorphism committed Apr 6, 2012
1 parent fc7fc90 commit c83d61d
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
11 changes: 5 additions & 6 deletions src/rustc/back/link.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,15 +485,14 @@ fn mangle(ss: path) -> str {
n
}

fn exported_name(path: path, hash: str, _vers: str) -> str {
fn exported_name(path: path, id: ast::node_id, _vers: str) -> str {
// FIXME: versioning isn't working yet
ret mangle(path + [path_name(hash)]); // + "@" + vers;

ret mangle(path + [path_name(int::str(id))]); // + "@" + vers;
}

fn mangle_exported_name(ccx: @crate_ctxt, path: path, t: ty::t) -> str {
let hash = get_symbol_hash(ccx, t);
ret exported_name(path, hash, ccx.link_meta.vers);
fn mangle_exported_name(ccx: @crate_ctxt, path: path, id: ast::node_id)
-> str {
ret exported_name(path, id, ccx.link_meta.vers);
}

fn mangle_internal_name_by_type_only(ccx: @crate_ctxt, t: ty::t, name: str) ->
Expand Down
8 changes: 4 additions & 4 deletions src/rustc/middle/trans/base.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1944,7 +1944,7 @@ fn monomorphic_fn(ccx: @crate_ctxt, fn_id: ast::def_id, real_substs: [ty::t],
let llfty = type_of_fn_from_ty(ccx, mono_ty);

let pt = *pt + [path_name(ccx.names(name))];
let s = mangle_exported_name(ccx, pt, mono_ty);
let s = mangle_exported_name(ccx, pt, fn_id.node);
let lldecl = decl_internal_cdecl_fn(ccx.llmod, s, llfty);
ccx.monomorphized.insert(hash_id, lldecl);
ccx.item_symbols.insert(fn_id.node, s);
Expand Down Expand Up @@ -4454,7 +4454,7 @@ fn register_fn_full(ccx: @crate_ctxt, sp: span, path: path,
fn register_fn_fuller(ccx: @crate_ctxt, sp: span, path: path,
node_id: ast::node_id, node_type: ty::t,
cc: lib::llvm::CallConv, llfty: TypeRef) -> ValueRef {
let ps: str = mangle_exported_name(ccx, path, node_type);
let ps: str = mangle_exported_name(ccx, path, node_id);
let llfn: ValueRef = decl_fn(ccx.llmod, ps, cc, llfty);
ccx.item_symbols.insert(node_id, ps);

Expand Down Expand Up @@ -4583,7 +4583,7 @@ fn get_item_val(ccx: @crate_ctxt, id: ast::node_id) -> ValueRef {
alt check i.node {
ast::item_const(_, _) {
let typ = ty::node_id_to_type(ccx.tcx, i.id);
let s = mangle_exported_name(ccx, my_path, typ);
let s = mangle_exported_name(ccx, my_path, i.id);
let g = str::as_c_str(s, {|buf|
llvm::LLVMAddGlobal(ccx.llmod, type_of(ccx, typ), buf)
});
Expand Down Expand Up @@ -4670,7 +4670,7 @@ fn trans_constant(ccx: @crate_ctxt, it: @ast::item) {
for vec::each(variants) {|variant|
let p = path + [path_name(variant.node.name),
path_name("discrim")];
let s = mangle_exported_name(ccx, p, ty::mk_int(ccx.tcx));
let s = mangle_exported_name(ccx, p, it.id);
let disr_val = vi[i].disr_val;
note_unique_llvm_symbol(ccx, s);
let discrim_gvar = str::as_c_str(s, {|buf|
Expand Down
11 changes: 11 additions & 0 deletions src/test/run-pass/issue-2074.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn main(args: [str]) {
let one = fn@() -> uint {
enum r { a };
ret a as uint;
};
let two = fn@() -> uint {
enum r { a };
ret a as uint;
};
one(); two();
}

4 comments on commit c83d61d

@brson
Copy link
Contributor

@brson brson commented on c83d61d Apr 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this on IRC and I believe this is not the right thing to do, for two reasons: 1) it changes our versioning scheme, which relies on attaching type hashes to symbols; 2) using node id's creates an unstable public interface.

@catamorphism
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will investigate how naming works for nested functions, and figure out how to do that for enums as well, rather than using node IDs to uniqify.

@graydon
Copy link
Contributor

@graydon graydon commented on c83d61d Apr 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's actually a third reason (I know, this is horrible! marijn did this at one point too though, don't feel bad about it) which is that the C++ name-mangling rules we use for encoding paths as ::-separated names breaks if you have a path component that starts with a plain number (like a node ID). Plus as brson says, it breaks other aspects of linkage.

Maybe we need a big comment in the mangler: "never incorporate node IDs here; I know it's tempting, but don't do it!" :)

@graydon
Copy link
Contributor

@graydon graydon commented on c83d61d Apr 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general technology to use in the backend for this is "mangled types when sufficient, sequential name-generators when necessary" (effectively gensyms). But the sequential generators are unsatisfying, as they produce a different form of unstable interface. Hopefully they don't infect too many exported names. Ideally you want names to derive from the identity of the item, either the programmer-provided path or the type or some other mostly-stable concept of identity, so that when unrelated things change, its name doesn't change as a side effect.

Please sign in to comment.