Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbol names for generics are not stable across crates #32554

Closed
dotdash opened this Issue Mar 28, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@dotdash
Copy link
Contributor

dotdash commented Mar 28, 2016

Given this:
cgu.rs

#![crate_type="lib"]
pub fn id<T>(t: T) -> T {
  t
}
pub fn user() {
  id(0);
}

a.rs

#![crate_type="lib"]
extern crate cgu;
pub fn user() {
  cgu::id(0);
}

We get two different symbol names for the monomorphizations of id.

cgu.ll

define internal i32 @_ZN3cgu2id17h7cbc0f1ce1830012E(i32) unnamed_addr #0 {
entry-block:
  %dropflag_hint_6 = alloca i8
  %t = alloca i32
  store i8 61, i8* %dropflag_hint_6
  store i32 %0, i32* %t
  %1 = load i32, i32* %t
  ret i32 %1
}

a.ll

define internal i32 @_ZN3cgu2id17h8a873a51d5137f87E(i32) unnamed_addr #0 {
entry-block:
  %t = alloca i32
  store i32 %0, i32* %t
  %1 = load i32, i32* %t
  ret i32 %1
}

cc @nikomatsakis @michaelwoerister

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Mar 29, 2016

Interesting.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Mar 29, 2016

I'm going to look into this a little...

@michaelwoerister michaelwoerister self-assigned this Mar 29, 2016

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Mar 29, 2016

That might be the culprit:

self.sess.cstore.crate_name(cnum)

pub fn crate_disambiguator(&self, cnum: ast::CrateNum) -> token::InternedString {
        if cnum == LOCAL_CRATE {
            self.sess.crate_disambiguator.get().as_str()
        } else {
            self.sess.cstore.crate_name(cnum)
        }
    }

One at least...

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 29, 2016

@michaelwoerister d'oh that looks like a copy-n-paste bug...

On Tue, Mar 29, 2016 at 10:46 AM, Michael Woerister <
notifications@github.com> wrote:

That might be the culprit:

self.sess.cstore.crate_name(cnum)

pub fn crate_disambiguator(&self, cnum: ast::CrateNum) -> token::InternedString {
if cnum == LOCAL_CRATE {
self.sess.crate_disambiguator.get().as_str()
} else {
self.sess.cstore.crate_name(cnum)
}
}

One at least...


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32554 (comment)

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Mar 29, 2016

It does. I'm currently in the process of writing a test case for this.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Mar 29, 2016

aren't these internal symbols anyway?

@dotdash

This comment has been minimized.

Copy link
Contributor Author

dotdash commented Mar 29, 2016

@arielb1 Not necessarily when using multiple codegen units. And even otherwise, it's nice to have the same name across all crates. In theory, for a size-optimized build we might also want to switch to linkonce_odr or weak_odr linkage, in which case we'd also need the names to be the same.

bors added a commit that referenced this issue Apr 1, 2016

Auto merge of #32579 - michaelwoerister:stable-symbol-name-fix, r=eddyb
Fix typo in TxCtxt::crate_disambiguator() and add test case.

r? @nikomatsakis

Fixes #32554

bors added a commit that referenced this issue Apr 2, 2016

Auto merge of #32579 - michaelwoerister:stable-symbol-name-fix, r=eddyb
Fix typo in TxCtxt::crate_disambiguator() and add test case.

r? @nikomatsakis

Fixes #32554

bors added a commit that referenced this issue Apr 3, 2016

Auto merge of #32579 - michaelwoerister:stable-symbol-name-fix, r=eddyb
Fix typo in TxCtxt::crate_disambiguator() and add test case.

r? @nikomatsakis

Fixes #32554

@bors bors closed this in #32579 Apr 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.