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

Wrapper for -Z gcc-ld=lld to invoke rust-lld with the correct flavor #89288

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Sep 26, 2021

This PR adds an lld-wrapper tool which is installed as ld and ld64 in lib\rustlib\<host_target>\bin\gcc-ld directory and whose sole purpose is to invoke rust-lld in the parent directory with the correct flavor. Lld decides which flavor to use from either the first two commandline arguments or from the name of the executable (ld for GNU/ld flavor, ld64 for Darwin/Macos/ld64 flavor and so on). Symbolic links could not be used as they are not supported by rustup and on Windows.

The wrapper replaces full copies of rust-lld which added some significant bloat. On UNIXish operating systems it exec rust-lld, on Windows it spawns it as a child process.

Fixes #88869.

r? @Mark-Simulacrum
cc @nagisa @petrochenkov @1000teslas

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 26, 2021
@nagisa
Copy link
Member

nagisa commented Sep 26, 2021

Out of curiosity, what's the size of this wrapper?

@hkratz
Copy link
Contributor Author

hkratz commented Sep 26, 2021

Out of curiosity, what's the size of this wrapper?

On Linux the wrapper is 548kb, on Macos 424kb, haven't checked Windows yet.

The rust-lld distributed with the current bootstrap compiler is only about 6.5mb on Linux with libLLVM-13.so linked in dynamically. On Macos rust-lld as distributed with the current bootstrap compiler does apparently not link to the llvm library dynamically and weighs in at about 73mb, haven't checked if that is on purpose.

@est31
Copy link
Member

est31 commented Sep 27, 2021

I wonder if it can be reduced further with https://github.com/johnthagen/min-sized-rust or so.

@tschuett
Copy link

Could this be a Python script instead to save space?

@est31
Copy link
Member

est31 commented Sep 27, 2021

@tschuett then it'd require python to be installed on the system. Same goes for bash scripts. I wonder though whether one could build something smaller, like something with no_std that just calls execve on unixes and whatever windows has on windows. Half a megabyte is definitely a lot for a binary whose main job is to just call another binary.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 27, 2021

FWIW, just out of curiosity, I tried to build it with

[profile.release]
opt-level = "z"
lto = true
codegen-units = 1
panic = "abort"

and build-std (cargo +nightly build -Z build-std=std,panic_abort -Z build-std-features=panic_immediate_abort --target x86_64-unknown-linux-gnu --release --features ld).

On Linux, it got the executable size after stripping from ~250 KiB to 50 KiB.

@est31
Copy link
Member

est31 commented Sep 27, 2021

Stripping is probably non trivial but one could probably add debug = false to the profile.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 27, 2021

I probably measured it wrong before, even without stripping it sits at 75 KiB (debug = false doesn't change anything, I suppose that it's the default value in release?).

I also looked at why the binary is so big with cargo-bloat (but for that I had to turn off LTO, so the results might be skewed.. Most of the space was taken by backtrace and symbol resolving (addr2line, gimli). Maybe also Command can be an issue (#64140). But with build-std it's reduced quite well.

@hkratz
Copy link
Contributor Author

hkratz commented Sep 27, 2021

Tried the minimization on Windows, the binary went from 300kb to 150kb.

The installed size of the x86_64 Windows toolchains are:

$ du -sm .rustup/toolchains/*
1056    .rustup/toolchains/beta-x86_64-pc-windows-msvc
1083    .rustup/toolchains/nightly-x86_64-pc-windows-msvc
961     .rustup/toolchains/stable-x86_64-pc-windows-msvc

So while reducing the ld.exe + ld64.exe overhead from 120mb to 600kb has an impact, further reducing it from 600kb to 300kb or less is not really worth the build time increase of -Z build-std IMHO.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 27, 2021

It might be a bit overkill, I agree.

Although, on Linux at least, stripping the binary should be trivial, and could be done on a "best effort" basis (if strip is not available, just don't strip it).

@hkratz
Copy link
Contributor Author

hkratz commented Sep 27, 2021

@Mark-Simulacrum If I use ToolBootstrap I have to always use the bootstrap compiler otherwise it asserts. Isn't that a problem when cross-compiling? I will do some testing with cross-compilation.

@Mark-Simulacrum
Copy link
Member

I don't think we need to bother with trying to get these down to even smaller sizes; several hundred kilobytes is tiny compared to the rest of the rustc tarball, and so cutting that even a little ultimately doesn't matter that much IMO.

@Mark-Simulacrum If I use ToolBootstrap I have to always use the bootstrap compiler otherwise it asserts. Isn't that a problem when cross-compiling? I will do some testing with cross-compilation.

I'm not sure I follow. Can you say what assertion you're seeing? The bootstrap compiler should be capable of cross-compiling to ~any architecture (unless it's been added within the last release cycle, but I don't think we need to worry about that -- targets don't jump to having host tools from the get go). You might need a builder.ensure(compile::Std { compiler, target }); though.

@hkratz
Copy link
Contributor Author

hkratz commented Sep 27, 2021

It only panics if the wrapper is built as ToolBootstrap with a stage 1 compiler. That is probably to be expected:

thread 'main' panicked at 'assertion failed: !use_snapshot || stage == 0 || self.local_rebuild', src/bootstrap/builder.rs:1077:9
s

With the latest commit it always uses the bootstrap compiler so the panic is gone. If using the bootstrap compiler is fine for cross-compiling the PR is ready for re-review.

@Mark-Simulacrum
Copy link
Member

Yes, that's expected, but shouldn't happen with the latest changes as you indicated :)

OK, great, then I think we can likely go ahead with this. I'm not sure that current_exe() will work in all cases -- I know we've had problems with it in the past -- but I also don't see an easy alternative, so it seems OK for now. We can always iterate on this in the future.

Can you squash the commits? r=me with that done.

@tschuett
Copy link

You can't have cc pass -flavor darwin to the linker (rust-lld)?

@hkratz
Copy link
Contributor Author

hkratz commented Sep 27, 2021

You can't have cc pass -flavor darwin to the linker (rust-lld)?

With either -Wl,<option> or -Xlinker <option> clang adds <option> after other options. lld requires them to be the first two arguments.

@hkratz
Copy link
Contributor Author

hkratz commented Sep 28, 2021

@Mark-Simulacrum Squashed.

Is it too late to consider it for the beta? Even if there were problems with the wrapper, -Z gcc-ld=lld does not work on stable anyway without RUSTC_BOOTSTRAP=1 shenanigans.

@Mark-Simulacrum
Copy link
Member

Yes, we don't generally backport anything except regression fixes (which this isn't, as it targets an unstable feature).

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2021

📌 Commit ac081e1 has been approved by Mark-Simulacrum

@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 Oct 2, 2021
@hkratz
Copy link
Contributor Author

hkratz commented Oct 2, 2021

@Mark-Simulacrum I think I was a bit unclear. The installed size increase of >100MB from stable to beta on Windows/Macos affects everyone and not just users of the unstable feature. That is why the linked issue #88869 is marked as regression-from-stable-to-beta with medium priority.

The risk of a backport is low as even if the wrapper has bugs it would not affect people using beta/stable.

@Mark-Simulacrum
Copy link
Member

Ah, I see. I think a backport makes sense then. Thanks!

@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 Oct 5, 2021
@Mark-Simulacrum
Copy link
Member

Alright, r=me with commits squashed. Thanks!

The wrapper is installed as `ld` and `ld64` in the `lib\rustlib\<host_target>\bin\gcc-ld`
directory and its sole purpose is to invoke `rust-lld` in the parent directory with
the correct flavor.
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 7, 2021

📌 Commit 6162fc0 has been approved by Mark-Simulacrum

@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 Oct 7, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 7, 2021
…ulacrum

Wrapper for `-Z gcc-ld=lld` to invoke rust-lld with the correct flavor

This PR adds an `lld-wrapper` tool which is installed as `ld` and `ld64` in `lib\rustlib\<host_target>\bin\gcc-ld` directory and whose sole purpose is to invoke `rust-lld` in the parent directory with the correct flavor. Lld decides which flavor to use from either the first two commandline arguments or from the name of the executable (`ld` for GNU/ld flavor, `ld64` for Darwin/Macos/ld64 flavor and so on). Symbolic links could not be used as they are not supported by rustup and on Windows.

The wrapper replaces full copies of rust-lld which added some significant bloat. On UNIXish operating systems it exec rust-lld, on Windows it spawns it as a child process.

Fixes rust-lang#88869.

r? `@Mark-Simulacrum`
cc `@nagisa` `@petrochenkov` `@1000teslas`
Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 8, 2021
…ulacrum

Wrapper for `-Z gcc-ld=lld` to invoke rust-lld with the correct flavor

This PR adds an `lld-wrapper` tool which is installed as `ld` and `ld64` in `lib\rustlib\<host_target>\bin\gcc-ld` directory and whose sole purpose is to invoke `rust-lld` in the parent directory with the correct flavor. Lld decides which flavor to use from either the first two commandline arguments or from the name of the executable (`ld` for GNU/ld flavor, `ld64` for Darwin/Macos/ld64 flavor and so on). Symbolic links could not be used as they are not supported by rustup and on Windows.

The wrapper replaces full copies of rust-lld which added some significant bloat. On UNIXish operating systems it exec rust-lld, on Windows it spawns it as a child process.

Fixes rust-lang#88869.

r? ``@Mark-Simulacrum``
cc ``@nagisa`` ``@petrochenkov`` ``@1000teslas``
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 8, 2021
…ingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#87918 (Enable AutoFDO.)
 - rust-lang#88137 (On macOS, make strip="symbols" not pass any options to strip)
 - rust-lang#88772 (Fixed confusing wording on Result::map_or_else.)
 - rust-lang#89025 (Implement `#[link_ordinal(n)]`)
 - rust-lang#89082 (Implement rust-lang#85440 (Random test ordering))
 - rust-lang#89288 (Wrapper for `-Z gcc-ld=lld` to invoke rust-lld with the correct flavor)
 - rust-lang#89476 (Correct decoding of foreign expansions during incr. comp.)
 - rust-lang#89622 (Use correct edition for panic in [debug_]assert!().)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1c1c6ed into rust-lang:master Oct 8, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 8, 2021
@cuviper cuviper mentioned this pull request Oct 13, 2021
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 13, 2021
@cuviper cuviper modified the milestones: 1.57.0, 1.56.0 Oct 13, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2021
[beta] backports

- 2229: Consume IfLet expr rust-lang#89282
- Wrapper for -Z gcc-ld=lld to invoke rust-lld with the correct flavor rust-lang#89288
- Fix unsound optimization with explicit variant discriminants rust-lang#89489
- Fix stabilization version for bindings_after_at rust-lang#89605
- Turn vtable_allocation() into a query rust-lang#89619
- Revert "Stabilize Iterator::intersperse()" rust-lang#89638
- Ignore type of projections for upvar capturing rust-lang#89648
- ~~Add Poll::ready and~~ revert stabilization of task::ready! rust-lang#89651
- CI: Use mirror for libisl downloads for more docker dist builds rust-lang#89661
-  Use correct edition for panic in [debug_]assert!(). rust-lang#89622
-  Switch to our own mirror of libisl plus ct-ng oldconfig fixes rust-lang#89599
-  Emit item no type error even if type inference fails rust-lang#89585
-  Revert enum discriminants rust-lang#89884
@hkratz hkratz deleted the lld_wrapper branch November 6, 2021 20:37
@petrochenkov
Copy link
Contributor

petrochenkov commented May 24, 2022

I missed this PR and only noticed the lld-wrapper tool later.

I strongly suspect that separate tool is an unproportionately large hammer and not the best solution, so I made #97352.
(I didn't yet look at this in detail though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doublicated ld\ld64 executables added into rustc package, increasing it's size