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

MinGW: enable dllexport/dllimport #72049

Merged
merged 2 commits into from
Jul 29, 2020
Merged

MinGW: enable dllexport/dllimport #72049

merged 2 commits into from
Jul 29, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented May 9, 2020

Fixes (only when using LLD) #50176
Fixes #72319

This makes windows-gnu on pair with windows-msvc when it comes to symbol exporting.
For MinGW it means both good things like correctly working dllimport/dllexport, ability to link with LLD and bad things like #27438.

Not sure but maybe this should land behind unstable compiler option (-Z) or environment variable?

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@bors

This comment has been minimized.

@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2020
@petrochenkov
Copy link
Contributor

r? @petrochenkov

cc @retep998, do you know anything about the changes in src/librustc_codegen_llvm/context.rs by chance?
(I've seen the other parts of this PR in #70852, but didn't investigate this one yet.)

@retep998
Copy link
Member

retep998 commented Jun 4, 2020

@petrochenkov This table covers dllimport linking success in the MSVC case: #27438 (comment)

For MinGW, several more of those entries become success instead of error (because it tries really hard to support minimally ported C code from non-windows which never put any thought into dllimport/dllexport), however the case of plain static with dynamic library is still always an error because direct access to a static cannot be redirected with a thunk the way functions can.

We should absolutely emit the appropriate dllimport/dllexport attributes on all Windows targets including MinGW as the resulting codegen is still better when done correctly.

@petrochenkov
Copy link
Contributor

@mati865
Could you add a test for #72319?
Tests using nm are usually put into the run-make directory.

@mati865
Copy link
Contributor Author

mati865 commented Jun 6, 2020

#72319 happens only when linking specific libs like llvm-libunwind. I have no idea what makes it so "special" but the test would probably require shipping that library...

@petrochenkov
Copy link
Contributor

Ok, I think this is similar to #70937 in the sense that it should be the right thing to do, it affects only windows-gnu, but there may be compatibility issues.

So, same comments from #70937 (comment) apply, and we can choose the same process of dealing with the change - land it and see whether it causes any complaints in practice.

@petrochenkov
Copy link
Contributor

#72319 happens only when linking specific libs like llvm-libunwind

Ok, shipping libunwind for a test is overkill.
(Although it would be great to figure out what happens exactly.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 6, 2020
@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 8, 2020

📌 Commit c9bd8320cd9a63580c2f2624636585d858b5c07f has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2020
@mati865
Copy link
Contributor Author

mati865 commented Jun 8, 2020

@bors r-

Sorry but there is one missing change here. I'd have committed it already but had to fix undefined reference to __ms_vsnprintf first and I'm waiting on all tests passed.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 8, 2020
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2020
@petrochenkov
Copy link
Contributor

I don't have anything new to say compared to #72049 (comment), unfortunately.

r=me after squashing commits if this doesn't break tier 1 targets.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2020
@petrochenkov
Copy link
Contributor

Needs a tidy fix.

@petrochenkov
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 28, 2020

✌️ @mati865 can now approve this pull request

@mati865
Copy link
Contributor Author

mati865 commented Jul 28, 2020

Apparently I forgot to hit submit button after the rebase.
I've reworded my in-code comments hoping they are clearer now. I'll spellcheck them and fix tidy tomorrow.

This fixes various cases where LD could not guess dllexport correctly and greatly improves compatibility with LLD which is not going to support linker scripts anytime soon
@mati865
Copy link
Contributor Author

mati865 commented Jul 29, 2020

I don't expect any breakage on the affected targets (only windows-gnu) but we should start testing ASAP and if need we can revert it for beta to give more time.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jul 29, 2020

📌 Commit 87abd65 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 29, 2020
@bors
Copy link
Contributor

bors commented Jul 29, 2020

⌛ Testing commit 87abd65 with merge 584e83d...

@bors
Copy link
Contributor

bors commented Jul 29, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 584e83d to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linking static lib to dylib can cause symbols to be missing from import lib
9 participants