-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Attempt two at solving Windows symbol linking issues #8117
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
Conversation
Reverts the prevous attempt at fixing symbol export errors in EXEs. Instead, for Windows triples, we mark all targets as needing to be static. Then, after the products are planned, walks throught the staticTargets of DLL products and marks as not static. This allows executables in packages to import modules in local DLL products as exporting. This is a temporary fix until we add the ability for targets to explicity depend on local products.
Doing recursive deps brought back the exported a symbol you imported warnings.
This will happen when a module that exports a symbol for a DLL is also linked into an EXE with modules that import it.
@swift-ci please test |
@swift-ci please test windows |
// Occurs when an object file exports a symbol is imported | ||
// into another module in the same executable. | ||
// FIXME: Need to support targets as DLLs | ||
args += ["-Xlinker", "/ignore:4217"] |
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.
Please do not do this - it is hiding a real issue (one which has performance implications).
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.
The ones I'm seeing aren't actionable by the user. They're caused by our lack of support for dynamic targets. There's so many, you miss any other warning that might be actionable.
If a target appears in a DLL, it doesn't get the -static arg. But then if you include it in a EXE, like tests, you get the warning.
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.
A lot of the people I talk to agree we need to allow targets as DLLs, so this is really only temporary until we can get there. We'll need to do a Pitch/Evolution proposal for that since that will need a package manifest change so it will end up being post 6.1.
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.
@compnerd brings up a good point about hiding user caused warnings. I'll take this out for now and see what the fallout is. And we'll continue to work to make sure SwiftPM isn't causing these warnings.
This can hide real user problems. We'll check back later if these warnings are being too much noise.
@swift-ci please test |
@swift-ci please test windows |
Instead of creating two modules, one that exports symbols and one that doesn't and letting consuming products pick which one they want, we now mark all targets on Windows as static and then reset that if a target is in a DLL product. This gets by the missing feature of marking a target as a DLL (only products can be DLLs).
We also turn off the exporting imported symbol warning that is generated when a module in a DLL is also used in a static context confusing the linker.