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

Temporary workaround for compiler bugs in impl Trait feature. #91

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

tikue
Copy link
Collaborator

@tikue tikue commented Feb 2, 2017

We will want to revert this as soon as possible. There's not really a good way to test whether it's fixed because we don't even have a rustc bug right now to track.

cc @jonhoo

@shaladdle shaladdle merged commit 2ffa113 into google:master Feb 2, 2017
jonhoo added a commit to mit-pdos/noria that referenced this pull request Feb 2, 2017
@mcmathja
Copy link

mcmathja commented Feb 2, 2017

Was this about a linker error when trying to implement FutureClient from a separate crate? Was running into issues with that the other night. Tracked this down, if it's of any assistance: rust-lang/rust#35870. Modifying the macro to mark service methods inline resolved the issue in my case.

@compressed
Copy link
Contributor

That's interesting. That issue also talks about some visibility issues. I wonder if that is actually the underlying problem since the full type has some types that needed to be public with this change.

Maybe just making those public would make it go away.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 2, 2017

@mcmathja yeah, that bug came up in our discussion on gitter yesterday too — thanks for confirming!

@compressed I just tried locally reverting the signature change in 9d552e4, but keeping the visibility modifiers, and that then still causes the linking error for me unfortunately :/

@tikue
Copy link
Collaborator Author

tikue commented Feb 2, 2017

Thanks for the info @mcmathja! Do you happen to have a reproduction you can share? I'd love to open a rustc issue.

@mcmathja
Copy link

mcmathja commented Feb 3, 2017

Played around with this for a bit and the issue is more subtle than I thought. The calls to unreachable!() here and here are getting expanded to the standard panic call, which runs something like:

::rt::begin_panic("internal error: entered unreachable code",
{
    static _FILE_LINE: (&'static str, u32) = ("src/lib.rs", 7u32);
     &_FILE_LINE
})

For me at least, it seems to be those _FILE_LINE references that are creating difficulties:

  = note: Undefined symbols for architecture x86_64:
            "service::FutureClient::test::_$u7b$$u7b$closure$u7d$$u7d$::_FILE_LINE::hd619a832b2c2aa1b", referenced from:
                service::FutureClient::test::_$u7b$$u7b$closure$u7d$$u7d$::h4a4885791567c19d in client-b336e68e002de9fe.0.o
            "service::FutureClient::test::_$u7b$$u7b$closure$u7d$$u7d$::_FILE_LINE::hcb35c9d631e99707", referenced from:
                service::FutureClient::test::_$u7b$$u7b$closure$u7d$$u7d$::h4a4885791567c19d in client-b336e68e002de9fe.0.o

Replacing the macros with concrete errors resolves the issue. It's beyond me why only those two calls need to be replaced, I haven't been able to set up a more generic reproduction. (Have the one I used to generate those errors here). Looks like a different bug from the one I linked earlier in any case.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 3, 2017

Yup, that looks like my error too.

@jonhoo
Copy link
Contributor

jonhoo commented Feb 3, 2017

Interestingly, I get a bunch of these errors if I try to compile alacritty on beta or nightly (after a cargo update; haven't tried without).

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2017

So here's something interesting: I now have mit-pdos/noria@b7abc6e compiling on one machine, and failing with a linker error on another, just using impl Trait in my crate, and on the same nightly (2017-03-03). Digging into it now. Will report back what I find.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2017

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2017

A cargo update seems to have inexplicably fixed the problem, even though no dependencies changes. cargo build --release still fails though. And on more machines..

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2017

Following this up in rust-lang/rust#35870 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants