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

Comments

Projects
None yet
6 participants
@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

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 14, 2016

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 14, 2016

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

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Apr 14, 2016

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

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

@bluss bluss added the A-incr-comp label Apr 15, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Apr 16, 2016

@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

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented May 18, 2016

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

@Kha

This comment has been minimized.

Copy link
Contributor Author

Kha commented May 18, 2016

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

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented May 18, 2016

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

@eddyb

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented May 18, 2016

@eddyb yes, please!

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jun 28, 2016

This should be fixed since #33602 landed.

@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

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Aug 9, 2016

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

@sanxiyn

This comment has been minimized.

Copy link
Member

sanxiyn commented Oct 7, 2016

Fixed by #36832.

@sanxiyn sanxiyn closed this Oct 7, 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.