SR-7724: Build SourceKit libdispatch into its own directory #19640

kevints · 2018-10-01T16:26:20Z

This is a partial fix for https://bugs.swift.org/browse/SR-7724. It changes the Swift build system to place the libdispatch that SourceKit is linked to in a different directory. This is still not ideal (SourceKit should link the same version of dispatch swift apps will link at runtime) but there's a dependency cycle between swift and corelibs-libdispatch (at least the way build-script is currently structured). A future change can reduce the number of dispatch build directories from 2 to 1. This change is still valuable because it restores the ability to incrementally build libdispatch when calling build-script.

Partially resolves SR-7724.

This is a partial fix for https://bugs.swift.org/browse/SR-7724

kevints · 2018-10-01T16:32:56Z

@compnerd does this overlap with #19639?

compnerd · 2018-10-01T16:38:20Z

mmm, no, it doesn't really overlap with it, but does conflict with it. I'm not sure I understand the cycle here though. If libdispatch is built here, it is actually built after the standard library, but before SourceKit, so there is a total ordering which does allow it to be built just once. Can you explain how build-script breaks that?

kevints · 2018-10-01T16:47:10Z

@compnerd build-script always calls rm -fr on the libdispatch build dir when it's asked to build libdispatch (with --libdispatch). It also configures it with different flags into the same build dir. So the usual build order on a clean checkout on Linux is currently:

0. Build Swift prereqs (LLVM, Clang, etc)
1. Configure Swift
2. Build swift stdlib
3. Configure libdispatch using ExternalProject_Add, build it linked to swift stdlib
4. Build SourceKit linked to libdispatch
5. Finish building Swift tools
6. build-script deletes the libdispatch build dir that the swift build created
7. build-script configures libdispatch using build-script logic
8. build-script builds libdispatch
9. Install all binaries

So SourceKit winds up linked to a libdispatch that was configured by CMake ExternalProject, which is not included in the package but instead blown away and rebuilt by build-script. This PR doesn't change that, but creates a separate build directory for the sourcekit-libdispatch so that build-script can drop the rm -fr.

Does that make sense?

compnerd · 2018-10-01T17:13:50Z

Sure, I get that. @gottesmm - do you remember why we had to restore the rm -rf here? I think that the problem may have been that it doesn't reconfigure on a stdlib change? If so, we just need to track that dependency better rather than doing two builds. Doing something like a psuedo-dependency on the MD5 sum should be sufficient. I think that would be a much better approach. We should wire up any parameters for the libdispatch configuration down into the internal configuration.

gottesmm · 2018-10-01T17:20:27Z

@compnerd Question, isn't that something that is fixed by using @llvm-beanz's stuff from LLVM that wraps external project? Swift depends on LLVM's cmake. It should be ok to use those and if they solve the dependency stuff for us even better.

kevints · 2018-10-01T17:48:02Z

@compnerd I don't understand how this change conflicts with #19639. Is the goal that ExternalProject subsumes build-script? SourceKit without stdlib or swiftDispatch seems like an uncommon build configuration to support compared to SourceKit+stdlib+swiftDispatch. Right now sourcekit will be linked against a differently-configured libdispatch than the one that's ultimately installed on the system

compnerd · 2018-10-01T18:04:25Z

@gottesmm - hmm, possibly. In either case, we should look at wrapping up LLVM's ExternalProject wrapper as it would simplify the CMake logic in swift.

@kevints - yes, the longer term plan is to remove build-script entirely and have CMake orchestrate the builds with inter-project dependency tracking. The last piece I think that is left is my work to switch s-c-f to CMake from the python based build. Once that is done, I believe that all the components can be wired up using CMake.

kevints · 2018-10-01T18:25:39Z

@compnerd got it. any reason we can't use a separate build directory for sourcekit-dispatch while we still have 2 builds though?

gottesmm · 2018-10-01T18:26:35Z

@kevints @compnerd I don't think that the long term plan is to get of build-script entirely. I do think that there is a place for build-script to manage presets. But the low level driving of the build should be entirely cmake IMO.

compnerd · 2018-10-01T19:27:03Z

tools/SourceKit/CMakeLists.txt

+                          <INSTALL_DIR>/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}/${CMAKE_SHARED_LIBRARY_PREFIX}dispatch${CMAKE_SHARED_LIBRARY_SUFFIX}
+                          <INSTALL_DIR>/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}/${SWIFT_HOST_VARIANT_ARCH}/Dispatch.swiftmodule
+                          <INSTALL_DIR>/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}/${SWIFT_HOST_VARIANT_ARCH}/Dispatch.swiftdoc
+                        INSTALL_COMMAND ${CMAKE_COMMAND} -E echo "Finished building libdispatch for SourceKit"


If you want to have an echo that occurs afterwards, please keep the installation in the same command. I don't think that we should have the build install the components.

I should have mentioned this strange construct in a comment - cmake 3.5.3 doesn't have an INSTALL_BYPRODUCTS list for ExternalProject_Add so file-level dependencies don't work as they should with ninja (I want to depend on the "installed" location of libdispatch rather than the built one). But it also doesn't like having an empty INSTALL_COMMAND (which I why I added the echo here).

I ran into issues with the dependencies IIRC when I used the installed artifacts. But, if it works, sure, that is fine. However, according to the documentation, https://cmake.org/cmake/help/v3.5/module/ExternalProject.html, 3.5 does include support for BUILD_BYPRODUCTS.

compnerd · 2018-10-01T19:27:48Z

tools/SourceKit/cmake/modules/AddSwiftSourceKit.cmake

-    # NOTE(compnerd) since we do not use target_link_libraries, we do not get
-    # the implicit dependency tracking.  Add an explicit dependency until we can
-    # use target_link_libraries.
-    add_dependencies(${target} BlocksRuntime)


Not sure if this should be removed. This should be adjusted for the new layout right? Or am I missing something?

BlocksRuntime is statically linked into libdispatch already so it's not correct to link BlocksRuntime again, this build should be linking dispatch to get BlocksRuntime symbols.

$ readelf --dyn-syms -W swift-4.2-RELEASE-ubuntu14.04/usr/lib/swift/linux/libdispatch.so | grep -E '(Block_copy|Block_release)' 605: 0000000000060220 196 FUNC GLOBAL DEFAULT 11 _Block_release 740: 00000000000600f0 7 FUNC GLOBAL DEFAULT 11 _Block_copy 853: 0000000000060340 201 FUNC GLOBAL DEFAULT 11 _Block_copy_collectable

Yes, it is statically linked, but the symbols are not supposed to be re-exported. This is something that I can look into fixing: see #17451 and swiftlang/swift-corelibs-libdispatch#362. This actually is a bigger issue where the blocks usage breaks down :-(. Seems like I made a mistake in the libdispatch build? I'll take a look at that as well.

There should only be one copy of BlocksRuntime in the entire program. For example dispatch_async calls Block_copy and Block_release on the blocks that are passed to it. A program can also call these functions on the same block objects you pass to dispatch_async; if they are different you risk undefined behavior. So BlocksRuntime is really a public part of the libdispatch ABI and statically linking it into dispatch re-exporting its symbols isn't possible.

Agreed that there should be a single copy, which is why we need the shared build of it for other targets as well and that was the motivation for the original change that I got distracted from. I suppose it is high time that I revisit that.

I really think that exporting it as part of libdispatch is fundamentally the wrong approach. You can use block with C, and in such a scenario, you shouldn't pull in libdispatch. I think it is better to actually fix the build so that we build BlocksRuntime shared and use that appropriately.

compnerd · 2018-10-01T19:27:56Z

@gottesmm, I think that we could do that with CMake caches (it works great!), but thats a separate consideration. @kevints, I think that I can see an argument for two different builds - one is the tool host and one is the target. So, I think that if we want to do this in a separate directory, that does make some sense, but then we should configure it the same was as the host tools.

compnerd · 2018-10-01T23:00:49Z

tools/SourceKit/CMakeLists.txt

@@ -96,28 +96,33 @@ if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
    ExternalProject_Add(libdispatch
                        SOURCE_DIR
                          "${SWIFT_PATH_TO_LIBDISPATCH_SOURCE}"
-                        BINARY_DIR
-                          "${SWIFT_PATH_TO_LIBDISPATCH_BUILD}"
                        CMAKE_ARGS
                          -DCMAKE_C_COMPILER=${PATH_TO_CLANG_BUILD}/bin/clang
                          -DCMAKE_CXX_COMPILER=${PATH_TO_CLANG_BUILD}/bin/clang++


Actually, if we know that this build is for the host, we should also use the same compiler that we used to build swiftc no?

SourceKit is logically a part of the toolchain, so we should use the same build settings for it. This means (I think) that we should use the host libdispatch if one is available or build a toolchain-specific copy. This copy would need to be installed somewhere that it doesn't conflict with the runtime's copy in the common case that the host and target are the same, so disabling the swift overlay in this build seems like a non-starter.

This complexity isn't present on Darwin platforms as the swift overlay for Dispatch is built into a separate library (libswiftDispatch.dylib). Perhaps we should explore this approach for Linux as well, since that would allow us to have libsourcekitdInProc.so, libdispatch.so and libswiftDispatch.so installed side-by-side with SourceKit having no dependency on the swift runtime.

Yes, thinking more about this, I think that you are correct in that we need a libdispatch for the toolchain.

I definitely agree on the libdispatch swift SDK overlay being split out on Linux and Windows much like Darwin. This gives us uniform behaviour as well as gives us the ability to break some of that cycle.

compnerd · 2018-10-01T23:13:38Z

tools/SourceKit/CMakeLists.txt

                          INTERFACE_INCLUDE_DIRECTORIES
                            ${install_dir}/include
                          IMPORTED_LINK_INTERFACE_LIBRARIES
                            swiftCore-${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}-${SWIFT_HOST_VARIANT_ARCH})

-  add_library(BlocksRuntime STATIC IMPORTED)


The removal of BlocksRuntime is actually problematic for Windows builds, since we need that to enable SourceKit builds as well. Ideally, we would use a shared BlocksRuntime, but, that can be handled separately.

I still can't test on Windows, but at least on Linux the implementation of BlocksRuntime seems to live in libdispatch, so SourceKit should link that:

$ readelf --dyn-syms -W swift-4.2-RELEASE-ubuntu14.04/usr/lib/swift/linux/libdispatch.so | grep -E '(Block_copy|Block_release)' 605: 0000000000060220 196 FUNC GLOBAL DEFAULT 11 _Block_release 740: 00000000000600f0 7 FUNC GLOBAL DEFAULT 11 _Block_copy 853: 0000000000060340 201 FUNC GLOBAL DEFAULT 11 _Block_copy_collectable

This is a bug :-) It shouldn't be exposed from libdispatch. The static linkage shouldnt expose the symbols (which is part of the problem) - you cant actually use blocks without the shared runtime as John and I discussed on swiftlang/swift-corelibs-libdispatch#362. I think that I'm going to try to get a couple of my patches up and then look into that again.

I'm not sure you can - for the simple reason that you can call Block_copy and Block_release from outside of dispatch on objects you pass to dispatch. @rjmccall 's statement in
swiftlang/swift-corelibs-libdispatch#362 (comment) seems a far cry from "yes, it definitely works". AIUI, at best it might work if the implementations the program and the dispatch library can see are ABI compatible, but the best way to ensure that IMO is just to use the same definition in the first place.

Right, the same definition would be through a shared library (which is what the PR was working towards). I'm just saying that in the mean time, statically linking against it should get you unblocked right?

compnerd · 2018-10-02T17:13:16Z

FYI, after seeing this PR and thinking more about it, I ended up going down the same route as you to enable SourceKit on Windows, but, I think that I want to address the BlocksRuntime issue as well. If we get the BlocksRuntime built with hidden visibility and linked in, would it unblock you? I do realize that it is broken in terms of support, but, I think that fixing BlocksRuntime to be shared is the right approach for that, and we can look at that separate of this PR. I'm just trying to determine if you want to help look at that first or if you want to try to get this change patched up and merged first.

kevints · 2018-10-03T17:26:02Z

Closing this in favor of #19674 after offline discussion with @compnerd and @gottesmm

SR-7724: Build SourceKit libdispatch into its own directory

9832461

This is a partial fix for https://bugs.swift.org/browse/SR-7724

kevints mentioned this pull request Oct 1, 2018

Build both shared and static libraries when ENABLE_SWIFT is ON swiftlang/swift-corelibs-libdispatch#393

Closed

compnerd reviewed Oct 1, 2018

View reviewed changes

This was referenced Oct 2, 2018

SourceKit: explicitly link against BlocksRuntime as well #19668

Merged

build: give BlocksRuntime hidden visibility swiftlang/swift-corelibs-libdispatch#394

Closed

kevints closed this Oct 3, 2018

kevints deleted the SR-7724 branch October 3, 2018 17:26

This was referenced Oct 1, 2018

[SR-7724] libdispatch gets built twice on Linux when sourcekit is enabled swiftlang/swift-corelibs-libdispatch#654

Open

[SR-8897] libdispatch is underlinked - it does not link libswiftCore swiftlang/swift-corelibs-libdispatch#648

Closed

SR-7724: Build SourceKit libdispatch into its own directory #19640

SR-7724: Build SourceKit libdispatch into its own directory #19640

Uh oh!

Conversation

kevints commented Oct 1, 2018

Uh oh!

kevints commented Oct 1, 2018

Uh oh!

compnerd commented Oct 1, 2018

Uh oh!

kevints commented Oct 1, 2018

Uh oh!

compnerd commented Oct 1, 2018

Uh oh!

gottesmm commented Oct 1, 2018

Uh oh!

kevints commented Oct 1, 2018

Uh oh!

compnerd commented Oct 1, 2018

Uh oh!

kevints commented Oct 1, 2018

Uh oh!

gottesmm commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Oct 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Oct 2, 2018

Uh oh!

kevints commented Oct 3, 2018

Uh oh!

Uh oh!

compnerd Oct 2, 2018 •

edited

Loading