Skip to content

SourceKit: explicitly link against BlocksRuntime as well #19668

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

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Oct 2, 2018

On Darwin platforms, libdispatch and libBlocksRuntime are re-exported from
libSystem (via LC_REEXPORT_DYLIB). Other platforms do not have libdispatch and
libBlocksRuntime in their C runtime, so we need to explicitly link against them.
Now that we are building BlocksRuntime with hidden visibility, we do not
accidentally get the symbols from libdispatch.

@compnerd
Copy link
Member Author

compnerd commented Oct 2, 2018

CC: @gottesmm @kevints @harlanhaskins

@compnerd
Copy link
Member Author

compnerd commented Oct 2, 2018

Please test with following pull request:
swiftlang/swift-corelibs-libdispatch#394

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2018

Build failed
Swift Test OS X Platform
Git Sha - 5929e00d6e1fde72be3d2aa9bb0edc2c035cac8c

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2018

Build failed
Swift Test Linux Platform
Git Sha - 5929e00d6e1fde72be3d2aa9bb0edc2c035cac8c

@kevints
Copy link
Contributor

kevints commented Oct 2, 2018

@compnerd I am not sure I understand the comment about building BlocksRuntime with hidden visibility. As I mentioned in
#19640 (comment) and
#19640 (comment) I think we need to ensure that only one copy of BlocksRuntime exists in the final program. Building with hidden visibility only hides this issue.

Also the description contains placeholder text, can you update it or delete it?

@compnerd
Copy link
Member Author

compnerd commented Oct 2, 2018

@kevints, I agree that in the end we need a single copy of BlocksRuntime in the runtime. The hidden visibility is masking over the problem that the BlocksRuntime is not currently built shared. Building it shared is the only way to really guarantee that it is truly unique. The thing is, in order to get it to build shared, we need to fix a staging issue (the BlocksRuntime is not currently copied to a location that swift-pm is expecting. Once we fix that, we should have a shared BlocksRuntime which will give us the uniqueness on all the targets. It also will mirror what currently is the case on Darwin - libSystem.dylib uses a LC_REEXPORT_DYLIB for libSystem_blocks.dylib to provide a shared BlocksRuntime.

@gottesmm
Copy link
Contributor

gottesmm commented Oct 2, 2018

I am still reading the PR. But this seems like something that @gparker42 may be interested in.

@gparker42
Copy link
Contributor

Temporarily using a private BlocksRuntime should be safe for now. In the event that you get two runtimes in the same process, they'll probably have the same contents: the blocks runtime is a slow-moving target. Also I can't think of any places that perform comparisons of any symbol addresses (e.g. nothing checks for isa == _NSConcreteMallocBlock so nothing should be confused by two copies of _NSConcreteMallocBlock itself).

@nkcsgexi nkcsgexi requested a review from akyrtzi October 2, 2018 20:04
@compnerd
Copy link
Member Author

compnerd commented Oct 3, 2018

@gparker42 - yeah, thats my take as well. Plus, the short term that I'm talking about here is days (I have changes for libdispatch to build both BlocksRuntime -- shared and static -- and then a subsequent change to use the shared BlocksRuntime to build SourceKit).

@compnerd compnerd force-pushed the explicit-BlocksRuntime branch from 5929e00 to da22801 Compare October 3, 2018 15:24
@compnerd
Copy link
Member Author

compnerd commented Oct 3, 2018

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2018

Build failed
Swift Test Linux Platform
Git Sha - 5929e00d6e1fde72be3d2aa9bb0edc2c035cac8c

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2018

Build failed
Swift Test OS X Platform
Git Sha - 5929e00d6e1fde72be3d2aa9bb0edc2c035cac8c

On Darwin platforms, libdispatch and libBlocksRuntime are re-exported from
libSystem (via LC_REEXPORT_DYLIB).  Other platforms do not have libdispatch and
libBlocksRuntime in their C runtime, so we need to explicitly link against them.
Now that we are building BlocksRuntime with hidden visibility, we do not
accidentally get the symbols from libdispatch.
@compnerd compnerd force-pushed the explicit-BlocksRuntime branch from da22801 to e7bc37b Compare October 3, 2018 20:37
@compnerd
Copy link
Member Author

compnerd commented Oct 3, 2018

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2018

Build failed
Swift Test OS X Platform
Git Sha - da228015b17b1eca42162e9f0e68e41bee31f74f

@swift-ci
Copy link
Contributor

swift-ci commented Oct 3, 2018

Build failed
Swift Test Linux Platform
Git Sha - da228015b17b1eca42162e9f0e68e41bee31f74f

@compnerd compnerd merged commit 7c9b6f0 into swiftlang:master Oct 4, 2018
@compnerd compnerd deleted the explicit-BlocksRuntime branch October 4, 2018 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants