Skip to content

Keep start and stop symbols linked through clang (Linux) #8214

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

Conversation

etcwilde
Copy link

Changes in https://reviews.llvm.org/D96914 taught LLD to remove __start_##name and __stop_##name symbol names. The Swift runtime uses these specially named symbols to unpack the runtime type metadata on Linux and WebAssembly systems, resulting in broken binaries when linked with lld. This change tells the Swift-paired Clang to force the -z nostart-stop-gc flag when compiling for Linux-y systems, including Musl, WASM, and Android. Swift and the clang-linker have to work with other existing toolchains, so we can't simply flip the CMake option in the LLVM build system to keep these symbols and get a working binary.

Changes in https://reviews.llvm.org/D96914 taught LLD to remove
`__start_##name` and `__stop_##name` symbol names. The Swift runtime
uses these specially named symbols to unpack the runtime type metadata
on Linux and WebAssembly systems, resulting in broken binaries when
linked with `lld`. This change tells the Swift-paired Clang to force the
`-z nostart-stop-gc` flag when compiling for Linux-y systems, including
Musl, WASM, and Android. Swift and the clang-linker have to work with
other existing toolchains, so we can't simply flip the cmake option in
the LLVM build system to keep these symbols and get a working binary.
@etcwilde
Copy link
Author

@swift-ci please test

@etcwilde
Copy link
Author

I have checked this manually with
-target x86_64-linux-gnu test.cpp -fuse-ld=lld -###,
-target x86_64-linux-musl test.cpp -fuse-ld=lld -###,
-target aarch64-linux-android21 test.cpp -fuse-ld=lld -###, and
-target armv7a-linux-androideabi test.cpp -fuse-ld=lld -###, to ensure that clang includes the flags when linking with lld on Linux (GNU and Musl), as well as on Android. Looking for the right place to put a test for this.

@compnerd
Copy link
Member

I think that you can add a test in lld/tests/ELF.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This does deserve a test as we are diverging from upstream and we don't want to lose the difference accidentally.

@@ -836,7 +836,7 @@ std::string ToolChain::GetLinkerPath(bool *LinkerIsLLD) const {
if (llvm::sys::fs::can_execute(UseLinker))
return std::string(UseLinker);
} else {
llvm::SmallString<8> LinkerName;
llvm::SmallString<8> LinkerName = {};
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary? Would be nice to stay as close to upstream as possible.

Comment on lines +680 to +682
bool useLLD = false;
const char *Exec = Args.MakeArgString(ToolChain.GetLinkerPath(&useLLD));
if (useLLD) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not if (Args.hasArg(options::OPT_fuse_ld_EQ) && Args.getLastArgValue(options::OPT_fuse_ld_EQ).ends_with_insensitive("lld"))?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, mainly to support when someone isn't passing --fuse-ld=lld. It should still catch it if clang was configured to use lld by default. We're already calling GetLinkerPath, which is also conveniently supposed to detect when the linker is lld either through clang being configured to use lld by default, --fuse-ld=lld, or --ld-path=<filepath>/ld.lld.

@etcwilde
Copy link
Author

etcwilde commented Mar 5, 2024

I prefer swiftlang/swift#72061 to resolve this issue.

@etcwilde etcwilde closed this Mar 5, 2024
@etcwilde etcwilde deleted the ewilde/disable-symbol-munging branch March 5, 2024 05:29
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.

2 participants