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

[RFC] recommend rust-lld as the default linker #39

Closed
japaric opened this issue Aug 7, 2018 · 11 comments · Fixed by #41
Closed

[RFC] recommend rust-lld as the default linker #39

japaric opened this issue Aug 7, 2018 · 11 comments · Fixed by #41
Milestone

Comments

@japaric
Copy link
Member

japaric commented Aug 7, 2018

once the unstable flag -Z linker-flavor is not required

Advantages:

  • No need to install arm-none-eabi-gcc to build binaries.

Disadvantages:

  • Linking to system C libraries (e.g. newlib) requires passing the library search path, which varies per build system, to the linker. If you want to do this it's actually easier to use gcc.

cc @rust-embedded/cortex-m

@japaric japaric added this to the RC1 milestone Aug 7, 2018
@adamgreig
Copy link
Member

Seems fine to recommend lld as the default choice, especially with rust-embedded/cortex-m-rt#84 landed, and just mention in the docs (and the .cargo/config comments) how to use gcc instead and why you might want to.

@korken89
Copy link
Contributor

korken89 commented Aug 8, 2018

+1

@therealprof
Copy link
Contributor

Am I missing something? I just removed the linker from my cargo config and it compiled just fine. What is preventing us from making the change now?

@japaric
Copy link
Member Author

japaric commented Aug 8, 2018

@therealprof the -Z linker-flavor flag which is required to use rust-lld is unstable and it's not clear if it will be stabilized for the edition release or if we'll land linker flavor inference (rust-lang/rust#52101) before the edition release.

@therealprof
Copy link
Contributor

@japaric I misunderstood your proposal then: I thought rust-lld is the default now and we're recommending using gcc/binutils for linking. What is preventing us from using rust-lld as default?

@japaric
Copy link
Member Author

japaric commented Aug 8, 2018

@therealprof sorry, I think the RFC title could be more precise: "use rust-lld as the default linker".

For the thumb* targets rustc, by default, uses arm-none-eabi-gcc to link programs. quickstart v0.3.3 doesn't change the linker: rustc uses gcc as it normally would.

The proposal is to have a future quickstart release change the linker to rust-lld using .cargo/config. That means that if you clone quickstart and run cargo build --target $T you'd be using rust-lld.

What is preventing us from using rust-lld as default?

Changing the linker requires passing -Z linker-flavor to rustc. This flag is unstable and doesn't work on stable or beta. We have a policy that crates should work on stable (or beta) by default and that features that require nightly should be opt-in. Following that policy switching to rust-lld should be opt-in rather than the default.

@therealprof
Copy link
Contributor

@japaric

What is preventing us from using rust-lld as default?

Changing the linker requires passing -Z linker-flavor to rustc. This flag is unstable and doesn't work on stable or beta. We have a policy that crates should work on stable (or beta) by default and that features that require nightly should be opt-in. Following that policy switching to rust-lld should be opt-in rather than the default.

Sorry for being so imprecise. I meant: What is preventing rustc from using lld by default? There should be a preference to using llvm parts instead of unrelated external depedencies, no?

@japaric
Copy link
Member Author

japaric commented Aug 8, 2018

What is preventing rustc from using lld by default?

Changing the default linker of any of the thumb* targets would be a breaking change (*). Imagine a user that only passes -Wl,-Tlink.x to the linker (via rustcflags); if we change the default linker from gcc to lld their build would break because lld expects -Tlink.x (w/o the -Wl, prefix).

(*) However, everyone who can build a binary is using nightly so it might be OK to do this breaking change without breaking Rust stability promise ("no breaking changes when doing updates on the stable channel). But, I imagine such change would require a rust-lang/rust RFC and getting an OK from all stakeholders. Also, there would need to be a stable mechanism to switch from rust-lld to arm-none-eabi-gcc so presumably either -Z linker-flavor would have to be stabilized or linker flavor inference would have to be implemented before changing the default linker.

@therealprof
Copy link
Contributor

If we don't change it for Rust 2018 edition we'll be stuck with it forever (as approximation for "a long time"). I'd rather break it now instead of having to work around it for the foreseeable future. Unless of course a smarter mechanism is around the corner (*).

Fun fact: I've been using the binutils linker directly all the time (instead of going through gcc) and so the arguments to pass are exactly the same.

(*) Like introspecting the linker args and inferring which linker to use.

@japaric
Copy link
Member Author

japaric commented Aug 9, 2018

@alexcrichton what would it take (RFC or PR+FCP) to change the default linker of a built-in target? This is technically a breaking change but in this case the change would only break nightly users. More details in #39 (comment).

@alexcrichton
Copy link

An interesting question! It does sound unfortunatley like it'd be a breaking change, so in that sense to do this I think we'd just want to have a concrete handle on what exactly the breakage is (which it sounds like you've got) along with stakeholder buy-in that the breakage is worth it and/or there's a migration path.

In that sense this I think it's up to you whether it requires an RFC or not. The embedded working group likely embodies all the stakeholders here so if there's consenus to do this then it can likely be a PR!

A few things I've found helpful for things like this in the past are:

  • An easy flag for nightly users to test out the "to become defaut" behavior. I think this may already be implemented? (if so it'd be good to document here and test locally)
  • An easy flag for nightly users to go back to the old behavior if this change is indeed stabilized, just in case unexpected breakage turns up.

Does that make sense?

japaric added a commit that referenced this issue Aug 25, 2018
bors bot added a commit that referenced this issue Aug 28, 2018
41: use LLD as the default linker r=therealprof,korken89 a=japaric

closes #39

I added instructions on how to switch to a different linker to .cargo/config but
I don't think that's too visible. Beginners are unlikely to look into that file
if they run into problems with the default linker. Any suggestions to improve
the visibility of that information?

Also, don't merge this until the default linker changes for the Cortex-M targets
on nightly as this relies on that change.

r? @rust-embedded/cortex-m

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
bors bot added a commit that referenced this issue Aug 28, 2018
41: use LLD as the default linker r=therealprof a=japaric

closes #39

I added instructions on how to switch to a different linker to .cargo/config but
I don't think that's too visible. Beginners are unlikely to look into that file
if they run into problems with the default linker. Any suggestions to improve
the visibility of that information?

Also, don't merge this until the default linker changes for the Cortex-M targets
on nightly as this relies on that change.

r? @rust-embedded/cortex-m

Co-authored-by: Jorge Aparicio <jorge@japaric.io>
@bors bors bot closed this as completed in #41 Aug 28, 2018
therealfrauholle pushed a commit to therealfrauholle/cortex-m-quickstart that referenced this issue Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants