[IRGen] Be smarter about adding the FORCE_LINK symbols for overlays #15647
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We use dummy symbols to force overlays not to get dropped when autolinking, even if the user doesn't use anything from them explicitly. This behavior is triggered by the semi-hidden flag -autolink-force-load. (It's semi-hidden because it has few legitimate uses in real life. If you searched for "how to force autolinking to pick up a library" and found this commit, don't just do this and move on. Come talk to me on forums.swift.org.) Previously we added these dummy symbols to every object file using "common" linkage, a little-known feature added for C that ensures that only one definition will actually get used in the final object file. However, the way we were doing that wouldn't work so well for COFF, and so in 1025eed Saleem changed this to use "weak ODR" linkage. This has *nearly* the same effect, and avoids some other weirdness, but has the downside of making the symbol in the final dylib "weak" itself, meaning that some /other/ library could come along and override it. That impacts loading time, and an Apple-internal tool caught that as rdar://39019606. To avoid this whole mess, "just" emit the symbol into the object file that corresponds to the first file in the module, which allows us to mark it as a normal public symbol. P.S. None of this is actually important at the moment because all of the overlays are built with single-threaded WMO, which always produces one object file anyway. But I wanted to get it right once and for all.
|
@swift-ci Please test |
|
cc @Rostepher |
compnerd
approved these changes
Mar 31, 2018
There was a problem hiding this comment.
This mostly looks good. The one thing that I wonder about is the test suite. IIRC, there are windows tests for the auto linking too, but they seem untouched? That seems a bit odd. In general I've tried hard to try to make the tests run irrespective of the build host, so, perhaps it was loose enough as it was checking that the function was emitted, not the linkage type.
|
Yep, I did check that the COFF IRGen tests were still passing. If there are tests that are only run under Thanks, Saleem! |
jrose-apple
added a commit
to jrose-apple/swift
that referenced
this pull request
Apr 2, 2018
…wiftlang#15647) We use dummy symbols to force overlays not to get dropped when autolinking, even if the user doesn't use anything from them explicitly. This behavior is triggered by the semi-hidden flag -autolink-force-load. (It's semi-hidden because it has few legitimate uses in real life. If you searched for "how to force autolinking to pick up a library" and found this commit, don't just do this and move on. Come talk to me on forums.swift.org.) Previously we added these dummy symbols to every object file using "common" linkage, a little-known feature added for C that ensures that only one definition will actually get used in the final object file. However, the way we were doing that wouldn't work so well for COFF, and so in 1025eed Saleem changed this to use "weak ODR" linkage. This has *nearly* the same effect, and avoids some other weirdness, but has the downside of making the symbol in the final dylib "weak" itself, meaning that some /other/ library could come along and override it. That impacts loading time, and an Apple-internal tool caught that as rdar://39019606. To avoid this whole mess, "just" emit the symbol into the object file that corresponds to the first file in the module, which allows us to mark it as a normal public symbol. P.S. None of this is actually important at the moment because all of the overlays are built with single-threaded WMO, which always produces one object file anyway. But I wanted to get it right once and for all. (cherry picked from commit 6af333f)
|
Oh, yuck, this doesn't work with incremental builds. I don't think we need to fix that right now, but I should at least put in an error. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use dummy symbols to force overlays not to get dropped when autolinking, even if the user doesn't use anything from them explicitly. This behavior is triggered by the semi-hidden flag -autolink-force-load.
(It's semi-hidden because it has few legitimate uses in real life. If you searched for "how to force autolinking to pick up a library" and found this pull request, don't just do this and move on. Come talk to me on https://forums.swift.org.)
Previously we added these dummy symbols to every object file using "common" linkage, a little-known feature added for C that ensures that only one definition will actually get used in the final object file. However, the way we were doing that wouldn't work so well for COFF, and so in #14493 @compnerd changed this to use "weak ODR" linkage. This has nearly the same effect, and avoids some other weirdness, but has the downside of making the symbol in the final dylib "weak" itself, meaning that some other library could come along and override it. That impacts loading time, and an Apple-internal tool caught that as rdar://39019606.
To avoid this whole mess, "just" emit the symbol into the object file that corresponds to the first file in the module, which allows us to mark it as a normal public symbol.
P.S. None of this is actually important at the moment because all of the overlays are built with single-threaded WMO, which always produces one object file anyway. But I wanted to get it right once and for all.