Skip to content

Remove --gc-sections for all targets for now #5707

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

Merged

Conversation

keith
Copy link
Member

@keith keith commented Aug 2, 2022

Similar to the comment here about wasm-ld, lld recently switched a
default that broke this for all elf targets. This is a low risk fix
since we weren't passing this flag before either. People who really need
this behavior can still pass something like -Xlinker --gc-sections -Xlinker -z -Xlinker nostart-stop-gc

Fixes #5698

Similar to the comment here about wasm-ld, lld recently switched a
default that broke this for all elf targets. This is a low risk fix
since we weren't passing this flag before either. People who really need
this behavior can still pass something like `-Xlinker --gc-sections
-Xlinker -z -Xlinker nostart-stop-gc`
@keith
Copy link
Member Author

keith commented Aug 2, 2022

Ideally we can fix this instead with swiftlang/swift#60357, but if that PR isn't enough, this might be necessary in the short term.

@finagolfin
Copy link
Member

Can we just apply this as a short-term fix for the 5.7 branch, or do all 5.7 fixes have to go into trunk first? Your stdlib pull will fix trunk and there's no urgency with trunk, only 5.7.

@keith
Copy link
Member Author

keith commented Aug 3, 2022

here's a 5.7 PR #5708

@tomerd
Copy link
Contributor

tomerd commented Aug 4, 2022

@keith should we get this merged into main first, then 5.7?

@tomerd
Copy link
Contributor

tomerd commented Aug 4, 2022

@swift-ci smoke test

@keith keith marked this pull request as ready for review August 4, 2022 21:39
@keith
Copy link
Member Author

keith commented Aug 4, 2022

yea there was a question of if we could only do 5.7 and try to solve the core issue without reverting on main, but whatever the process is is fine with me. we can always revert this if we can solve the core issue too

@tomerd
Copy link
Contributor

tomerd commented Aug 4, 2022

I rather keep them symmetrical so we dont have a later regression, then we can revert / reintroduce when ready?

@keith
Copy link
Member Author

keith commented Aug 4, 2022

yep sounds fine!

@keith
Copy link
Member Author

keith commented Aug 5, 2022

@swift-ci please test windows platform

@keith
Copy link
Member Author

keith commented Aug 5, 2022

I doubt the windows failure here is related, it seems to be timing out

@tomerd
Copy link
Contributor

tomerd commented Aug 5, 2022

@compnerd okay to merge even with the windows failure?

@compnerd
Copy link
Member

compnerd commented Aug 5, 2022

I think that a retest should help - the offending change was reverted.

@compnerd
Copy link
Member

compnerd commented Aug 5, 2022

@swift-ci please test Windows platform

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 seems safe for Windows; if the Windows CI still flakes, lets go ahead and merge this anyways.

@tomerd tomerd merged commit e9cd5dc into swiftlang:main Aug 5, 2022
finagolfin added a commit to finagolfin/swift-package-manager that referenced this pull request Aug 19, 2022
It should work with lld now that the lld flag '-z nostart-stop-gc' was added with
swiftlang/swift#60544 and swiftlang/swift-driver#1153.
neonichu pushed a commit that referenced this pull request Aug 19, 2022
It should work with lld now that the lld flag '-z nostart-stop-gc' was added with
swiftlang/swift#60544 and swiftlang/swift-driver#1153.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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