CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` value #65795

MaxDesiatov · 2023-05-09T10:18:12Z

SWIFT_CONCURRENCY_GLOBAL_EXECUTOR is defined in stdlib/cmake/modules/StdlibOptions.cmake, which is not included during the first pass of evaluation of the root CMakeLists.txt. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where usr/lib/swift_static/linux/static-stdlib-args.lnk didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds.

Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux.

Additionally, since are trivial tests previously didn't link Dispatch statically, the didn't expose a bug where %import-static-libdispatch substitution had a missing value. To fix that I had to update lit.cfg and clean up some of the related path computations to infer a correct substitution value.

Resolves some of the errors reported in #65097.

`SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` is defined in `stdlib/cmake/modules/StdlibOptions.cmake`, which is not included during the first pass of evaluation of the root `CMakeLists.txt`. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where `usr/lib/swift_static/linux/static-stdlib-args.lnk` didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds. Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux.

MaxDesiatov · 2023-05-09T10:18:24Z

@swift-ci test

MaxDesiatov · 2023-05-09T10:18:53Z

@swift-ci build toolchain

MaxDesiatov · 2023-05-09T10:19:34Z

Running a first pass of tests before running cross-repo integration tests...

al45tair · 2023-05-09T10:49:12Z

Potentially resolves #65097.

Isn't this going to auto-close that issue? So either it does resolve it, or we should adjust the message wording here?

MaxDesiatov · 2023-05-09T10:51:21Z

I'm not merging this one until we have appropriate integration tests enabled and passing, will update the wording after that.

MaxDesiatov · 2023-05-10T13:06:23Z

@swift-ci test

MaxDesiatov · 2023-05-10T13:06:57Z

@swift-ci build toolchain

MaxDesiatov · 2023-05-10T13:14:26Z

test/lit.cfg

-        make_path(libdispatch_static_artifact_dir, 'src', 'swift', 'libswiftDispatch.a'),
-        make_path(libdispatch_swift_static_module_dir, 'Dispatch.swiftmodule')]


We don't need a Swift overlay for Dispatch to statically linked a simple executable, Dispatch is used directly without an overlay from the concurrency runtime.

MaxDesiatov · 2023-05-10T13:14:51Z

test/lit.cfg

@@ -1593,19 +1593,13 @@ elif (run_os in ['linux-gnu', 'linux-gnueabihf', 'freebsd', 'openbsd', 'windows-
        config.import_libdispatch = ('-I %s -I %s -L %s'
            % (libdispatch_source_dir, libdispatch_swift_module_dir, libdispatch_artifact_dir))

-    libdispatch_static_artifact_dir = config.libdispatch_static_build_path
-    libdispatch_swift_static_module_dir = make_path(libdispatch_static_artifact_dir, 'src', 'swift', 'swift')


Overlay path deleted as unused.

MaxDesiatov · 2023-05-10T13:15:22Z

test/lit.cfg

@@ -1593,19 +1593,13 @@ elif (run_os in ['linux-gnu', 'linux-gnueabihf', 'freebsd', 'openbsd', 'windows-
        config.import_libdispatch = ('-I %s -I %s -L %s'
            % (libdispatch_source_dir, libdispatch_swift_module_dir, libdispatch_artifact_dir))

-    libdispatch_static_artifact_dir = config.libdispatch_static_build_path
-    libdispatch_swift_static_module_dir = make_path(libdispatch_static_artifact_dir, 'src', 'swift', 'swift')
+    libdispatch_static_artifact_dir = os.path.join(config.libdispatch_static_build_path, 'lib')


Added a lib component for easier reuse of this variable in subsequent expressions below this line.

MaxDesiatov · 2023-05-10T13:15:44Z

test/lit.cfg

    libdispatch_static_artifacts = [
-        make_path(libdispatch_static_artifact_dir, 'src', 'libdispatch.a'),


Built artifacts are located in lib, not src.

MaxDesiatov · 2023-05-10T13:16:26Z

test/lit.cfg

-        config.import_libdispatch_static = ('-I %s -I %s -L %s -L %s -L %s'
-            % (libdispatch_source_dir, libdispatch_swift_static_module_dir,
-            make_path(libdispatch_static_artifact_dir, 'src'),
-            make_path(libdispatch_static_artifact_dir, 'src', 'BlocksRuntime'),
-            make_path(libdispatch_static_artifact_dir, 'src', 'swift')))
+        config.import_libdispatch_static = '-L %s' % libdispatch_static_artifact_dir


Since neither the overlay nor the headers are used for static linking, this is simplified to a single library search path flag.

MaxDesiatov · 2023-05-10T13:17:53Z

test/lit.cfg

+# WARNING: the order of components in a substitution name has to be different from the previous one, as lit does
+# a pure string substitution without understanding that these components are grouped together. That is, the following
+# subsitution name can't be `%import-libdispatch-static`, otherwise the first two components will be substituted with
+# the value of `%import-libdispatch` substitution with `-static` string appended to it.
 config.substitutions.append(('%import-static-libdispatch', getattr(config, 'import_libdispatch_static', '')))


While working on this fix I attempted to rename this to %import-libdispatch-static for consistency, which led to a hard to debug test failure due to incorrect flags passed. Added a note about this substitution behavior for posterity.

MaxDesiatov · 2023-05-10T13:18:37Z

utils/build-script-impl

@@ -1901,7 +1901,7 @@ for host in "${ALL_HOSTS[@]}"; do
                    -DSWIFT_PATH_TO_CMARK_BUILD:PATH="$(build_directory ${host} cmark)"
                    -DSWIFT_PATH_TO_LIBDISPATCH_SOURCE:PATH="${LIBDISPATCH_SOURCE_DIR}"
                    -DSWIFT_PATH_TO_LIBDISPATCH_BUILD:PATH="$(build_directory ${host} libdispatch)"
-                    -DSWIFT_PATH_TO_LIBDISPATCH_STATIC_BUILD:PATH="$(build_directory ${host} libdispatch_static)"
+                    -DSWIFT_PATH_TO_LIBDISPATCH_STATIC_BUILD:PATH="$(build_directory ${host} swift)/$(basename $(build_directory ${host} libdispatch))-static-prefix"


Dispatch built as a dependency of the Swift compiler is located in the Swift build subdirectory, not in its own directory at the top level of the build. It also has that -static-prefix suffix (sic), which I haven't found where it's coming from, seems like it isn't passed to build-script-impl itself, so I'm hardcoding that suffix here for now.

IIRC, this is the dispatch built for the concurrency runtime. I think this will work for this use-case, but it is probably worth noting.

For concurrency runtime and SourceKit IIUC

MaxDesiatov · 2023-05-10T14:51:43Z

swiftlang/swift-integration-tests#78

@swift-ci build toolchain linux

MaxDesiatov · 2023-05-10T16:17:40Z

@swift-ci build toolchain

tomerd · 2023-05-10T17:47:26Z

seems reasonable to me. should any of this be separated into a dedicated "cleanup" PR, or do they all need to be done together?

MaxDesiatov · 2023-05-10T17:49:41Z

They have to go together otherwise the Linux test suite will fail, like it did for 5897ab5 in the branch of this PR.

tomerd · 2023-05-10T17:53:11Z

I meant the other order, ie first do the cleanup then fix the static linking initialization issue. but not a big deal either way

MaxDesiatov · 2023-05-10T17:58:11Z

AFAIU this repository prefers merge commits, so commit separation will be preserved after the PR is merged. If having separate PRs would make the review easier I can split it, but the diff seems relatively small to me from that perspective.

drexin · 2023-05-10T18:10:03Z

CMakeLists.txt

@@ -202,6 +202,10 @@ set(SWIFT_HOST_VARIANT_ARCH "${SWIFT_HOST_VARIANT_ARCH_default}" CACHE STRING
 # This is primarily to support building smaller or faster project files.
 #

+# Subsequent options may refer to `StdlibOptions`, which have to be defined first.
+list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/stdlib/cmake/modules)
+include(StdlibOptions)


Instead of including StdlibOptions here, why don't we move the following into StdlibOptions?

# Use dispatch as the system scheduler by default. # For convenience, we set this to false when concurrency is disabled. set(SWIFT_CONCURRENCY_USES_DISPATCH FALSE) if(SWIFT_ENABLE_EXPERIMENTAL_CONCURRENCY AND "${SWIFT_CONCURRENCY_GLOBAL_EXECUTOR}" STREQUAL "dispatch") set(SWIFT_CONCURRENCY_USES_DISPATCH TRUE) endif()

AFAICT it is only used in the stdlib build, so really has no business being set in the root CMakeLists.txt

^ Was just thinking that.

Good point, that's addressed now.

It is only used in the stdlib build, so really has no business being set in the root `CMakeLists.txt`.

MaxDesiatov · 2023-05-10T18:38:18Z

CMakeLists.txt

-set(SWIFT_CONCURRENCY_USES_DISPATCH FALSE)
-if(SWIFT_ENABLE_EXPERIMENTAL_CONCURRENCY AND "${SWIFT_CONCURRENCY_GLOBAL_EXECUTOR}" STREQUAL "dispatch")
-  set(SWIFT_CONCURRENCY_USES_DISPATCH TRUE)
-endif()


Moved to stdlib/cmake/modules/StdlibOptions.cmake

MaxDesiatov · 2023-05-10T18:39:03Z

@swift-ci test

etcwilde

This is a fair bit better.

The sourcekit should be a misnomer coming from our build-soup (don't have to fix it now). We're not building dispatch for the "host", we're building it for the "target", so it may not work in a cross-compiling situation.

MaxDesiatov · 2023-05-10T19:20:43Z

@swift-ci build toolchain

MaxDesiatov · 2023-05-10T22:14:49Z

@swift-ci test linux

@al45tair

[5.9] CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` Explanation: Resolves issues with static linking on Linux Risk: Medium, affects Linux builds and top-level CMake declarations. Original PR: #65795. Reviewed by: @al45tair @drexin @etcwilde Resolves: some of the issues reported in #65097 Tests: Added in swiftlang/swift-integration-tests#115 `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` is defined in `stdlib/cmake/modules/StdlibOptions.cmake`, which is not included during the first pass of evaluation of the root `CMakeLists.txt`. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where `usr/lib/swift_static/linux/static-stdlib-args.lnk` didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds. Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux. Additionally, since are trivial tests previously didn't link Dispatch statically, the didn't expose a bug where `%import-static-libdispatch` substitution had a missing value. To fix that I had to update `lit.cfg` and clean up some of the related path computations to infer a correct substitution value. Resolves some of the errors reported in #65097.

@al45tair

[5.8] CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` Explanation: Resolves issues with static linking on Linux Risk: Medium, affects Linux builds and top-level CMake declarations. Original PRs: #65795 and #64312 for `main`, #65824 and #64633 for `release/5.9` Reviewed by: @al45tair @drexin @etcwilde Resolves: some of the issues reported in #65097, also resolves #58380 Tests: Added in swiftlang/swift-integration-tests#118 `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` is defined in `stdlib/cmake/modules/StdlibOptions.cmake`, which is not included during the first pass of evaluation of the root `CMakeLists.txt`. It is available on subsequent evaluations after the value is stored in CMake cache. This led to subtle bugs, where `usr/lib/swift_static/linux/static-stdlib-args.lnk` didn't contain certain flags on clean toolchain builds, but did contain them in incremental builds. Not having these autolinking flags in toolchain builds leads to errors when statically linking executables on Linux. Additionally, since our trivial lit tests previously didn't link Dispatch statically, they didn't expose a bug where `%import-static-libdispatch` substitution had a missing value. To fix that I had to update `lit.cfg` and clean up some of the related path computations to infer a correct substitution value.

MaxDesiatov requested review from drexin, etcwilde and al45tair May 9, 2023 10:19

al45tair approved these changes May 9, 2023

View reviewed changes

Fix -DSWIFT_PATH_TO_LIBDISPATCH_STATIC_BUILD value

2db62ca

MaxDesiatov requested a review from artemcm as a code owner May 10, 2023 13:05

MaxDesiatov commented May 10, 2023

View reviewed changes

MaxDesiatov mentioned this pull request May 10, 2023

[5.9] CMake: fix missing SWIFT_CONCURRENCY_GLOBAL_EXECUTOR #65824

Merged

drexin requested changes May 10, 2023

View reviewed changes

Move SWIFT_CONCURRENCY_USES_DISPATCH to StdlibOptions.cmake

5275255

It is only used in the stdlib build, so really has no business being set in the root `CMakeLists.txt`.

MaxDesiatov requested a review from drexin May 10, 2023 18:37

Remove redundant include(StdlibOptions)

a25432d

MaxDesiatov commented May 10, 2023

View reviewed changes

drexin approved these changes May 10, 2023

View reviewed changes

etcwilde approved these changes May 10, 2023

View reviewed changes

MaxDesiatov merged commit 0fc3659 into main May 17, 2023

MaxDesiatov deleted the maxd/fix-missing-cmake-value branch May 17, 2023 19:52

MaxDesiatov mentioned this pull request May 31, 2023

[5.8] CMake: fix missing SWIFT_CONCURRENCY_GLOBAL_EXECUTOR #66246

Merged

gwynne mentioned this pull request Jun 4, 2023

Reordering autolink-extract runtime orderings #65375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` value #65795

CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` value #65795

MaxDesiatov commented May 9, 2023 •

edited

Loading

MaxDesiatov commented May 9, 2023

MaxDesiatov commented May 9, 2023

MaxDesiatov commented May 9, 2023

al45tair commented May 9, 2023

MaxDesiatov commented May 9, 2023 •

edited

Loading

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023 •

edited

Loading

etcwilde May 10, 2023

MaxDesiatov May 10, 2023 •

edited

Loading

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

tomerd commented May 10, 2023

MaxDesiatov commented May 10, 2023 •

edited

Loading

tomerd commented May 10, 2023

MaxDesiatov commented May 10, 2023

drexin May 10, 2023

etcwilde May 10, 2023

MaxDesiatov May 10, 2023

MaxDesiatov May 10, 2023 •

edited

Loading

MaxDesiatov commented May 10, 2023

etcwilde left a comment

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

		make_path(libdispatch_static_artifact_dir, 'src', 'swift', 'libswiftDispatch.a'),
		make_path(libdispatch_swift_static_module_dir, 'Dispatch.swiftmodule')]

		libdispatch_static_artifacts = [
		make_path(libdispatch_static_artifact_dir, 'src', 'libdispatch.a'),

CMake: fix missing SWIFT_CONCURRENCY_GLOBAL_EXECUTOR value #65795

CMake: fix missing SWIFT_CONCURRENCY_GLOBAL_EXECUTOR value #65795

Conversation

MaxDesiatov commented May 9, 2023 • edited Loading

MaxDesiatov commented May 9, 2023

MaxDesiatov commented May 9, 2023

MaxDesiatov commented May 9, 2023

al45tair commented May 9, 2023

MaxDesiatov commented May 9, 2023 • edited Loading

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov May 10, 2023 • edited Loading

Choose a reason for hiding this comment

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

tomerd commented May 10, 2023

MaxDesiatov commented May 10, 2023 • edited Loading

tomerd commented May 10, 2023

MaxDesiatov commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov May 10, 2023 • edited Loading

Choose a reason for hiding this comment

MaxDesiatov commented May 10, 2023

etcwilde left a comment

Choose a reason for hiding this comment

MaxDesiatov commented May 10, 2023

MaxDesiatov commented May 10, 2023

CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` value #65795

CMake: fix missing `SWIFT_CONCURRENCY_GLOBAL_EXECUTOR` value #65795

MaxDesiatov commented May 9, 2023 •

edited

Loading

MaxDesiatov commented May 9, 2023 •

edited

Loading

MaxDesiatov May 10, 2023 •

edited

Loading

MaxDesiatov May 10, 2023 •

edited

Loading

MaxDesiatov commented May 10, 2023 •

edited

Loading

MaxDesiatov May 10, 2023 •

edited

Loading