Skip to content

[CMake] Support Macros in Linux #68082

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 12 commits into from
Aug 30, 2023
Merged

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Aug 23, 2023

For compiling codes required for macro support, we now need swiftc compiler in the build machine.

Unlike Darwin OSes, where swiftCore runtime is guaranteed to be present in /usr/lib, Linux doesn't have ABI stability and the stdlib of the build machine is not at the specific location. So the built compiler cannot relies on the shared object in the system.

@rintaro
Copy link
Member Author

rintaro commented Aug 23, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

Comment on lines +254 to +259
add_custom_command(
OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp"
COMMAND ${CMAKE_COMMAND} -E copy_if_different
"${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp.tmp"
"${CMAKE_CURRENT_BINARY_DIR}/HeaderDependencies.cpp"
)
Copy link
Member Author

@rintaro rintaro Aug 23, 2023

Choose a reason for hiding this comment

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

This is to avoid mtime of HeaderDependencies.cpp being updated every time cmake configuration happens.

Comment on lines 551 to 558

# At runtime, use swiftCore in the current toolchain.
# FIXME: This assumes the ABI hasn't changed since the builder.
set(swift_runtime_rpath "$ORIGIN/${relpath_to_lib_dir}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}")

if(ASRLF_BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS")
set(swift_runtime_rpath "${host_lib_dir}")
else()
set(swift_runtime_rpath "$ORIGIN/${relpath_to_lib_dir}/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}")
# But before building the stdlib with the tool, use the builder libs. This should be removed in install time.
list(APPEND swift_runtime_rpath "${host_lib_dir}")
Copy link
Member Author

Choose a reason for hiding this comment

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

RUNPATH is now like $ORIGIN/../lib/swift/linux:/opt/swift/5.8.1/usr/lib/swift/linux
So after the swiftCore is built in the build directory, that's used.

@@ -89,7 +111,8 @@ function(add_swift_unittest test_dirname)
endif()
endif()

if (SWIFT_SWIFT_PARSER)
if (SWIFT_SWIFT_PARSER AND NOT ASU_IS_TARGET_TEST)
# Link to stdlib the compiler uses.
Copy link
Member Author

Choose a reason for hiding this comment

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

e.g. runtime tests is a "target" test, not "host" test. "target" test should not link with host libraries.

@@ -6,7 +6,7 @@ function(swift_supports_implicit_module module_name out_var)
"${CMAKE_Swift_COMPILER}"
-Xfrontend -disable-implicit-${module_name}-module-import
-Xfrontend -parse-stdlib
-c - -o /dev/null
-parse -
Copy link
Member Author

Choose a reason for hiding this comment

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

Linux swiftc reads the output file after compiling (for e.g. autolink-extract). Since /dev/null is not a valid object file, this failed.
All we want to check here is if the frontend accepts the command line option, so -parse should be enough.

Comment on lines -36 to 37
set(SWIFT_SYNTAX_LIBRARIES_SOURCE_DIR
set(SWIFT_SYNTAX_LIBRARIES_BUILD_DIR
"${SWIFT_PATH_TO_EARLYSWIFTSYNTAX_BUILD_DIR}/lib/swift/host")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is really meant to be a "build" directory where earlyswiftsyntax's .so files exist.

Comment on lines 84 to 94
swift_install_in_component(
FILES "${SWIFT_HOST_LIBRARIES_DEST_DIR}/${sharedlib}"
DESTINATION "lib${LLVM_LIBDIR_SUFFIX}/swift/host"
COMPONENT compiler
)

swift_install_file_set_rpath(
"lib${LLVM_LIBDIR_SUFFIX}/swift/host/${sharedlib}"
"$$ORIGIN:$$ORIGIN/../linux"
compiler
)
Copy link
Member Author

Choose a reason for hiding this comment

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

SwiftSyntax libraries (.so and .swiftmodule) are now being installed from swift build directory, but not earlyswiftsyntax build directory. So we can centralize the RUNPATH mutation logic in this repository.

# Add rpath to 'lib/host'
if(SWIFT_HOST_VARIANT_SDK STREQUAL "LINUX")
set_property(TARGET ${name}
APPEND PROPERTY INSTALL_RPATH "$ORIGIN/..")
Copy link
Member Author

Choose a reason for hiding this comment

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

Linux's RUNPATH doesn't inherits executable's RUNPATH. So each .so must have its own complete RUNPATH.

@rintaro rintaro force-pushed the swift-swift-linux branch from 800b056 to 4c10475 Compare August 23, 2023 17:31
@rintaro
Copy link
Member Author

rintaro commented Aug 23, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@finagolfin
Copy link
Member

I notice you are stripping builder runpaths. I haven't look into why that's needed in detail, but maybe it won't be needed if -no-toolchain-stdlib-rpath is passed in so the Swift compiler doesn't set that builder runpath and you set it manually with CMake's BUILD_RPATH instead, which is automatically removed before installation.

That may be easier than removing builder runpaths yourself before installation, as you seem to be doing here? I could be way off since I didn't look deeply into why you're doing this.

@rintaro rintaro force-pushed the swift-swift-linux branch from 4c10475 to 37216ab Compare August 23, 2023 20:57
@rintaro
Copy link
Member Author

rintaro commented Aug 23, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

1 similar comment
@rintaro
Copy link
Member Author

rintaro commented Aug 23, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

For compiling codes required for macro support, we now need swiftc
compiler in the build machine.

Unlike Darwin OSes, where swiftCore runtime is guaranteed to be present
in /usr/lib, Linux doesn't have ABI stability and the stdlib of the
build machine is not at the specific location. So the built compiler
cannot relies on the shared object in the toolchain.
@rintaro rintaro force-pushed the swift-swift-linux branch from 37216ab to 9c9010e Compare August 24, 2023 17:33
@rintaro
Copy link
Member Author

rintaro commented Aug 24, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@bnbarham
Copy link
Contributor

I notice you are stripping builder runpaths. I haven't look into why that's needed in detail, but maybe it won't be needed if -no-toolchain-stdlib-rpath is passed in so the Swift compiler doesn't set that builder runpath and you set it manually with CMake's BUILD_RPATH instead, which is automatically removed before installation.

That may be easier than removing builder runpaths yourself before installation, as you seem to be doing here? I could be way off since I didn't look deeply into why you're doing this.

The TLDR is that we need to cherry-pick this change to 5.9.0 and thus would like as few changes as possible.

-no-toolchain-stdlib-rpath is actually being used for the "pure swift" libs and tools in the compiler, but only so that we can add the host path to the end rather than the start. We could split BUILD/INSTALL_RPATH here rather than using BUILD_WITH_INSTALL_RPATH and then stripping, but that's a much larger change (especially if we want to do it for the non-"pure swift" cases).

We also need to do this for the copied-in "early" swift-syntax libs, which currently aren't part of the swift build itself. Since they're built separately, we can't do the same BUILD/INSTALL_RPATH split. I have a PR up to build swift-syntax in the same tree (#66043), but will get back to that after this is in and 5.9 is released (I ran into some issues with the bootstrapping case, which I haven't had the time to look into). I also have a local patch that does most of the BUILD/INSTALL_RPATH splitting that I'll put up as well.

Once that's all in, I definitely agree that using BUILD/INSTALL_RPATH here rather than the manual stripping is the way to go.

@rintaro
Copy link
Member Author

rintaro commented Aug 24, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

Many shared libs and executables are only run after stdlib/runtime are
built. They don't need to link with builders stdlib at all.
@rintaro rintaro force-pushed the swift-swift-linux branch from 26cc7d5 to 40841b3 Compare August 24, 2023 23:10
@rintaro
Copy link
Member Author

rintaro commented Aug 24, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

and Swift parser integration is enabled.
If swift parser integration is enabled, SwiftSyntax libraries are always
built with host tools. Other SwiftCompilerSources modules must use the
same runtime with parser libraries.
@rintaro rintaro force-pushed the swift-swift-linux branch from 40841b3 to 0f0c492 Compare August 24, 2023 23:32
@rintaro
Copy link
Member Author

rintaro commented Aug 24, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 25, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain Linux Platform

@rintaro
Copy link
Member Author

rintaro commented Aug 25, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain macOS Platform

Comment on lines 75 to 77
get_filename_component(swift_bin_dir ${CMAKE_Swift_COMPILER} DIRECTORY)
get_filename_component(swift_dir ${swift_bin_dir} DIRECTORY)
set(host_lib_dir "${swift_dir}/lib/swift/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these used? I guess this is currently working because ASTGen is statically linked and the pure tools would all use FALSE now anyway, so this function is only used by the plugins. But those aren't used except for in tests and thus host isn't actually needed anywhere that uses this right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are leftover. Just removed.

@@ -744,7 +745,7 @@ function(add_swift_host_library name)
_set_target_prefix_and_suffix(${name} "${libkind}" "${SWIFT_HOST_VARIANT_SDK}")

if (ASHL_SHARED AND ASHL_HAS_SWIFT_MODULES)
_add_swift_runtime_link_flags(${name} "." "")
_add_swift_runtime_link_flags(${name} "." "" FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, maybe required_for_minimal_compiler doesn't make sense as the name. Since these ... are required, it's just they don't have any Swift in them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think any shared (ASHL_SHARED) libraries are required for building stdlib.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is only shared, true. Having said that, if we did move to shared libraries more, it would be the case that they are "required for minimal" but this would still be FALSE because they wouldn't have any Swift. And that was more my point.

Comment on lines 28 to 30
if((NOT SWIFT_SWIFT_PARSER) AND NOT(BOOTSTRAPPING_MODE STREQUAL "HOSTTOOLS"))
return()
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an OR? ie. We just want !(linux && swift_swift_parser && bootstrapping == hosttools) but this is !swift_swift_parser && bootstrapping != hosttools

Copy link
Member Author

Choose a reason for hiding this comment

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

My intent was !(linux && (swift_swift_parser || boostrapping==hosttools)) because boostrapping==hosttools alone also needs to strip builder's RUNPATH.

Copy link
Contributor

Choose a reason for hiding this comment

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

bootstrapping==hosttools without early swift syntax shouldn't have the host in the RUNPATH anyway though right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should, otherwise which runtime does the SwiftCompilerSources library link with (before it builds stdlib/runtime)?

Copy link
Contributor

@bnbarham bnbarham Aug 25, 2023

Choose a reason for hiding this comment

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

Ah true, forgot about SwiftCompilerSources here 😅 Thanks, sorry for the run around!

file(RELATIVE_PATH relative_rtlib_path "${path}" "${SWIFTLIB_DIR}/${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_LIB_SUBDIR}")
list(APPEND RPATH_LIST "$ORIGIN/${relative_rtlib_path}")
# NOTE: SourceKit components are NOT executed before stdlib is built.
# So there's no need to add RUNPATH to builder's runtime libraries.

elseif(ASKD_BOOTSTRAPPING_MODE STREQUAL "BOOTSTRAPPING")
get_bootstrapping_swift_lib_dir(bs_lib_dir "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Lines 169-191 are very odd, given that there is no bootstrapping for SK. I know we're trying to make minimal changes, but IMO we should at least:

  1. Delete 159 which is just plain wrong
  2. Delete the else branch, since we can't get into there for Linux anyway
  3. Remove the bootstrapping check, since it's always empty
  4. Delete 161-165 since that's handled just above this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, (4) adds host. IMO I'd move that to inside the ifs here since the if there is wrong anyway (it should be specifically linux/android/openbsd). So ie.

if(SWIFT_SWIFT_PARSER)
    file(RELATIVE_PATH relative_hostlib_path "${path}" "${SWIFTLIB_DIR}/host")
    list(APPEND RPATH_LIST "$ORIGIN/${relative_hostlib_path}")
endif()

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Done

@@ -26,6 +26,7 @@ if(BOOTSTRAPPING_MODE MATCHES "BOOTSTRAPPING.*")
HAS_SWIFT_MODULES
BOOTSTRAPPING 0
THINLTO_LD64_ADD_FLTO_CODEGEN_ONLY
REQUIRED_FOR_MINIMAL_COMPILER
Copy link
Contributor

Choose a reason for hiding this comment

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

Could skip adding this and the next, since they're only used for bootstrapping which we're not doing.

# Ensure that we can find the host shared libraries.
set_property(
TARGET libSwiftScan
APPEND PROPERTY INSTALL_RPATH "@loader_path/swift/host")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just append them both at once with : but doesn't really matter.

@@ -859,7 +859,6 @@ install-lldb
install-llbuild
install-swiftpm
install-swift-driver
install-swiftsyntax
Copy link
Contributor

Choose a reason for hiding this comment

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

Could just leave this in since the produce is changed. There's a lot of these that would need to be removed anyway.

Comment on lines 892 to 893
# Linux only support 'hosttools' with early-swiftsyntax.
bootstrapping=hosttools
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to need to add this in quite a few places. Maybe we should override and set to hosttools when early swiftsyntax is enabled, since that's the only thing that works? Bit weird but...

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to force setting set(BOOTSTRAPPING_MODE "HOSTTOOLS") when SWIFT_SWIFT_PARSER is enabled.

"""
self.install_with_cmake(["install"], self.host_install_destdir(host_target))
# No-op.
None
Copy link
Contributor

Choose a reason for hiding this comment

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

pass is the Python-ism you want here

@bnbarham
Copy link
Contributor

Thanks Rintaro!

Swift Build Toolchain Ubuntu 20.04 (x86_64) — Done

:O!

SourceKit components are never built while bootstrapping stages. So it
doen't need to have stage 0 or 1 handling.
@rintaro
Copy link
Member Author

rintaro commented Aug 25, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 25, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain

`$ORIGIN` is only supported in Linux, BSD, etc. (i.e. not in Windows)
@rintaro
Copy link
Member Author

rintaro commented Aug 25, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 28, 2023

libSwiftScan is built in 'lib' but installed in 'lib/swift/host' RUNPATH
should have correct '$ORIGIN/../{platform}' to load 'swiftCore' runtime
library.
swift-compatibility-symbols, swift-def-to-strings-converter,
and swift-serialize-diagnostics don't use any Swift modules. But when
SWIFT_SWIFT_PARSER was enabled, they are linked with swiftCore. But
these binaries can be executed before the runtime is being built.
We need to stop them linking with swiftCore.
@rintaro rintaro force-pushed the swift-swift-linux branch from ab0142b to dc68773 Compare August 28, 2023 19:13
@rintaro
Copy link
Member Author

rintaro commented Aug 28, 2023

@rintaro
Copy link
Member Author

rintaro commented Aug 28, 2023

@rintaro rintaro changed the title [WIP][CMake] Support Macros in Linux [CMake] Support Macros in Linux Aug 28, 2023
@rintaro rintaro marked this pull request as ready for review August 28, 2023 21:52
@rintaro
Copy link
Member Author

rintaro commented Aug 28, 2023

swiftlang/llvm-project#7331
swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain Windows platform

RPATH_SET is not available until cmake 3.21.0. Use RPATH_CHANGE instead.
@rintaro rintaro force-pushed the swift-swift-linux branch from b4e7d09 to f645069 Compare August 29, 2023 01:12
@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain

2 similar comments
@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain

@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain

Some non-stdlib thing e.g. swift-backtrace still might be built before
the runtime is built. For building Swift code in Linux "hosttools",
always set 'LD_LIBRARY_PATH' to the runtime in the builder.
@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please Build Toolchain

@rintaro
Copy link
Member Author

rintaro commented Aug 29, 2023

swiftlang/swift-syntax#2090
@swift-ci Please smoke test

@bnbarham bnbarham merged commit 0988236 into swiftlang:main Aug 30, 2023
@finagolfin
Copy link
Member

I see a handful of runpath issues with the last CI-built toolchain for linux after this pull, I'll look into the fixes:

> ./usr/bin/lldb-server
./usr/bin/lldb-server: error while loading shared libraries: libswiftCore.so: cannot open shared object file: No such file or directory
> readelf -d ./usr/bin/lldb-server |ag runpath
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:$ORIGIN/swift/linux:$ORIGIN/../lib/swift/host]

That second relative path is wrong, hence the runtime error.

@bnbarham
Copy link
Contributor

bnbarham commented Aug 30, 2023

elseif(SWIFT_HOST_VARIANT_SDK MATCHES "LINUX|ANDROID|OPENBSD|FREEBSD")
set_property(
TARGET libSwiftScan
APPEND PROPERTY INSTALL_RPATH "$ORIGIN/swift/host")
Copy link
Member

@finagolfin finagolfin Aug 30, 2023

Choose a reason for hiding this comment

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

@rintaro, is this needed for some intermediate build step or something? Because the final installed library also has this path, which is wrong:

> readelf -d ./usr/lib/swift/host/lib_InternalSwiftScan.so |ag runpath
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN:$ORIGIN/./swift/linux:$ORIGIN/../linux:$ORIGIN/swift/host:$ORIGIN/../host]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, see the many FIXME above. This all needs to be split into BUILD/INSTALL_RPATH + built into host rather than into lib, but that's too much for 5.9. We'll fix this up after all the 5.9 PRs are in as well. We could remove it, but it doesn't really hurt either.

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.

4 participants