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: Be a little more picky about dllimport #27208

Merged
merged 1 commit into from
Jul 24, 2015

Conversation

alexcrichton
Copy link
Member

Currently you can hit a link error on MSVC by only referencing static items from
a crate (no functions for example) and then link to the crate statically (as all
Rust crates do 99% of the time). A detailed investigation can be found on
github
, but the tl;dr is that we need to stop applying dllimport so
aggressively.

This commit alters the application of dllimport on constants to only cases where
the crate the constant originated from will be linked as a dylib in some output
crate type. That way if we're just linking rlibs (like the motivation for this
issue) we won't use dllimport. For the compiler, however, (which has lots of
dylibs) we'll use dllimport.

cc #26591

Currently you can hit a link error on MSVC by only referencing static items from
a crate (no functions for example) and then link to the crate statically (as all
Rust crates do 99% of the time). A detailed investigation can be found [on
github][details], but the tl;dr is that we need to stop applying dllimport so
aggressively.

This commit alters the application of dllimport on constants to only cases where
the crate the constant originated from will be linked as a dylib in some output
crate type. That way if we're just linking rlibs (like the motivation for this
issue) we won't use dllimport. For the compiler, however, (which has lots of
dylibs) we'll use dllimport.

[details]: rust-lang#26591 (comment)

cc rust-lang#26591
@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@alexcrichton
Copy link
Member Author

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned Aatch Jul 22, 2015
@brson
Copy link
Contributor

brson commented Jul 23, 2015

@bors r+

I approved this as a workaround, but it seems like the dllimport situation is a big mess, and from your code comment maybe impossible to resolve correctly with the type of hueristic-based approach we've got now.

@bors
Copy link
Contributor

bors commented Jul 23, 2015

📌 Commit a0efd3a has been approved by brson

@bors
Copy link
Contributor

bors commented Jul 23, 2015

⌛ Testing commit a0efd3a with merge a24dec0...

@bors
Copy link
Contributor

bors commented Jul 23, 2015

💔 Test failed - auto-win-gnu-64-nopt-t

@alexcrichton
Copy link
Member Author

@bors: retry

On Thu, Jul 23, 2015 at 4:36 PM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-gnu-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-win-gnu-64-nopt-t/builds/790


Reply to this email directly or view it on GitHub
#27208 (comment).

@bors
Copy link
Contributor

bors commented Jul 24, 2015

⌛ Testing commit a0efd3a with merge c9ef1a5...

bors added a commit that referenced this pull request Jul 24, 2015
Currently you can hit a link error on MSVC by only referencing static items from
a crate (no functions for example) and then link to the crate statically (as all
Rust crates do 99% of the time). A detailed investigation can be found [on
github][details], but the tl;dr is that we need to stop applying dllimport so
aggressively.

This commit alters the application of dllimport on constants to only cases where
the crate the constant originated from will be linked as a dylib in some output
crate type. That way if we're just linking rlibs (like the motivation for this
issue) we won't use dllimport. For the compiler, however, (which has lots of
dylibs) we'll use dllimport.

[details]: #26591 (comment)

cc #26591
@bors bors merged commit a0efd3a into rust-lang:master Jul 24, 2015
@alexcrichton alexcrichton deleted the msvc-less-dllimport branch August 17, 2015 19:50
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