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

Ship more LLVM tools with the Rust toolchain #49584

Closed
japaric opened this issue Apr 2, 2018 · 18 comments
Closed

Ship more LLVM tools with the Rust toolchain #49584

japaric opened this issue Apr 2, 2018 · 18 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems

Comments

@japaric
Copy link
Member

japaric commented Apr 2, 2018

Summary

Ship llvm tools like llvm-nm and llvm-size with the Rust toolchain.

Rationale

Binary size and performance are critical to embedded development due to the resource constrained
nature of the systems embedded applications run in. Thus it's very important to have tools to
inspect the binaries produced by the compiler.

One could use GNU binutils for this task but that requires installing one set of tools per target
architecture (arm-none-eabi-* for ARM Cortex-M, avr-* for AVR, etc.); and on some platforms
(e.g. Windows) it can be hard to find a set of pre-compiled tools, specially for not yet widespread
architectures (e.g. RISCV).

The alternative to GNU binutils are LLVM tools. Thanks to LLVM being multi-architecture a single
set of LLVM binutils supports several architectures (ARM Cortex-M, AVR, MSP430, RISCV, etc); in
fact, LLVM tools support all the architectures that rustc supports.

By having LLVM tools installed with the Rust toolchain we save users one, possibly difficult,
installation step while also providing them a single set of tools they can use for both native and
embedded development.

Also, we are already shipping LLD with the Rust toolchain to ease WASM development.

Detailed proposal

The following tools will be built and shipped with the Rust toolchain:

  • Binary inspection tools

    • llvm-nm
    • llvm-objdump
    • llvm-size
  • Binary manipulation tools

    • llvm-objcopy, used to transform ELF files into binary files, which are the type of files some
      flashing tools use

Apart from these tools useful for embedded development other tools are being requested by the
general Rust community for profiling:

@japaric (embedded WG) discussed this proposal with @alexcrichton (team-core, team-infra) and we tentatively agreed on the following points:

  • These tools will unconditionally be shipped with the Rust toolchain. That means there won't be a
    llvm-binutils rustup component for them. This is also the case for lld.

  • These tools will only be available on some platforms. Where "some" are mainly tier 1 platforms
    (x86_64 Windows / Linux / macOS). This is also the case for lld.

  • These tools will be installed somewhere in the sysroot but won't be added to the user's $PATH.
    This is also the case for lld.

  • We make no guarantees whatsoever about the stability or availability of these tools. If a LLVM
    upgrade changes the CLI or output format of one of these tools it's up to the end user to adjust
    their invocations accordingly. Similarly, if LLVM drops support for any of these tools then that
    tool will stop being available in the sysroot.

  • The embedded WG will create and maintain at set of Cargo subcommands (e.g. cargo size) that make llvm-{nm,objcopy,objdump,size} available to the end user.

  • We should check how adding these tools affects the binary size of the Rust toolchain installed by
    rustup. These tools are statically linked to LLVM by default, which means they'll be relatively
    large. We can explore dynamically linking them to LLVM in the future.

cc @rust-lang/dev-tools

@japaric
Copy link
Member Author

japaric commented Apr 2, 2018

We should check how adding these tools affects the binary size of the Rust toolchain installed by
rustup.

  • llvm-nm 18 MB
  • llvm-objcopy 3.3 MB
  • llvm-objdump 18 MB
  • llvm-size 3.2 MB
  • all.tar.gz 11 MB
  • all.tar.xz 6.7 MB

For reference, lld was 59 MB.

This data was collected from a fresh LLVM HEAD build. I also noticed that we are already building these tools as part of the compiler build but there we are dynamically linking the tools to libLLVM.so (82 MB).

@est31
Copy link
Member

est31 commented Apr 2, 2018

On one hand it is cool to not need a separate rustup component. On the other hand, you are making the downloads for everyone to take longer. Given that the embedded team has decided on shipping target specific std facades, there is already a setup cost for people when they want to do embedded development. Can't we put all the LLVM tools (including LLD) into a separate rustup component?

It takes a non-zero amount of monthly money to host all the nightlies. LLVM follows a vastly different change pattern than the compiler: it only changes every now and then. It can be deduplicated far more easily if it were isolated into a separate component and we wouldn't add a new copy of the same files to every single nightly tarball.

Maybe this is a question better suited for the future though.... it can be solved in parallel.

We make no guarantees whatsoever about the stability or availability of these tools. If a LLVM
upgrade changes the CLI or output format of one of these tools it's up to the end user to adjust
their invocations accordingly.
[...]
The embedded WG will create and maintain at set of Cargo subcommands (e.g. cargo size) that make llvm-{nm,objcopy,objdump,size} available to the end user.

I generally like the idea of having a cargo size. But the deal with cargo just like with rustc always has been stability, no? So cargo size will only be available on the nightly channel? Adding the llvm tools to the path will mess with linux users who have set up llvm on their own... this isn't easy. Maybe add it to the path while only shipping it to Windows hosts? GNU/Linux and Mac users are able to easily install those tools themselves.

@parched
Copy link
Contributor

parched commented Apr 2, 2018

But the deal with cargo just like with rustc always has been stability, no? So cargo size will only be available on the nightly channel? Adding the llvm tools to the path will mess with linux users who have set up llvm on their own... this isn't easy.

Perhaps wrappers could be added to the path with different names like rustc-size, rustc-objdump, etc.

@whitequark
Copy link
Member

@est31 @parched are you misreading? The LLVM tools are not added to PATH:

These tools will be installed somewhere in the sysroot but won't be added to the user's $PATH.
This is also the case for lld.

@est31
Copy link
Member

est31 commented Apr 3, 2018

The LLVM tools are not added to PATH:

@whitequark I was considering alternatives to the current proposal. I think it is fine to add the tools to PATH on Windows.

@japaric
Copy link
Member Author

japaric commented Apr 3, 2018

On the other hand, you are making the downloads for everyone to take longer.

LLD already made downloads larger and AFAICT no one complained, which probably means that either they didn't notice or they didn't consider it a significant change.

Given that the embedded team has decided on shipping target specific std facades, there is already a setup cost for people when they want to do embedded development.

There's nothing special about the rust-std component for the THUMB targets; you install them once using rustup target add thumbv7m-none-eabi and then they get updated with each rustup update -- just like all other cross compilation targets.

Can't we put all the LLVM tools (including LLD) into a separate rustup component?

I'm trying to remember exactly what @alexcrichton said but he made it sound like this doing that was a non-trivial effort because "rustup doesn't understand the concept of target (host?) specific rustup components" (?) -- the llvm tools are not going to be available for all hosts (triples for which we build and distribute cargo+rustc)

It takes a non-zero amount of monthly money to host all the nightlies. (..)

This sound like an orthogonal optimization. We are already building and caching LLVM artifacts so this only reduces use of storage and not build time -- it has been my impression that we expend more $$$ on CI time than storage, or at least the infra team is always optimizing for build time rather than storage.

I generally like the idea of having a cargo size. But the deal with cargo just like with rustc always has been stability, no?

To clarify cargo size and the like are not going to be built-in Cargo commands. They are going to be external Cargo subcommands (e.g. cargo install cargo-size). Stability of those subcommands is up to the authors, not the Rust / Cargo team.

@alexcrichton
Copy link
Member

I'm personally fine adding these tools and making our downloads a bit larger, if it becomes a problem over time we can continue to shard and create more components

@nrc
Copy link
Member

nrc commented Apr 4, 2018

My instinct was that these tools should be a component. If that is hard we should fix rustup (for some definition of 'we'). I find the download of the Rust distro annoyingly long already (in NZ it is very different from in the US) and don't want to make it longer.

if it becomes a problem over time we can continue to shard and create more components

This seems like it would be a backcompat hazard and people would get mad.

@japaric
Copy link
Member Author

japaric commented Apr 30, 2018

I have opened a PR (#50336) that implements this

bors added a commit that referenced this issue Apr 30, 2018
ship LLVM tools with the toolchain

this PR adds llvm-{nm,objcopy,objdump,size} to the rustc sysroot (right next to LLD)

this slightly increases the size of the rustc component. I measured these numbers on x86_64 Linux:

- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.gz 180M -> 193M (+7%)
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.xz 129M -> 137M (+6%)

r? @alexcrichton
cc #49584
bors added a commit that referenced this issue Jun 21, 2018
ship LLVM tools with the toolchain

this PR adds llvm-{nm,objcopy,objdump,size} to the rustc sysroot (right next to LLD)

this slightly increases the size of the rustc component. I measured these numbers on x86_64 Linux:

- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.gz 180M -> 193M (+7%)
- rustc-1.27.0-dev-x86_64-unknown-linux-gnu.tar.xz 129M -> 137M (+6%)

r? @alexcrichton
cc #49584
@bradjc
Copy link
Contributor

bradjc commented Jun 23, 2018

Now that #50336 has been merged, I tried:

$ rustup install nightly-2018-06-23
$ rustup component add llvm-tools
error: toolchain 'nightly-2018-06-23-x86_64-apple-darwin' does not contain component 'llvm-tools' for target 'x86_64-apple-darwin'

Is that the expected way to add the LLVM tools?

@Mark-Simulacrum
Copy link
Member

#50336 was actually insufficient -- it forgot to mark LlvmTools in src/builder/dist.rs as default when llvm tools are enabled. So looks like we need another PR.

@bradjc
Copy link
Contributor

bradjc commented Jun 26, 2018

Ok $ rustup component add llvm-tools now works!

Am I correct that the next step is a PR to rustup so that llvm-size and the rest show up in the user's path (like rustfmt, cargo, etc.)?

@japaric
Copy link
Member Author

japaric commented Jun 28, 2018

Am I correct that the next step is a PR to rustup so that llvm-size and the rest show up in the user's path (like rustfmt, cargo, etc.)?

No, the upstream work is done. The tools not being in the user PATH is pretty much intentional (see this issue description). Next step is to update cargo-binutils to make the Cargo subcommands use llvm-tools.


One last question to answer here is: Should we make the llvm-tools component nightly only? Or should we let the llvm-tools component ride the train towards stable? cc @alexcrichton @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I think I'm unopposed to this riding the trains -- we expect it to eventually, right? We've also landed this fairly early in the cycle, so I think that would be fine.

@alexcrichton
Copy link
Member

I'd also be ok letting this ride the trains, we don't have a great notion of "nightly only" components right now. Rather our "not yet 1.0" components tend to be called things like foo-preview

@japaric
Copy link
Member Author

japaric commented Jun 28, 2018

@alexcrichton Should we rename the component to llvm-tools-preview then?

Also, I noticed that the tools are installed in $(rustc --print sysroot)/bin. I think they were supposed to be installed in $(rustc --print sysroot)/lib/rustlib/$host/bin, right next to lld. Should we change the installation path to the later?

@alexcrichton
Copy link
Member

@japaric if you're up for it both of those changes sound great to me!

@XAMPPRocky XAMPPRocky added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jun 29, 2018
@japaric
Copy link
Member Author

japaric commented Jun 29, 2018

if you're up for it both of those changes sound great to me!

PR #51922

I think we can close this issue as the bulk of work has been done. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems
Projects
None yet
Development

No branches or pull requests

9 participants