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.

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

Fix flags when using clang as linker for Fuchsia #99500

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Jul 20, 2022

Don't add C runtime or set dynamic linker when linking with clang for
Fuchsia. Clang already does this for us.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 20, 2022
@rust-highfive
Copy link
Collaborator

r? @fee1-dead

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2022
@tmandry tmandry marked this pull request as draft July 20, 2022 11:10
@fee1-dead
Copy link
Member

@tmandry you should r question rust-lang/compiler if you want it to be reviewed or r question @ghost if not. I don't think I am capable of reviewing this.

@tmandry tmandry marked this pull request as ready for review July 20, 2022 16:33
@tmandry
Copy link
Member Author

tmandry commented Jul 20, 2022

r? @rust-lang/compiler

@rust-highfive rust-highfive assigned oli-obk and unassigned fee1-dead Jul 20, 2022
@petrhosek
Copy link
Contributor

Don't add Scrt1.o for Fuchsia, which is already handled by lld/clang and they seem to be better at knowing where to look for it.

There's no logic in LLD for locating Scrt1.o so I believe this is going to break linking when using LLD as a linker.

@Dylan-DPC

This comment was marked as outdated.

@bors bors 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 Jul 21, 2022
@Dylan-DPC
Copy link
Member

oops wrong PR :D

@nikic nikic mentioned this pull request Jul 22, 2022
@tmandry
Copy link
Member Author

tmandry commented Jul 22, 2022

There's no logic in LLD for locating Scrt1.o so I believe this is going to break linking when using LLD as a linker.

Then it seems we should include Scrt1.o only when using lld/ld as a linker and skip it for clang/gcc.

This doesn't seem specific to Fuchsia, but I can't find any general handling of it. Is this because other platforms just aren't using lld yet? @petrochenkov do you know?

@petrochenkov
Copy link
Contributor

r? @petrochenkov
@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 22, 2022
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2022
@tmandry
Copy link
Member Author

tmandry commented Jul 22, 2022

Based on the above discussion I don't think we should merge this as-is (since it would break linking with lld).

@petrochenkov
Copy link
Contributor

we should include Scrt1.o only when using lld/ld as a linker and skip it for clang/gcc

The code that decides whether to link startup objects or not is fn crt_objects_fallback.
This case can certainly be solved by adding a new Fuchsia variant to enum CrtObjectsFallback and implementing some custom logic for it, but that may be an overkill, I'll check in more detail in the next few days.

@petrochenkov
Copy link
Contributor

Ok, we are missing some target spec infra here, so the best thing you can do right now is

--- a/compiler/rustc_codegen_ssa/src/back/link.rs
+++ b/compiler/rustc_codegen_ssa/src/back/link.rs
@@ -2086,7 +2086,9 @@ fn add_order_independent_options(
     // Make the binary compatible with data execution prevention schemes.
     cmd.add_no_exec();

-    if crt_objects_fallback {
+    // FIXME: we are currently missing some infra here (per-linker-flavor CRT objects),
+    // so Fuchsia has to be special-cased.
+    if crt_objects_fallback && !(sess.target.os == "fuchsia" && flavor == LinkerFlavor::Gcc) {
         cmd.no_crt_objects();
     }

And the same exception for fn add_pre_link_objects.
@rustbot author

@rustbot rustbot 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 Aug 1, 2022
@tmandry
Copy link
Member Author

tmandry commented Aug 3, 2022

@petrochenkov We aren't currently using crt_objects_fallback, do you mean we should switch to using that?

It seems like your original idea to add a Fuchsia variant to CrtObjectsFallback makes sense; we could then check the linker flavor in the crt_objects_fallback function.

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 3, 2022

It seems like your original idea to add a Fuchsia variant to CrtObjectsFallback makes sense

No, no, I reread the code and realized that the "CRT object fallback" is a very outdated naming at this point, the flag really means "whether self-contained linking mode is enabled" (as in -Clink-self-contained). Fuchsia doesn't use this mode, the startup objects are found on the system and not shipped with rustc. So it wasn't a good suggestion.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 10, 2022
rustc_target: Update some old naming around self contained linking

The "fallback" naming pre-dates introduction of `-Clink-self-contained`.
Noticed when reviewing rust-lang#99500.

This PR doesn't break any json target spec, but supporting per-linker-flavor startup objects needed by rust-lang#99500 will break them, so maybe next time I'll remove the compatibility names.
@tmandry tmandry force-pushed the fuchsia-flags branch 2 times, most recently from 122b5ce to 5564eb1 Compare August 10, 2022 22:44
@tmandry
Copy link
Member Author

tmandry commented Aug 10, 2022

Okay, I've updated this to just remove flags when linking with clang. We don't use the crt_fallback for Fuchsia at all so we don't need to modify that path.

@rustbot ready

@rustbot rustbot 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 Aug 10, 2022
@tmandry tmandry changed the title Fix linker flags for Fuchsia Fix flags when using clang as linker for Fuchsia Aug 10, 2022
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 10, 2022

📌 Commit 5472b38a4b5f4d26ca49faf33453ad9f2e341fc1 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Aug 10, 2022
@rust-log-analyzer

This comment has been minimized.

Don't add C runtime or set dynamic linker when linking with clang for
Fuchsia. Clang already does this for us.
@tmandry
Copy link
Member Author

tmandry commented Aug 10, 2022

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 10, 2022

📌 Commit 55d5dcb has been approved by petrochenkov

It is now in the queue for this repository.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2022
…iaskrgr

Rollup of 13 pull requests

Successful merges:

 - rust-lang#93896 (rustdoc: make item-infos dimmer on dark theme)
 - rust-lang#99337 (rustdoc: simplify highlight.rs)
 - rust-lang#99421 (add crt-static for android)
 - rust-lang#99500 (Fix flags when using clang as linker for Fuchsia)
 - rust-lang#99511 (make raw_eq precondition more restrictive)
 - rust-lang#99992 (Add `x.sh` and `x.ps1` shell scripts)
 - rust-lang#100112 (Fix test: chunks_mut_are_send_and_sync)
 - rust-lang#100203 (provide correct size hint for unsupported platform `CommandArgs`)
 - rust-lang#100307 (Fix rust-lang#96847)
 - rust-lang#100350 (Stringify non-shorthand visibility correctly)
 - rust-lang#100374 (Improve crate selection on rustdoc search results page)
 - rust-lang#100392 (Simplify visitors)
 - rust-lang#100418 (Add stability attributes to BacktraceStatus variants)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92b32e3 into rust-lang:master Aug 12, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 12, 2022
@tmandry tmandry deleted the fuchsia-flags branch August 12, 2022 20:02
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 13, 2022
rustc_target: Update some old naming around self contained linking

The "fallback" naming pre-dates introduction of `-Clink-self-contained`.
Noticed when reviewing rust-lang#99500.

This PR doesn't break any json target spec, but supporting per-linker-flavor startup objects needed by rust-lang#99500 will break them, so maybe next time I'll remove the compatibility names.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 14, 2022
rustc_target: Update some old naming around self contained linking

The "fallback" naming pre-dates introduction of `-Clink-self-contained`.
Noticed when reviewing rust-lang#99500.

This PR doesn't break any json target spec, but supporting per-linker-flavor startup objects needed by rust-lang#99500 will break them, so maybe next time I'll remove the compatibility names.
@workingjubilee workingjubilee added the O-fuchsia Operating system: Fuchsia label May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-fuchsia Operating system: Fuchsia S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.