Skip to content

[5.7] Remove --gc-sections for all targets for now #5708

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 5, 2022
Merged

[5.7] Remove --gc-sections for all targets for now #5708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions Sources/Build/BuildPlan.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,17 +1290,12 @@ public final class ProductBuildDescription {
return ["-Xlinker", "-dead_strip"]
} else if buildParameters.triple.isWindows() {
return ["-Xlinker", "/OPT:REF"]
} else if buildParameters.triple.arch == .wasm32 {
// FIXME: wasm-ld strips data segments referenced through __start/__stop symbols
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Should we really be resolving this by making this the unconditional else branch? Is there no solution here where we check a second condition to get this behavior? (Sorry about my ignorance in this area, I’m mostly just coming to this conclusion in a this context-free way)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we'd have to check that the linker was not lld, which might be fine to do just based on the use-ld arg value, but also that supports paths like -use-ld=/path/to/lld, which just made it feel potentially more error prone.

Copy link
Member

Choose a reason for hiding this comment

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

Other than this concern, I’d be fine taking it onto the release branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine? its basically adding additional [dead stripping] linker arguments for certain platforms, and none in the default case

Copy link
Member Author

Choose a reason for hiding this comment

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

It is too bad that folks not using lld can't get the dead stripping benefit, but theoretically at least they can opt in manually 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

@airspeedswift thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that checking for lld is too error-prone, as linker selection is not a direct SPM flag, so you're stuck parsing the various -Xswiftc and -Xlinker flags and still wouldn't catch situations where people don't pass in SPM flags but symbolically link their system linker to lld, because they use lld by default with all toolchains.

// FIXME: wasm-ld / ld.lld strips data segments referenced through __start/__stop symbols
// during GC, and it removes Swift metadata sections like swift5_protocols
// We should add support of SHF_GNU_RETAIN-like flag for __attribute__((retain))
// to LLVM and wasm-ld
// This workaround is required for not only WASI but also all WebAssembly archs
// using wasm-ld (e.g. wasm32-unknown-unknown). So this branch is conditioned by
// arch == .wasm32
return []
} else {
return ["-Xlinker", "--gc-sections"]
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/BuildTests/BuildPlanTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ final class BuildPlanTests: XCTestCase {
XCTAssertEqual(try result.buildProduct(for: "exe").linkArguments(), [
"/fake/path/to/swiftc", "-g", "-L", "/path/to/build/release",
"-o", "/path/to/build/release/exe", "-module-name", "exe", "-emit-executable",
"-Xlinker", "--gc-sections", "-Xlinker", "-rpath=$ORIGIN",
"-Xlinker", "-rpath=$ORIGIN",
"@/path/to/build/release/exe.product/Objects.LinkFileList",
"-target", defaultTargetTriple,
])
Expand Down