-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@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. |
#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... |
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. |
Ok, shipping libunwind for a test is overkill. |
This comment has been minimized.
This comment has been minimized.
@bors r+ |
📌 Commit c9bd8320cd9a63580c2f2624636585d858b5c07f has been approved by |
@bors r- Sorry but there is one missing change here. I'd have committed it already but had to fix |
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. |
Needs a tidy fix. |
@bors delegate+ |
✌️ @mati865 can now approve this pull request |
Apparently I forgot to hit submit button after the rebase. |
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
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 |
📌 Commit 87abd65 has been approved by |
☀️ Test successful - checks-actions, checks-azure |
Fixes (only when using LLD) #50176
Fixes #72319
This makes
windows-gnu
on pair withwindows-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?