Just autolink everything #16349
Just autolink everything #16349
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test source compatibility |
There was a problem hiding this comment.
Assuming it's safe to yank @_usableFromInline (nobody else is using it? we made no promises about it?) this seems reasonable & conservative.
(I also have no idea what sort of corner this paints us into post-4.2 but I do not feel like I have enough of the full picture to know what you are alluding to as a "real fix" later, or whether this makes it harder, etc. I.e. I feel my opinion on the local change doesn't necessarily extend to an opinion on the overall strategy)
|
Build failed |
|
Linux test failure is unrelated @swift-ci Please test Linux |
Yep, Slava added it within the last two weeks, and only for this purpose.
At worst, we'll have to keep this logic around for Swift 4 mode. But in practice I don't think that this will affect anything outside of inlinable code—everywhere you could directly reference a name, autolinking already picked it up. You can see some of my thoughts on where to go for real on the forums. |
|
Build failed |
Makes sense, thanks for clarifying! |
|
Build failed |
jrose-apple
force-pushed
the
just-autolink-everything
branch
from
f6c21cc to
f51e027
Compare
|
Source compat tests still going at https://ci.swift.org/job/swift-PR-source-compat-suite/1152/. The only changes here are test updates. @swift-ci Please test |
|
Build failed |
|
Build failed |
|
Source compat tests passed, now just waiting for regular tests. |
|
Why are you reverting the cleanups? I don't think they're actually related to the solution and made the code a bit simpler. |
|
I needed |
jrose-apple
force-pushed
the
just-autolink-everything
branch
from
f51e027 to
c9ad24e
Compare
|
Full Apple platform tests continue here: https://ci.swift.org/job/swift-PR-osx/4726/ @swift-ci Please test Linux |
|
@swift-ci Please smoke test macOS |
|
Build failed |
|
I just can't win on this one. @compnerd, how are these tests supposed to work on non-Apple platforms? |
…nces from inlinable functions (swiftlang#16326)" This reverts commit ee6e190. It's not sufficient to solve the problem, and the choices were to do something more complicated, or just take a simple brute force approach. We're going with the latter.
This reverts commit a12f42c.
This reverts commit 0c32c54.
And disable them on non-ObjC platforms again, because those overlays depend on a stdlib that contains _ObjectiveCBridgeable.
This is unfortunate in that it makes the linker do extra work, but in practice it probably doesn't matter much, and meanwhile it handles all our problems with @inlinable. Alternate solution to rdar://problem/39338239
One bit of Slava's cleanup that I /can/ keep. No one is using this parameter anymore.
jrose-apple
force-pushed
the
just-autolink-everything
branch
from
c9ad24e to
fb59305
Compare
|
@swift-ci Please test Linux |
|
@swift-ci Please smoke test macOS |
|
Build failed |
|
@jrose-apple the IRGen tests should definitely work on all targets, nothing there is an execution test, so we should be able to enable them on all targets. ObjC interop CodeGen is entirely controlled at the driver level. When I ran the tests |
|
The tests in question were using the mock SDK but not the mock overlays, which isn't exactly a useful configuration. But yeah, hopefully Lily's work will get us further. |
|
@swift-ci Please test Linux |
|
Build failed |
|
@swift-ci Please test Linux |
|
The breaks Red Hat Fedora 28: |
|
Those seem like reasonable errors, actually. Are we missing a build dependency on swiftGlibc? |
|
Hm, there's a …oh, right, we don't use autolinking in our CMake build, but we will still pick up the |
|
I don't know. I use unified builds if it matters. Also, this pull request causes a 46.5% performance regression on my box when building |
|
…that makes no sense. Swift.o only imports one thing. It's pretty much not possible for that to be this PR. |
|
Hmm… I think I misread where your branch started. Let me double check |
|
Hi @jrose-apple – Just to circle back, the performance problem I observed is not the fault of this pull request. That being said, the performance problem I'm observing is truly weird. I'll post to the forum. |
Yet another attempt at fixing rdar://problem/39338239.
The problem: If module A uses some inlinable code from module B (including function default arguments), and that code uses a symbol in module C, but module A doesn't itself import module C, we'll get a link error.
Attempt 1: Record which imports are used within inlinable code (#16326).
Issue 1: Module B might not have imported module C directly (comment).
Attempt 2: Record which modules are used within inlinable code by adding them to the list of imports (#16299).
Issue 2: This affects name lookup. Declarations in module C might be shadowed by whatever imported module C.
Attempt 3: Record a separate list of modules used within inlinable code, for autolinking purposes only (#16317).
Issue 3: Doesn't play nice with Apple-internal SDK tricks using the newly-introduced
export_asautolinking feature (apple/swift-clang#124).Attempt 4 (this PR): Just autolink everything that's imported, publicly or privately. This isn't as bad as it sounds; we already drag in private imports just to load a swiftmodule, and C modules tend to re-export everything all the time anyway for compatibility with textual includes. We'll come up with something better post-4.2. (This means reverting a number of Slava's changes in pursuit of the earlier attempts.)
Rejected alternative: use Attempt 3's model, but save autolinking info directly, instead of saving a list of modules. Rejected because really we're just making this more complex when we need a safe fix for 4.2.