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

Consider using llvm-strip #123151

Open
workingjubilee opened this issue Mar 28, 2024 · 14 comments
Open

Consider using llvm-strip #123151

workingjubilee opened this issue Mar 28, 2024 · 14 comments
Labels
A-codegen Area: Code generation C-discussion Category: Discussion or questions that doesn't represent real issues. O-illumos the other shiny OS O-linux Operating system: Linux O-macos Operating system: macOS T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Mar 28, 2024

Historically, when handling issues like "the tools on the system we are delivering the toolchain to, for a particular target arch, have lots of individual quirks", Rust has had two primary moves:

  • Allow the tools we use to be completely configured via flags
  • Ship our own tool and teach the compiler to prefer that for that host/target

On macOS various systems, we are encountering reports in the wild of people having bad inadequately-built strip binaries in their PATH that cannot support all Rust compiler use-cases12 (e.g. GNU binutils that is built with only host platform support).

In particular, this means:

  • sometimes cargo run --release produces binaries that get SIGKILLed on macOS
  • sometimes cross-compiled binaries, even between ELF/DWARF hosts and targets, have broken stacktraces, because the strip isn't aware it should be sensitive to the needs of that target platform's debugging infra.

We can try to work with Nix, Homebrew, and apparently illumos and most Linux distros to solve this problem, but if this is a persistent issue, and considering the somewhat mysterious errors that result, it may be most productive to simply ship our own copy of strip on these hosts. I believe llvm-strip gets everything right, as far as we are concerned, and would be the choice to "bless" accordingly.

Even if it has its own problems, it would be far easier to ship desired patches to binary support we control, or invite people to push fixes to upstream: we'd just need to cooperate with LLVM (or whoever else supplies this), rather than relying on every single distro cooperating perfectly with us. And I think that it's probably worth it: llvm-strip costs only about 175KB on my system, and probably a little bit of build infra, but pays for itself several times over, obviously: every Rust binary built with the toolchain, thanks to enabling strip, is a few MB lighter for its --release mode.

Footnotes

  1. On macOS this is especially bad. The local toolchain comes with llvm-strip which supports mach-o correctly, but in practice, no one uses the local toolchain exclusively. Yeah, XCode is the standard for merely building something, but in order to make a really complete development environment on macOS, you have to add something, like Homebrew, MacPorts, or Nix. So people on macOS are constantly injecting odd strip binaries into their PATH that can't even support the host, leading to us using one that is flawed. These tools try to compensate for the inevitable odd system configurations that result but do so imperfectly, resulting in erroneous behavior.

  2. By default, it seems e.g. Nix only builds with host support, because the cost of all-targets support is about 60 more MB. I'm sure they're not the only one. 'tis a very "don't pay for what you don't need" mindset to make you install every single cross-build target individually and pay a few dozen MB each time, instead of paying double, once.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 28, 2024
@workingjubilee workingjubilee changed the title Consider shipping llvm-strip Consider shipping llvm-strip for macOS Mar 28, 2024
@workingjubilee workingjubilee added O-macos Operating system: macOS A-codegen Area: Code generation T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 28, 2024
@workingjubilee
Copy link
Member Author

This obviously was exacerbated by the recent decision to strip in --release but I should note it was very much preexisting. And I'm not sure who exactly would be responsible for this decision, seems like a shared one.

@workingjubilee
Copy link
Member Author

workingjubilee commented Mar 28, 2024

There are other options, of course, like trying to respect e.g. a $STRIP environment variable, I just don't know if other compilers already respect any such variable and thus if there would be any precedent/intuition/pattern we would find useful. These choices are also not mutually exclusive, either.

It may also be the case that this is sufficiently easy-enough to fix that the package management tools in question will sort this out and we should just twiddle our thumbs for a couple weeks. That would be mildly preferable to macOS hosts having Yet Another strip binary, even if the waste is trivial. I'm mostly just trying to collect the information that we have floating around.

taiki-e referenced this issue in nextest-rs/nextest Mar 28, 2024
Restore behavior changed in 4e259a7, because
it turns out that `strip = "debuginfo"` still strips too much.

This was observed with the cargo-nextest 0.9.68-rc.1 binary on illumos, where
`pstack` showed lots of ??? marks.

Part of #1345.
@taiki-e
Copy link
Member

taiki-e commented Mar 28, 2024

I think this would also help to fix the problem with strip in cross-compilation (#114411, and if my guess is correct illumos has the same issue not only macOS).

@workingjubilee
Copy link
Member Author

y'all really out here shipping binutils with only one arch supported when ./configure --enable-targets=all is right there, huh

@workingjubilee workingjubilee changed the title Consider shipping llvm-strip for macOS Consider shipping llvm-strip Mar 28, 2024
@workingjubilee
Copy link
Member Author

At least one prominent Linux distribution has informed me that their strip does not have cross-compilation support.

@workingjubilee workingjubilee added O-linux Operating system: Linux O-illumos the other shiny OS labels Mar 28, 2024
@the8472
Copy link
Member

the8472 commented Mar 28, 2024

despite the local toolchain having an applicable, useful strip

Another option would be looking at all the available strips on the PATH and picking a preferred one / skipping bad ones. Assuming they can be easily identified.

At least one prominent Linux distribution has informed me that their strip does not have cross-compilation support.

Does that result in an error or in a corrupted binary like on macos? The former doesn't seem so bad since we can turn that into a warning or error.

@jieyouxu jieyouxu added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 28, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Mar 28, 2024

Some relevant outputs. --version is the usual, --info lists the supported architectures of this binutils.

$ strip --version
GNU strip (GNU Binutils) 2.42.0
Copyright (C) 2024 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License version 3 or (at your option) any later version.
This program has absolutely no warranty.
$ llvm-strip --version
llvm-strip, compatible with GNU strip
LLVM (http://llvm.org/):
  LLVM version 17.0.6
  Optimized build.
$ strip --info
BFD header file version (GNU Binutils) 2.42.0
elf64-x86-64
 (header little endian, data little endian)
  i386
elf32-i386
 (header little endian, data little endian)
  i386
elf32-iamcu
 (header little endian, data little endian)
  iamcu
elf32-x86-64
 (header little endian, data little endian)
  i386
pei-i386
 (header little endian, data little endian)
  i386
pe-x86-64
 (header little endian, data little endian)
  i386
pei-x86-64
 (header little endian, data little endian)
  i386
elf64-little
 (header little endian, data little endian)
  i386
  iamcu
  bpf
elf64-big
 (header big endian, data big endian)
  i386
  iamcu
  bpf
elf32-little
 (header little endian, data little endian)
  i386
  iamcu
  bpf
elf32-big
 (header big endian, data big endian)
  i386
  iamcu
  bpf
pe-bigobj-x86-64
 (header little endian, data little endian)
  i386
pe-i386
 (header little endian, data little endian)
  i386
pdb
 (header little endian, data little endian)
elf64-bpfle
 (header little endian, data little endian)
  bpf
elf64-bpfbe
 (header big endian, data big endian)
  bpf
srec
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
symbolsrec
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
verilog
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
tekhex
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
binary
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
ihex
 (header endianness unknown, data endianness unknown)
  i386
  iamcu
  bpf
plugin
 (header little endian, data little endian)

         elf64-x86-64 elf32-i386 elf32-iamcu elf32-x86-64 pei-i386 pe-x86-64 
    i386 elf64-x86-64 elf32-i386 ----------- elf32-x86-64 pei-i386 pe-x86-64
   iamcu ------------ ---------- elf32-iamcu ------------ -------- ---------
     bpf ------------ ---------- ----------- ------------ -------- ---------

         pei-x86-64 elf64-little elf64-big elf32-little elf32-big 
    i386 pei-x86-64 elf64-little elf64-big elf32-little elf32-big
   iamcu ---------- elf64-little elf64-big elf32-little elf32-big
     bpf ---------- elf64-little elf64-big elf32-little elf32-big

         pe-bigobj-x86-64 pe-i386 pdb elf64-bpfle elf64-bpfbe srec symbolsrec 
    i386 pe-bigobj-x86-64 pe-i386 --- ----------- ----------- srec symbolsrec
   iamcu ---------------- ------- --- ----------- ----------- srec symbolsrec
     bpf ---------------- ------- --- elf64-bpfle elf64-bpfbe srec symbolsrec

         verilog tekhex binary ihex plugin 
    i386 verilog tekhex binary ihex ------
   iamcu verilog tekhex binary ihex ------
     bpf verilog tekhex binary ihex ------
$ llvm-strip --info
llvm-strip: error: unknown argument '--info'

@workingjubilee workingjubilee added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Mar 28, 2024
@workingjubilee
Copy link
Member Author

I'm going to push this into T-compiler's agenda for next week as the possible solutions all involve affecting rustc's behavior at least mildly.

@workingjubilee
Copy link
Member Author

Hmm. I spent some time dinking about and it seems we do in fact ship llvm-tools as a rustup component and they are no longer a "preview" anything, so the question would be if they should be considered effectively mandatory or not.

@ChrisDenton
Copy link
Member

There was a discussion on zulip about llvm-tools.

@wesleywiser
Copy link
Member

I think using llvm-strip is entirely reasonable here especially given the extremely small size of the tool. If we don't want to rely on llvm-tools being installed, I think it would be reasonable to ship it in the sysroot (possibly duplicating it with one in llvm-tools).

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 11, 2024
@Mark-Simulacrum
Copy link
Member

Please don't rely on llvm-tools for this purpose, it's lack of -preview was just an accident (not an intentional stabilization).

It's also a larger component and we shouldn't make it downloaded by default for users, IMO, and as such the experience of rustc/cargo shouldn't degrade without it. I think duplicating llvm-strip as a sibling to rust-lld makes a lot of sense to give a uniform experience for users.

@workingjubilee
Copy link
Member Author

workingjubilee commented Apr 18, 2024

Okay cool! Then it sounds like folks are largely on-board with "ship llvm-strip, but not the full llvm-tools component, just llvm-strip", and it's mostly down to PRing the changes. Will try to take a look at that in a few days.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 5, 2024
…=jieyouxu,albertlarsan68

bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip

Fixes rust-lang#131206.

- Includes `llvm-strip` (a symlink to `llvm-objcopy`) in the compiler dist artifact so that it can be used for `-Cstrip` instead of the system tooling.
- Uses `llvm-strip` instead of `/usr/bin/strip` for macOS. macOS needs a specific linker and the system one is preferred, hence rust-lang#130781 but that doesn't work when cross-compiling, so use the `llvm-strip` utility instead.

cc rust-lang#123151
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 6, 2024
Rollup merge of rust-lang#131405 - davidtwco:hardcoded-strip-macos, r=jieyouxu,albertlarsan68

bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip

Fixes rust-lang#131206.

- Includes `llvm-strip` (a symlink to `llvm-objcopy`) in the compiler dist artifact so that it can be used for `-Cstrip` instead of the system tooling.
- Uses `llvm-strip` instead of `/usr/bin/strip` for macOS. macOS needs a specific linker and the system one is preferred, hence rust-lang#130781 but that doesn't work when cross-compiling, so use the `llvm-strip` utility instead.

cc rust-lang#123151
@Noratrieb
Copy link
Member

I think this was fixed in #131405. Is that a correct assessment? @workingjubilee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-discussion Category: Discussion or questions that doesn't represent real issues. O-illumos the other shiny OS O-linux Operating system: Linux O-macos Operating system: macOS T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants