Linking with lld stopped working on linux/Android since lld 13 and this repo enabled `--gc-sections`

[finagolfin]

Description

After dead-stripping was enabled in #4135, various people reported problems with using lld, though it wouldn't always show up. It appears to be some change with lld 13, as lld 12 works fine, perhaps the commit @drodriguez linked (I'll try reverting that commit and report back later).

Obviously, this is really a regression in how Swift interacts with lld 13 and 14, but I'm reporting it here first as we'll probably want to amend SPM 5.7 quickly before the underlying lld issue is figured out. @keith, maybe we should disable stripping on ELF if the linker is lld for now?

@3405691582, can you reproduce on OpenBSD?

Expected behavior

Linking to work fine with lld, as it does with gold.

Actual behavior

Several linker errors like this show up instead:

ld.lld: error: undefined hidden symbol: __start_swift5_protocols
>>> referenced by SwiftRT-ELF.cpp
>>>               /home/butta/swift-DEVELOPMENT-SNAPSHOT-2022-07-25-a-ubuntu20.04/usr/lib/swift/linux/x86_64/swiftrt.o (swift_image_constructor())

Steps to reproduce

1. Clone swift-argument-parser
2. Remove a file because of some unrelated bug: rm Sources/ArgumentParserTestHelpers/TestHelpers.swift
3. Build the package in release mode with lld: ../swift-DEVELOPMENT-SNAPSHOT-2022-07-25-a-ubuntu20.04/usr/bin/swift build -v -c release -Xswiftc -use-ld=lld

Swift Package Manager version/commit hash

latest Jul. 25 trunk snapshot and 5.7 Jul. 23 snapshot

Swift & OS version (output of swift --version && uname -a)

Swift 5.8 and 5.7 on Ubuntu 20.04 x86_64

[finagolfin]

I can confirm that reverting llvm/llvm-project@6d2d3bd fixes this issue. We're going to have to adapt that lld ELF commit for Swift.

[keith]

Here's a fix for this, it's not fully complete yet because the problem is the R directive I'm adding here wasn't supported until clang 13+, and binutils 2.36, so I think I need to conditionalize this somehow based on those versions. swiftlang/swift#60357

Can you test with your real world example to make sure this works there too?

[keith]

I think disabling --gc-sections if we need to as a safer fix is fine. We can leave -dead_strip on darwin in that case still

[keith]

For future reference for others the default change that introduced the issue here was highly controversial and I believe also broke rust, here is some further reading:

• https://reviews.llvm.org/D96914
• https://reviews.llvm.org/D114186

[finagolfin]

Here's a fix for this, it's not fully complete yet because the problem is the R directive I'm adding here wasn't supported until clang 13+, and binutils 2.36, so I think I need to conditionalize this somehow based on those versions

Yikes, but since Swift 5.7 and trunk both use clang/lld 13+, maybe that's enough?

Can you test with your real world example to make sure this works there too?

Just tried it natively on Android, can confirm your pull fixes the problem with lld 14.

I think disabling --gc-sections if we need to as a safer fix is fine.

It all depends if we can get your fix in quickly before the 5.7 release. If we can, no problem. If not, probably best to disable --gc-sections for 5.7 till we can get your fix into 5.7, then maybe re-enable that flag in SPM later with a 5.7 patch release.

@compnerd and @tomerd, what do you think?

[keith]

Yikes, but since Swift 5.7 and trunk both use clang/lld 13+, maybe that's enough?

The problem is that if the host compiler isn't up to date you can't build swift itself. I asked and it doesn't seem like there's an official matrix of supported versions here. This issue is really the worse case scenario for this IMO because you could theoretically use a GCC to build swift and then lld to link your swift program with the compiler, in which case this fix still wouldn't work.

[drodriguez]

Around February I tried the "R" in our internal builds, and while it fixed many things, it didn't fix all (I commented about it in #4135 (comment)). I haven't tried since then, and maybe something in the environment made my testing not be successful, but I wanted to point it out again. I really hope for something like that to work, but it might not be enough.

[keith]

Do you happen to have any of the example failures around still?

[finagolfin]

The problem is that if the host compiler isn't up to date you can't build swift itself.

Your fix is in the stdlib, which already has to be built by the Swift-forked clang, so the host compiler should be irrelevant.

This issue is really the worse case scenario for this IMO because you could theoretically use a GCC to build swift and then lld to link your swift program with the compiler, in which case this fix still wouldn't work.

I don't think we can worry about all that, particularly since Swift concurrency already requires compilation by the Swift-forked clang.

I haven't tried since then, and maybe something in the environment made my testing not be successful, but I wanted to point it out again. I really hope for something like that to work, but it might not be enough.

@drodriguez, I haven't tried running the compiler validation suite on this fix, only tried my repro case above after recompiling the stdlib. It would be good to know what else failed for you and why, hopefully not a result of this fix.

[keith]

Your fix is in the stdlib, which already has to be built by the Swift-forked clang, so the host compiler should be irrelevant.

Is this always the case? That you use the host compiler to build the forked clang and then that to build the stdlib? If so that would definitely solve that issue!

[finagolfin]

Is this always the case? That you use the host compiler to build the forked clang and then that to build the stdlib? If so that would definitely solve that issue!

Yes, that is the default for how the Swift toolchain is currently built, because of changes like swiftlang/swift#39245. That change eventually made it into clang 14, llvm/llvm-project@f670c5a, so building the stdlib may work with a stock clang 14 now, or there may be other changes specific to the Swift fork too.