Add -dead_strip / --gc-sections for release builds

[keith]

In the past it was unsafe to pass -dead_strip because there was some
required swift metadata that was stripped. It seems that at this point all
of those cases are either fixed or marked as @llvm.used, so passing
these flags should safely reduce binary size in some cases. If there are
other cases where this isn't safe we should likely annotate them as
@llvm.used.

Related: https://bugs.swift.org/browse/SR-521 #215

[tomerd]

cc @kubamracek @jckarter @compnerd

[tomerd]

thanks @keith this makes sense based on what @kubamracek was saying on another thread. couple of questions / ideas:

1. should this be behind a flag so that its possible to disable/enable (ie enabled by default but possible to disable If causes issues as an escape hatch)
2. do we need to add a small unit test? especially if do add the flag (which I lean towards thinking we should)
3. do we know that this also works fine on Linux and Window which use different linkers afaik?

[compnerd]

CC: @kubamracek (I believe Kuba was involved in the changes for the metadata preservation).

[keith]

A flag sounds fine to me, would folks prefer opt-in or opt-out? I'm not sure how much testing an opt-in flag would get at this point, but I'll defer to you all for your risk preference.

I did test this on Linux and I think --gc-sections is safe for all ELF linkers

[tomerd]

@keith I would go with opt out given that @kubamracek believes this is safe enough. we need @compnerd init on Windows too. lmk if you need guidance on how to expose a flag and pipe it through the different layers

[kubamracek]

I'm definitely supportive of this, I'm not aware of any problems/bugs in this area (but TBH I have mostly only worked with Darwin / Mach-O / ld64, and almost not at all with other platforms / file formats / linkers). I am even supportive in making dead stripping be the default behavior, but I want to point out that there are cases where dead-stripping will affect a program -- and mostly it's "shady" things like dlsym'ing for an otherwise unused symbol, and probably only really affects C code and not Swift code. Before we make dead-stripping the default, we should acknowledge that stuff like this might be affected. I personally think it's the right choice (and programs relying on not-dead-stripping would either need to opt out, or fix their code, there's some simple options like __attribute__((used))), but I wonder what the thinking of others is :)

[tomerd]

its also worth mentioning that at the current state of things, -dead_strip would have little effect on Swift code, and would mostly effect C code built with SwiftPM

[keith]

Updated with a flag!

[compnerd]

Please do a test build on Windows just to make sure that things don't break.

[tomerd]

Please do a test build on Windows just to make sure that things don't break.

@compnerd @shahmishal is there a way to trigger CI for Windows for this change? maybe a dummy PR with cross-repo test?

[tomerd]

@swift-ci please smoke test

[shahmishal]

Please do a test build on Windows just to make sure that things don't break.

@compnerd @shahmishal is there a way to trigger CI for Windows?

Hopefully soon we will be able to from SwiftPM, however for now you will have to do cross PR to trigger Windows testing.

[tomerd]

Hopefully soon we will be able to from SwiftPM, however for now you will have to do cross PR to trigger Windows testing.

thanks @shahmishal created swiftlang/swift#41374

what is the trigger phrase for Windows verification?

[shahmishal]

@swift-ci test Windows

[compnerd]

@shahmishal does that now trigger a different job? There are two separate builds - the @swift-ci please test Windows builds and tests the toolchain (which is why it is quick); the full toolchain build is almost twice as long but builds a full toolchain image (and tests a bit more, though still in sufficient for this as of yet :-()

[tomerd]

this is pending verification on Windows, see discussion on swiftlang/swift#41374

[tomerd]

@keith in the meantime maybe add an entry in the change log to document the change?

[tomerd]

@swift-ci please smoke test

[tomerd]

pending verification on Windows, see discussion on swiftlang/swift#41374

[tomerd]

@swift-ci please smoke test

[compnerd]

:-( it seems that something isn't really correct:

S:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swiftc.exe -Xlinker -debug -L S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release -o S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release\Calculator.exe -module-name Calculator -lSwiftCOM -lcassowary -emit-executable @S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release\Calculator.product\Objects.LinkFileList -target x86_64-unknown-windows-msvc -lUser32 -lComCtl32 -sdk S:\Library\Developer\Platforms\Windows.platform\Developer\SDKs\Windows.sdk -libc MD -I S:\Library\Developer\Platforms\Windows.platform\Developer\Library\XCTest-development\usr\lib\swift\windows\x86_64 -L S:\Library\Developer\Platforms\Windows.platform\Developer\Library\XCTest-development\usr\lib\swift\windows

Note that the -Xlinker /OPT:REF is missing. That was built with -c release.

[keith]

I inverted the boolean when we renamed from disable -> enable, fixed, sorry about that! It didn't break the tests because it was inverted there too after that change. Is there a way to test the actual flag behavior (like lit in LLVM)?

[tomerd]

@swift-ci please smoke test

[compnerd]

S:\Library\Developer\Toolchains\unknown-Asserts-development.xctoolchain\usr\bin\swiftc.exe -Xlinker -debug -L S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release -o S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release\Calculator.exe -module-name Calculator -emit-executable -Xlinker /OPT:REF @S:\SourceCache\swift-win32\.build\x86_64-unknown-windows-msvc\release\Calculator.product\Objects.LinkFileList -target x86_64-unknown-windows-msvc -lUser32 -lComCtl32 -lOle32 -lPortableDeviceGuids -sdk S:\Library\Developer\Platforms\Windows.platform\Developer\SDKs\Windows.sdk -libc MD -I S:\Library\Developer\Platforms\Windows.platform\Developer\Library\XCTest-development\usr\lib\swift\windows\x86_64 -L S:\Library\Developer\Platforms\Windows.platform\Developer\Library\XCTest-development\usr\lib\swift\windows

Built and ran the calculator demo, seems to be fine, and the linker was also fine with the flags. I think that this should be fine from the Windows side at least. And the command line indicates that the flag is now there.

[keith]

thanks for testing!

[tomerd]

thanks @keith

[keith]

Thanks all!

[drodriguez]

@keith: do you remember which linkers and versions did you use to test in Linux ELF? With a recent LLD you have swiftlang/llvm-project@6d2d3bd which enables GC of the start/stop symbols, which seems to be affecting the Swift metadata section symbols in our setup.

[keith]

Interesting, I just tried the defaults, do you have a repro case I can debug with?

[drodriguez]

Not with the upstream build system, sadly. I was just trying to compile a newer SwiftPM with a recently compiled SwiftPM that includes this (and pointing to a quite recent LLD) and started seeing these linking problems about the start and stop symbols being hidden undefined. I think it will need a fix in how the sections are declared in https://github.com/apple/swift/blob/main/stdlib/public/runtime/SwiftRT-ELF.cpp but a first idea adding "R" to the section declaration did fix many test in the check-swift test suite, but not all of them (IIRC there were 76 left, many of them crashes).

I will try to keep looking. For the time being I can work around it with --disable-dead-strip so it is not the end of the world.

[keith]

I think the assumption from folks here was generally that things should be correctly referenced / non-strippable today, so if that's not the case we may have to temporarily reconsider this default unless we can come up with a fix for those

[finagolfin]

I just hit this when building the latest Mar. 9 trunk source snapshot of the toolchain natively on my Android AArch64 phone that uses lld, which is fixed by disabling this flag by default in the SPM source. This may be a recent lld regression, which this new flag triggers for us on Android, as this guy reported the issue on linux with lld 13, unrelated to this SPM flag, on the forum last month, or maybe lld or clang 13 enabled this flag by default all of a sudden on linux too.

Strangely, my Android CI, which cross-compiles an Android SDK and several packages against it, worked fine on the first run with this new Mar. 9 SPM, I'll look into that further.

There may be issues with this flag and lld on ELF platforms interacting badly with swiftrt.o, we'll need to investigate.