Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions tools/SourceKit/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DCMAKE_SWIFT_COMPILER=$<TARGET_FILE:swift>c
-DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
-DENABLE_SWIFT=YES
-DENABLE_TESTING=NO
BUILD_COMMAND
${CMAKE_COMMAND} --build <BINARY_DIR> --target install
BUILD_BYPRODUCTS
${SWIFT_PATH_TO_LIBDISPATCH_BUILD}/src/${CMAKE_SHARED_LIBRARY_PREFIX}dispatch${CMAKE_SHARED_LIBRARY_SUFFIX}
${SWIFT_PATH_TO_LIBDISPATCH_BUILD}/${CMAKE_STATIC_LIBRARY_PREFIX}BlocksRuntime${CMAKE_STATIC_LIBRARY_SUFFIX}
<BINARY_DIR>/src/${CMAKE_SHARED_LIBRARY_PREFIX}dispatch${CMAKE_SHARED_LIBRARY_SUFFIX}
<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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

STEP_TARGETS
configure
BUILD_ALWAYS
1)
configure build
)

# CMake does not like the addition of INTERFACE_INCLUDE_DIRECTORIES without
# the directory existing. Just create the location which will be populated
# during the installation.
ExternalProject_Get_Property(libdispatch install_dir)
file(MAKE_DIRECTORY ${install_dir}/include)
ExternalProject_Get_Property(libdispatch binary_dir)

# TODO(compnerd) this should be taken care of by the
# INTERFACE_INCLUDE_DIRECTORIES below
Expand All @@ -138,20 +143,12 @@ if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
set_target_properties(dispatch
PROPERTIES
IMPORTED_LOCATION
${SWIFT_PATH_TO_LIBDISPATCH_BUILD}/src/${CMAKE_SHARED_LIBRARY_PREFIX}dispatch${CMAKE_SHARED_LIBRARY_SUFFIX}
${install_dir}/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}/${CMAKE_SHARED_LIBRARY_PREFIX}dispatch${CMAKE_SHARED_LIBRARY_SUFFIX}
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

set_target_properties(BlocksRuntime
PROPERTIES
IMPORTED_LOCATION
${SWIFT_PATH_TO_LIBDISPATCH_BUILD}/${CMAKE_STATIC_LIBRARY_PREFIX}BlocksRuntime${CMAKE_STATIC_LIBRARY_SUFFIX}
INTERFACE_INCLUDE_DIRECTORIES
${SWIFT_PATH_TO_LIBDISPATCH_SOURCE}/src/BlocksRuntime)

set(SOURCEKIT_NEED_EXPLICIT_LIBDISPATCH TRUE)
endif()

Expand Down
9 changes: 0 additions & 9 deletions tools/SourceKit/cmake/modules/AddSwiftSourceKit.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,6 @@ function(add_sourcekit_default_compiler_flags target)
# TODO(compnerd) this should really use target_compile_options but the use
# of keyword and non-keyword flags prevents this
list(APPEND c_compile_flags "-fblocks")
# TODO(compnerd) this should really use target_link_libraries but the use of
# explicit_llvm_config using target_link_libraries without keywords on
# executables causes conflicts here
list(APPEND link_flags "-L${SWIFT_PATH_TO_LIBDISPATCH_BUILD}")
list(APPEND link_flags "-lBlocksRuntime")
# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@compnerd compnerd Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endif()

# Convert variables to space-separated strings.
Expand Down
5 changes: 0 additions & 5 deletions utils/build-script-impl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2561,11 +2561,6 @@ for host in "${ALL_HOSTS[@]}"; do
continue
;;
*)
# FIXME: Always re-build libdispatch on non-darwin platforms.
# Remove this when products build in the CMake system.
echo "Cleaning the libdispatch build directory"
call rm -rf "${LIBDISPATCH_BUILD_DIR}"

cmake_options=(
${cmake_options[@]}
-DCMAKE_BUILD_TYPE:STRING="${LIBDISPATCH_BUILD_TYPE}"
Expand Down