Fix flags when using clang as linker for Fuchsia by tmandry · Pull Request #99500 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation22 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Don't add C runtime or set dynamic linker when linking with clang for
Fuchsia. Clang already does this for us.
Relevant to the compiler team, which will review and decide on the PR/issue.
label
r? @fee1-dead
(rust-highfive has picked a reviewer for you, use r? to override)
@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.
Don't add
Scrt1.ofor 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.
This comment was marked as outdated.
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
There's no logic in LLD for locating
Scrt1.oso 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?
Status: This is awaiting some action (such as code changes or more information) from the author.
label
Based on the above discussion I don't think we should merge this as-is (since it would break linking with lld).
we should include
Scrt1.oonly 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.
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
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
@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.
It seems like your original idea to add a
Fuchsiavariant toCrtObjectsFallbackmakes 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
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.
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
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
Fix linker flags for Fuchsia Fix flags when using clang as linker for Fuchsia
📌 Commit 5472b38a4b5f4d26ca49faf33453ad9f2e341fc1 has been approved by petrochenkov
It is now in the queue for this repository.
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
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.
📌 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
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request
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
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.
Labels
Operating system: Fuchsia
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.