-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Fix the symbol visibility in Swift compatibility lib into default instead of hidden, solve auto-linking issue and match Apple's behavior #70627
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
base: main
Are you sure you want to change the base?
Conversation
…visibility, not hidden This symbol is needed for auto-linking. When A.dylib depends on the B.dylib, B should export the symbol and A will linked at runtime This matches what Apple's xctoolchain shipped compatibility static library
default
instead of hidden, solve auto-linking issue and match Apple's behavior
@swift-ci Please test |
@swift-ci please smoke test |
Perhaps you could use Looking at the https://github.com/apple/swift/blob/f08f86c71617bacbc61f69ce842e284b27036598/stdlib/toolchain/Compatibility51/CMakeLists.txt, I'm curious as to why |
+1 for this. Did not get any result when searching "visibility=hidden" in the project. @dreampiggy |
Does non-Apple (means, Linux and Windows) use this backport compatibility override ? I guess this is no need for that The I also found that |
I'm curious about this as well. Even that some compile unites in It seems that this compile unit should not pass |
If I understand correctly, https://github.com/apple/swift/blob/1e172385e4f26e6bff8489d9f0b468f947e78146/stdlib/CMakeLists.txt#L89-L91 affects toolchain builds.
I'm not sure.
If you can't use |
Can change this attribute using the But anyway, is there someone who maintains the CMake config can explain why this compile units use visibility hidden as defaults ? |
Xcode Version 14.2 ,MacBook Pro (Retina, 15-inch, Mid 2015),how to fix it? |
Changes
TLDR: Change the
Overrides.cpp
2 symbols to explicit using thedefault
visibility, instead of implicithidden
visibility from the compiler options (-fvisibility=hidden
)This matches Apple's Xcode toolchain bundled
libswiftCompatibility50.a
binary.Background
Swift's compatibility static lib (
libswiftCompatibility50.a
) is used for backport Swift runtime features to Swift ABI-stable OS, like iOS 12.4. The new compiler language feature may need some runtime compatibility layer to run on the lower swift runtime host.However, we faced a issue when using the apple/swift with Xcode's bundled closed-source swift. The issues is because of the visibility for this symbol
__ZN5swift34swift50override_conformsToProtocolEPKNS_14TargetMetadataINS_9InProcessEEEPKNS_24TargetProtocolDescriptorIS1_EEPFPKNS_18TargetWitnessTableIS1_EES4_S8_E
The crash is like this:
Our team is trying to solve this issue and found out the reason in the upstream.
Issues
After some investigating of the dynamic library, we surprisingly found that this missing symbol is indeed missing. Following the crash message from dyld, this two dylibs contains a dependency relation (
AppStorageCore.framework
will loadEEAtomic
):In theory, the
AppStorageCore.framework
useundefined
symbol for that is OK. But theEEAtomic.framework
must export the symbol.Behavior differences
We start to search the toolchain codebase, and found that the symbol exists in toolchain's
libswiftCompatibility50.a
. The symbols introduced fromstdlib/toolchain/Compatibility50/Overrides.cpp
Xcode toolchain
We use
llvm-objdump -Ct
to compare between the binaries of these two toolchains.The results shows it's a
external
symbolSwift.org toolchain
However, when compared to the Xcode toolchain, the Swift.org open-sourced toolchain use
external hidden
symbolCompare
So why this use
hidden
visibility ?Compiler insert
-fvisibility=hidden
From the toolchain build log (using
./build-script -verbose-build
), we can see theOverrides.cpp
compile unit use the hidden visibility as defaultsSo, we must explicit declare the visibility in source code. Or it will use
hidden
but notdefault
visibility.This MR just used to change for this. After changes, our new built toolchain can successfully run the same use case without dyld runtime crash. And solve the problem.
I think this issue is suitable to upstream, so I fire this PR here. Hope someone can help to review and merge-in, or provide better solution.