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 monomorphized trait impls are not stable across crates #32948

Closed
Kha opened this issue Apr 14, 2016 · 12 comments
Closed

Symbol names for monomorphized trait impls are not stable across crates #32948

Kha opened this issue Apr 14, 2016 · 12 comments
Labels
A-incr-comp Area: Incremental compilation E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@Kha
Copy link
Contributor

Kha commented Apr 14, 2016

In the spirit of #32554:

crate A:

pub trait Foo {
    fn foo<T>();
}

pub struct Bar;

impl Foo for Bar {
    fn foo<T>() { }
}

pub fn bar() {
    Bar::foo::<Bar>();
}

crate B:

extern crate A;
use A::*;

fn bar2() {
    Bar::foo::<Bar>();
}

A.ll:

define internal void @"_ZN27_$LT$Bar$u20$as$u20$Foo$GT$3foo17hde2cfe0cb82f67ddE"() unnamed_addr #0 !dbg !12 {

B.ll:

define internal void @"_ZN33_$LT$A..Bar$u20$as$u20$A..Foo$GT$3foo17he9b727db8600b5deE"() unnamed_addr #0 !dbg !13 {

IOW, A uses the encoding of <Bar as Foo>::foo, while B uses <A::Bar as A::Foo>::foo. The issue seems to be that while symbol names correctly use ctxt::absolute_item_path_str, that function indirectly calls ctxt::item_path_str via e.g. TraitRef's Display implementation.

cc @nikomatsakis @michaelwoerister

@nikomatsakis
Copy link
Contributor

I think that calling absolute_item_path_str is wrong. That method is intended for user output (as the comment says).

@nikomatsakis
Copy link
Contributor

If you look in back::symbol_names, you will see the SymbolPathBuffer, which is what we ought to be using. Not sure how we wind up using the wrong thing exactly though.

@michaelwoerister
Copy link
Member

@Kha Good catch.
@nikomatsakis The problem is here:

buffer.push(&format!("<{} as {}>",

@bluss bluss added the A-incr-comp Area: Incremental compilation label Apr 15, 2016
@nikomatsakis
Copy link
Contributor

@michaelwoerister ah, I see, yes, that makes sense. I guess the problem is that we don't have a way to serialize type names that is cross-crate safe. We should try to keep the "spirit" of that rule working though since it'd be nice to get those paths in the symbol names.

@michaelwoerister
Copy link
Member

This should be fixed by @eddyb's metadata PR.

@Kha
Copy link
Contributor Author

Kha commented May 18, 2016

Using a global variable, interesting... well, I didn't think of that solution 😄 .

@michaelwoerister
Copy link
Member

@Kha It's just a thread-local at least :P

@eddyb
Copy link
Member

eddyb commented May 18, 2016

I think we can improve on the situation by moving to a scheme where we dump types in a structured, but simpler, manner than the current printing logic, and then we can even reuse parts of C++ mangling schemes to get smaller symbols and cleaner c++filt output, at the same time.

@michaelwoerister
Copy link
Member

@eddyb yes, please!

@michaelwoerister
Copy link
Member

This should be fixed since #33602 landed.

@michaelwoerister michaelwoerister added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Jun 28, 2016
@nikomatsakis nikomatsakis added this to the Incremental compilation alpha milestone Jul 23, 2016
@nikomatsakis nikomatsakis removed this from the Incremental compilation alpha milestone Aug 9, 2016
@nikomatsakis
Copy link
Contributor

Removing from milestone since this is just needs-test now.

@sanxiyn
Copy link
Member

sanxiyn commented Oct 7, 2016

Fixed by #36832.

@sanxiyn sanxiyn closed this as completed Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

No branches or pull requests

6 participants