-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustc: Add _imp_
symbols later in compilation
#45348
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @alexcrichton!
r=me with the comment in the test case added.
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment what this is all about? Otherwise one might be a little lost when this test fails.
On MSVC targets rustc will add symbols prefixed with `_imp_` to LLVM modules to "emulate" dllexported statics as that workaround is still in place after rust-lang#27438 hasn't been solved otherwise. These statics, however, were getting gc'd by ThinLTO accidentally which later would cause linking failures. This commit updates the location we add such symbols to happen just before codegen to ensure that (a) they're not eliminated by the optimizer and (b) the optimizer doesn't even worry about them. Closes rust-lang#45347
a1fd369
to
3541ffb
Compare
@bors: r=michaelwoerister |
📌 Commit 3541ffb has been approved by |
⌛ Testing commit 3541ffb with merge 0e612b6c44ca77fa77c60680ebf0f8a498902dee... |
💔 Test failed - status-appveyor |
The test against
|
wut |
That test failure is a quickcheck-based test, and otherwise this PR should only affect linkage (not runtime), so I'm going to retry assuming it's spurious. If this lands I'll open an issue upstream. @bors: retry |
rustc: Add `_imp_` symbols later in compilation On MSVC targets rustc will add symbols prefixed with `_imp_` to LLVM modules to "emulate" dllexported statics as that workaround is still in place after #27438 hasn't been solved otherwise. These statics, however, were getting gc'd by ThinLTO accidentally which later would cause linking failures. This commit updates the location we add such symbols to happen just before codegen to ensure that (a) they're not eliminated by the optimizer and (b) the optimizer doesn't even worry about them. Closes #45347
☀️ Test successful - status-appveyor, status-travis |
This fixes a flaky test which caused spurious failures in rust-lang#45348 and rust-lang#45380
On MSVC targets rustc will add symbols prefixed with
_imp_
to LLVM modules to"emulate" dllexported statics as that workaround is still in place after #27438
hasn't been solved otherwise. These statics, however, were getting gc'd by
ThinLTO accidentally which later would cause linking failures.
This commit updates the location we add such symbols to happen just before
codegen to ensure that (a) they're not eliminated by the optimizer and (b) the
optimizer doesn't even worry about them.
Closes #45347