[static][stdlib] Any ELF platform can build static binaries. #32736
Conversation
Instead of copying a static file from the repository, do what the cmakefile already does for static-stdlib-args.lnk and generate it dynamically. This means we can utilize the libicu flags set from the prior commit rather than requiring libicu be built as part of the Swift build just to ensure that the static link flags are correct.
|
Thanks for moving swift to BSD world further. Do you have plan to support FreeBSD as well? |
| @@ -1,3 +1,4 @@ | |||
| // UNSUPPORTED: static_stdlib | |||
There was a problem hiding this comment.
This is not obvious, could you add a comment explaining why it is not supported? (Here and in every other test, please.)
There was a problem hiding this comment.
Done. Since the explanation is somewhat wordy, I've created a Jira bug for the UNSUPPORTED annotations and referenced to the bug.
Do you think the (unrelated) XFAIL elsewhere needs a comment as well?
3405691582
force-pushed
the
MoreStaticBinaries
branch
from
1cf411d to
67c1479
Compare
|
cc @spevans, who may have some context on the splitting issue. |
|
@mko-io: I have sent you a pm on forums.swift.org. |
stdlib/public/runtime/CMakeLists.txt
Outdated
| else() | ||
| find_package(ICU REQUIRED COMPONENTS uc i18n) | ||
| get_filename_component(ICU_UC_LIBDIR "${ICU_UC_LIBRARIES}" DIRECTORY) | ||
| get_filename_component(ICU_I18N_LIBDIR "${ICU_I18N_LIBRARIES}" DIRECTORY) |
There was a problem hiding this comment.
This is code motion from further below.
Not your fault, but this really is not what it should be doing. The additional find_package(ICU) doesn't account for the custom build system.
In order to deal with the custom build system in Swift, the project added the custom variables to specify the library - SWIFT_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH}_ICU_{UC,I18N}. I think that we should continue to use this existing path rather than add all this additional complexity. SWIFT_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH}_ICU_{I18N,UC} should give you the path to the library. Setting that appropriately should wire the path all the way through. We should use that rather than the different paths for -licu... and the find_package.
Would be wonderful if you felt like doing that cleanup!
There was a problem hiding this comment.
Done. Added two commits in this PR to do this, since they are slightly non-trivial.
| if(SWIFT_BUILD_STATIC_STDLIB AND "${sdk}" STREQUAL "LINUX") | ||
| set(libdl -ldl) |
There was a problem hiding this comment.
Ugh; I really wish that we could use CMAKE_DL_LIBS, unfortunately, the custom cross-compilation build system in Swift doesn't support that. We already have the special handling for dl in _add_target_variant_link_flags, can we re-use that rather than duplicate knowledge of libdl here? Alternatively, perhaps we should consider creating a SWIFT_DL_LIBS_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH} variable to indicate if/when libdl is needed on a platform?
There was a problem hiding this comment.
This one's a little more complex, since it appears (to me) that _add_target_variant_link_flags works with a CMake target, and maybe there's a way to create a dummy target and pull the link libraries. The latter is probably simpler, but I'll probably need to spin up a Linux build to do this properly.
Added a TODO and I'll take a look at this in a later PR. Does that sound reasonable?
|
Never mind on the file comment - I missed the fact that you had removed the custom command. |
Static binaries can be built on other platforms, not just Linux; we use the same check for ELF output to condition static binaries like for static stdlib. However, since some ELF platforms don't need -ldl, pull this out into a conditionally set variable. As a side note: on OpenBSD, lld at HEAD does not work to link the test binary, but the platform lld works fine, so XFAIL this temporarily.
In the prior commit we opened static binaries for all ELF platforms when requested. For some reason, the ImageInspectionELF module is split out into its own library when building stdlib as a static library. However, there are still references to lookupSymbol from swiftCore to ImageInspectionELF, which cause lazy binding errors at runtime which foul up these unit tests. Currently it is unknown whether it is necessary to keep splitting out ImageInspectionELF like this; on its face, this seems rather unnecessary. Until then, mark these tests unsupported when stdlib exists as a static library, since these tests use the dynamic version of stdlib, and won't have the correct references to ImageInspectionELF.
As suggested in swiftlang#32736, it would be ideal to use the FindICU generated variables to refer to the ICU library paths. However, as it stands, this only finds the dynamic libraries, so here we find the static libraries as well.
3405691582
force-pushed
the
MoreStaticBinaries
branch
from
a51b67f to
4183d5f
Compare
|
The only reason it was ever gated on Linux was that I had no way to test on other ELF platforms and so couldnt correctly construct the Note This is why there are 2 |
Instead of deriving the static library paths twice, use the variables generated by the FindICU cmake module. The base cmakefiles only find icui18n and icuuc, but icudata is required to complete a static link, so we have to invoke find_package again here to ensure the variable is set.
3405691582
force-pushed
the
MoreStaticBinaries
branch
from
4183d5f to
e1c8011
Compare
|
Right. Maybe I haven't looked into it too deeply, but does it not make sense not have lookupSymbol fallback to the static mechanism if dlopen fails, rather than replacing lookupSymbol entirely? |
|
It been a while since I looked at it but I believe my thinking was that on Linux with Glibc and static binaries, using the |
|
@3405691582 btw there is an integration test @ swiftlang/swift-integration-tests#69 that hasnt been merged but you could modify for OpenBSD to act as a test for this PR |
|
@spevans unfortunately, Dispatch needs a lot of work to be done for OpenBSD, but something similar might work instead. Thanks for the pointer, I'll take a look. |
|
Still needed? Please rebase if parts of this would still be useful. |
Currently, building static binaries is gated to Linux. In order for this to work, the compiler looks for a file with a list of flags and uses them to generate a statically linked binary. This is a static file copied from the repository.
However, when building stdlib statically, the link files are generated dynamically from the cmakefiles; specifically, this is necessary to get the correct reference to the ICU libraries, as ICU is not a mandatory part of the build process. Therefore, we use the same dynamic generation of the linker flag file used in static stdlib generation for static binary generation.
Additionally, libdl is apparently necessary for Linux, but is not for OpenBSD, so this is conditioned here in a similar way that the ICU libraries are referenced.
Some unit tests are fouled up by this change because for some reason ImageInspectionELF.cpp is split out of swiftCore and put into its own library, and there still remain references from swiftCore to lookupSymbol in ImageInspectionELF. This causes lazy binding errors at runtime which mess up the expectations in these unit tests. This PR doesn't take a position yet as to whether such a split is necessary, so mark these unit tests UNSUPPORTED for now.
One unit test on OpenBSD is marked XFAIL for now because the test refers to lld at HEAD and not the platform lld; the lld at HEAD on OpenBSD does not link static binaries properly but the platform lld works fine if manually tested.