Skip to content
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

Jump to bottom

Only pass --[no-]gc-sections if linker is GNU ld. #85274

Merged
merged 5 commits into from
May 19, 2021
Merged

Only pass --[no-]gc-sections if linker is GNU ld. #85274

merged 5 commits into from
May 19, 2021

Conversation

luqmana
Copy link
Member

@luqmana luqmana commented May 14, 2021

Fixes a regression from #84468 where linking now fails with solaris linkers. LinkerFlavor::Gcc does not always mean GNU ld specifically. And in the case of at least the solaris ld in illumos, that flag is unrecognized and will cause the linking step to fail.

Even though removing the is_like_solaris branch from gc_sections in #84468 made sense as -z ignore/record are more analogous to the --[no-]-as-needed flags, it inadvertently caused solaris linkers to be passed the --gc-sections flag. So let's just change it to be more explicit about when we pass those flags.

@luqmana
Only pass --[no-]gc-sections if linker is GNU ld.
3fe1d7f 
LinkerFlavor::Gcc does not always mean GNU ld specifically. And in the
case of at least the solaris ld in illumos, that flag is unrecognized
and will cause the linking step to fail.
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2021
@luqmana
Copy link
Member Author

luqmana commented May 14, 2021

r? @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2021

📌 Commit 3fe1d7f has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2021
@bors
Copy link
Contributor

bors commented May 16, 2021

⌛ Testing commit 3fe1d7f with merge 5d28235010a9b782714887f7a7e91bf7dd8da0b4...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 16, 2021
@luqmana
Copy link
Member Author

luqmana commented May 16, 2021

Test failed because the mingw targets do use gcc as the linker but the target spec never indicated linker_is_gnu so just updated that target spec.

@petrochenkov
Copy link
Contributor

@luqmana
The windows_gnu_base.rs spec was right, the windows-gnu gcc/ld targets COFF so it's missing a bunch of options from the regular ELF-targeting gcc/ld's, that's why it wasn't marked as linker_is_gnu.

If it supports --gc-sections, then the condition should be self.sess.target.linker_is_gnu || self.sess.target.is_like_windows

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2021
@luqmana
Copy link
Member Author

luqmana commented May 16, 2021

Hmm, then should the docs be updated since they say linker_is_gnu is:

Whether the linker support GNU-like arguments such as -O. Defaults to false.

The man pages for i686-w64-mingw32-ld even describe it as "The GNU linker". I also checked all the branches we use linker_is_gnu and all of them seem to work fine for mingw targets (tested by trying to pass them to i686-w64-mingw32-gcc -Wl,. So I guess the question is what option(s) in particular was preventing us from marking the mingw targets as such?

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2021
@petrochenkov
Copy link
Contributor

So I guess the question is what option(s) in particular was preventing us from marking the mingw targets as such?

Here's what my investigation showed:

  • -pie errors with LLD, acts as compatibility noop with BFD
  • --as-needed errors with LLD, acts as compatibility noop with BFD
  • -znoexecstack errors with both LLD and BFD, but never hit in rustc due to the previous is_like_windows condition
  • --enable-new-dtags errors with both LLD and BFD, but never hit in rustc because has_rpath is false for windows-gnu targets

I'm ok with setting linker_is_gnu to true for windows-gnu targets, because the meaning of linker_is_gnu is vague enough in general, it's just some code need to be adjusted in the linker_is_gnu -> linker_is_gnu && !is_like_windows way for LLD to work.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2021
@luqmana
Copy link
Member Author

luqmana commented May 17, 2021

@petrochenkov Thanks for digging into that! Updated the PR to handle those now

@petrochenkov
Copy link
Contributor

r=me with #85274 (comment) addressed.

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2021

📌 Commit ac5fd90 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 18, 2021
@Dylan-DPC
Rollup merge of rust-lang#85274 - luqmana:linker-is-gnu-gc-sections, …
9e20d98 
…r=petrochenkov

Only pass --[no-]gc-sections if linker is GNU ld.

Fixes a regression from rust-lang#84468 where linking now fails with solaris linkers. LinkerFlavor::Gcc does not always mean GNU ld specifically. And in the case of at least the solaris ld in illumos, that flag is unrecognized and will cause the linking step to fail.

Even though removing the `is_like_solaris` branch from `gc_sections` in rust-lang#84468 made sense as `-z ignore/record` are more analogous to the `--[no-]-as-needed` flags, it inadvertently caused solaris linkers to be passed the `--gc-sections` flag. So let's just change it to be more explicit about when we pass those flags.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request May 18, 2021
Closed
@jackh726 jackh726 mentioned this pull request May 19, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 19, 2021
@bors
Auto merge of rust-lang#85458 - jackh726:rollup-zvvybmt, r=jackh726
9f8012e 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#83366 (Stabilize extended_key_value_attributes)
 - rust-lang#83767 (Fix v0 symbol mangling bug)
 - rust-lang#84883 (compiletest: "fix" FileCheck with --allow-unused-prefixes)
 - rust-lang#85274 (Only pass --[no-]gc-sections if linker is GNU ld.)
 - rust-lang#85297 (bootstrap: build cargo only if requested in tools)
 - rust-lang#85396 (rustdoc: restore header sizes)
 - rust-lang#85425 (Fix must_use on `Option::is_none`)
 - rust-lang#85438 (Fix escape handling)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ec0e0d1 into rust-lang:master May 19, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 19, 2021
@luqmana luqmana deleted the linker-is-gnu-gc-sections branch May 19, 2021 17:06
@bjorn3 bjorn3 mentioned this pull request May 20, 2021
Closed
phil-opp added a commit to rust-osdev/bootloader that referenced this pull request May 20, 2021
@phil-opp
Fix nightly regerssion by manually passing --gc-sections
eead1a5 
Rust used to pass `--gc-section` automatically. In rust-lang/rust#85274, this behavior was changed to only pass that flag for targets that use a GNU linker. So we have to add it manually, otherwise the bootloader grows too large to be loaded by our assembly stage.
@phil-opp phil-opp mentioned this pull request May 20, 2021
Merged
@jonas-schievink jonas-schievink mentioned this pull request May 20, 2021
Closed
@luqmana luqmana mentioned this pull request May 21, 2021
Merged
@phil-opp phil-opp mentioned this pull request May 21, 2021
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2021
@bors
Auto merge of rust-lang#85531 - luqmana:flip-gc, r=petrochenkov
21e1cd9 
Swap TargetOptions::linker_is_gnu default from false to true and update targets as appropriate.

rust-lang#85274 gated the `--gc-sections` flag on targets that specified `linker_is_gnu` to stop us from passing it to incompatible linkers. But that had the unintended effect of the flag no longer being passed on targets for which it is valid and hence caused a regression in binary size. Given that most `ld`-style linkers are GNU compatible, this change flips our default for `linker_is_gnu` from false to true. That also means updating the targets that relied on the previous default:
* Apple
* Illumos
* L4Re (not sure about this one)
* MSVC
* NvtPtx
* Solaris

Fixes rust-lang#85519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
8 participants
@luqmana @rust-highfive @petrochenkov @bors @rust-log-analyzer @bjorn3 @jackh726 @rustbot