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

trans: Fix __imp_ creation for i686 MSVC #28741

Merged
merged 1 commit into from
Oct 1, 2015

Conversation

alexcrichton
Copy link
Member

Turns out the symbol names are slightly different on 32-bit than on 64, so the
prefix needs to be tweaked just a bit!

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @vadimcn or @brson

Note that the bots didn't catch this because we're not running tests on 32-bit MSVC :(

@vadimcn
Copy link
Contributor

vadimcn commented Sep 30, 2015

@alexcrichton: Something's odd about this: if import stubs on x86 were prefixed with _imp__, how come @alexbool saw unresolved symbols such as __imp___ZN15Logger$LT$E$GT$6do_log15__STATIC_FMTSTR20 hf5db1df99b097f02HbaE (with two leading underscores)?

@vadimcn
Copy link
Contributor

vadimcn commented Sep 30, 2015

Upon closer look, I see that the .rlib defines symbol
___imp__ZN15Logger$LT$E$GT$6do_log4_LOC20h021bff66b31ba9b7HaaE,
whereas on the import side it's:
__imp___ZN15Logger$LT$E$GT$6do_log4_LOC20h021bff66b31ba9b7HaaE.

Looks like we are being screwed up by LLVM's symbol mangling here - I think x86 ABI requires adding a leading underscore, which is done during IR -> asm translation, but before addition of __imp_ for dllimport's.

In short, I think your fix works, though the "correct" way is probably this:

    let prefix = if cx.sess().target.target.target_pointer_width == "32" {
        "\x01__imp__"
    } else {
        "\x01__imp_"
    };

(leading \x01 suppresses symbol mangling).

@vadimcn
Copy link
Contributor

vadimcn commented Sep 30, 2015

@bors: r+ a1129e3bb51853845b942ef077be0992a50e5c59

@vadimcn
Copy link
Contributor

vadimcn commented Sep 30, 2015

Heh, bors ain't listening

@brson
Copy link
Contributor

brson commented Sep 30, 2015

@bors r+

@vadimcn bors should listen now. typo in the config file

@alexcrichton
Copy link
Member Author

@bors: r=vadimcn fb2f43d

Ended up taking @vadimcn's suggestion anyway, thanks for the investigation as well! I added a comment as well which will hopefully serve for future readers.

@vadimcn
Copy link
Contributor

vadimcn commented Sep 30, 2015

I suppose this one should be nominated along with #28646?

@alexcrichton
Copy link
Member Author

Indeed!

@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 30, 2015
Turns out the symbol names are slightly different on 32-bit than on 64, so the
prefix needs to be tweaked just a bit!
@alexcrichton
Copy link
Member Author

@bors: r+ 25354de

@alexcrichton
Copy link
Member Author

@bors: r=vadimcn 25354de

@bors
Copy link
Contributor

bors commented Oct 1, 2015

⌛ Testing commit 25354de with merge e5ba127...

bors added a commit that referenced this pull request Oct 1, 2015
Turns out the symbol names are slightly different on 32-bit than on 64, so the
prefix needs to be tweaked just a bit!
@bors bors merged commit 25354de into rust-lang:master Oct 1, 2015
@alexcrichton alexcrichton deleted the fix-msvc-32 branch October 1, 2015 22:54
@alexcrichton
Copy link
Member Author

Accepting for backport to beta

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 1, 2015
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants