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

Enable strip in release mode by default #4122

Closed
SuperCuber opened this issue Jun 5, 2017 · 32 comments · Fixed by #13257
Closed

Enable strip in release mode by default #4122

SuperCuber opened this issue Jun 5, 2017 · 32 comments · Fixed by #13257
Labels
A-configuration Area: cargo config files and env vars C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review S-needs-team-input Status: Needs input from team on whether/how to proceed. T-cargo Team: Cargo

Comments

@SuperCuber
Copy link

SuperCuber commented Jun 5, 2017

I'd expect build --release to apply all optimisations, and one of them would be file size.

I tried two programs, a trivial "hello rust" and a trivial "hello panic". Both of their sizes were 3.8M which is pretty crazy for something that is supposed to be optimised... So I tried strip --strip-alling the executables.

Hello rust went to 372K
Hello panic went to 364K

That's a pretty huge change... The only difference in functionality I found is that RUST_BACKTRACE=1 shows a lot of <unknown>s on striped files which is to be expected, but I think that's not a real issue because the whole point of --release is that it's not supposed to be debugged, there is the normal build for that...

For now I will be strip --strip-alling all of my released executables but I really don't see a reason why build --release doesn't do it. I'd be happy to be enlightened if I missed anything though!

@ghost
Copy link

ghost commented Jun 5, 2017

I think this should only change if panic = 'abort' to avoid the backtrace getting wiped.

@SuperCuber
Copy link
Author

@cedenday I don't see an issue with backtraces being wiped in --release if it's used in the way it needs to be used though. Also it gives the basic information of where the panic was (file and line number) after striping so it should give enough information to rebuild the project in debug mode and reproduce + backtrace the issue.

@alexcrichton
Copy link
Member

Thanks for the report! I think this is mostly due to the addition of debuginfo into the standard library a few versions ago, so the lion's share of strip is stripping out that debugging information. This does make sense to me to do by default! I'm not sure what the best place for that would be, though. We probably can't literally run strip because it's not very cross-platform so this would probably go into rustc assuming LLVM can handle it, but that'd likely involve a good amount of work.

@carols10cents carols10cents added A-configuration Area: cargo config files and env vars C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build labels Oct 3, 2017
@mati865
Copy link
Contributor

mati865 commented Apr 5, 2018

@alexcrichton maybe LLVM's opt --strip-debug could be handy?
https://llvm.org/docs/CommandGuide/opt.html

For ld/lld --strip-all can be used and it gives the best results.
In my tests on Arch Linux (fairly recent toolchains) RUSTFLAGS="-C link-arg=-s" cargo build --release gave ~3% better results than running strip -s on release binary.

@matklad
Copy link
Member

matklad commented Apr 5, 2018

@mati865 we have a nightly rustc flag for that: rust-lang/rust#49212. I think the idea is, eventually, to enable stripping for --release by default and flip the flag's meaning to disable strip.

@mati865
Copy link
Contributor

mati865 commented Apr 5, 2018

@matklad that's nice but it would be even better if there was option to use --strip-all (-s) instead of --strip-debug (-S).

@jcbritobr
Copy link

Hello. When this will be solved?

@kornelski
Copy link
Contributor

cc #3483

@LifeIsStrange
Copy link

Maybe useful :
https://www.phoronix.com/scan.php?page=news_item&px=LLVM-Strip

@garrett-hopper
Copy link

Should this be closed now that cargo-features = ["strip"] works on nightly?

See: #3483 (comment)

@jcbritobr
Copy link

jcbritobr commented Aug 23, 2020

Should this be closed now that cargo-features = ["strip"] works on nightly?

See: #3483 (comment)

I think not. --release flag is explicit.

@ckcr4lyf
Copy link

ckcr4lyf commented Feb 26, 2022

FYI: With Rust 1.59.0, this can be specified in Cargo.toml as:

[profile.release]
strip = true

and this will strip them in the compile process, rather than needing to explicitly call strip afterwards.

Ref: https://blog.rust-lang.org/2022/02/24/Rust-1.59.0.html#creating-stripped-binaries

@edmorley
Copy link

Should this issue be renamed to be Enable strip in release mode by default, or a new issue filed for that and this one closed out? :-)

@jcbritobr
Copy link

Should this issue be renamed to be Enable strip in release mode by default, or a new issue filed for that and this one closed out? :-)

seems acceptable for me.

@SuperCuber SuperCuber changed the title Build --release doesn't optimise filesize? Enable strip in release mode by default Feb 27, 2022
@SuperCuber
Copy link
Author

Gotchu :)

@epage
Copy link
Contributor

epage commented Oct 12, 2023

Note that there are plans to shrink the amount of debug info by default, see #11958 for some talk of this.

@epage epage added the S-needs-team-input Status: Needs input from team on whether/how to proceed. label Oct 12, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Dec 23, 2023

I would like to propose to change Cargo defaults so that debug = 0 implies strip = "debuginfo". This has been discussed on Zulip several times, most recently here.

Motivation

Currently, when debug = 0 is used in a Cargo profile, Cargo promises that there will be no debuginfo in the resulting binary. However, this is not true, because the standard library contains debuginfo, and this debuginfo is propagated into the resulting binary by default, unless it is stripped.

This means that every binary built by Cargo with its default profile options will contain the stdlib debug symbols, which take about 3-4 MiB on x64 Linux. So, today, no Rust (Linux) binary built with the default profile options can be smaller than 4 MiB! Of course, the symbols can be removed via stripping, but not everyone knows about this.

Notably, when someone new to Rust tries to build a helloworld binary in release mode and then finds out that it takes more than 4 MiB, they might be discouraged and consider Rust binaries to be "bloated".

An example using the latest (1.74.1) stable rustc:

Debug symbols stripped Helloworld release size
Yes 446 KiB
No 4,5 MiB

That's a big difference! Defaults do matter.

What exactly is being proposed

When a Cargo profile should not produce debuginfo (debug is set to 0), and the strip configuration option is not explicitly selected, we should set strip to "debuginfo", to make sure that here will indeed be no debug symbols. Notably, this will have an effect on the default release Cargo profile.

cargo pseudocode:

profile.strip = profile.strip.or_else(|| if profile.debug == "0" { Some("debuginfo") } else { None });

Here is a proposed implementation.

Note that this would also apply to proc macros and build scripts, not just binaries (unless we explicitly do this only for binaries).

Advantages

  • Rust release binaries will be smaller by ~3-4 MiB by default.
  • We will uphold our promise from the Cargo documentation and actually avoid producing debug symbols when debug is set to 0.
  • On Linux, this actually makes compilation slightly faster, since the linker has less work to do. Although this is relevant only for really small crates.

Disadvantages

  • Stack traces of release/debug = 0 builds will contain less information (notably, it will not contain stdlib line numbers). Here is an example of the difference. However, I think that this is not an issue:
    • When the user requests that there will be no debuginfo, it should be the expected behavior… that there will be no debuginfo. The fact that there were (some) debug symbols before was basically just an oversight caused by the way we distribute the standard library. If you want debug symbols, you should just set debug = 1 (or some other value that produces debug symbols).
    • It's unlikely that seeing a bunch of line numbers in stdlib (usually in panic/Result/etc. code) is helpful on its own, without seeing line numbers from the actual application and its dependencies.
  • On macOS, debug symbols are stripped not via the linker, but by invoking a separate strip binary. This can have some performance overhead, but it seems that it might not be large - one experiment showed around a 1% slowdown when compiling cargo itself. ehuss has also mentioned some problems with finding the strip binary on macOS.

Unresolved questions/concerns

  • Should we do this on all platforms by default, or do it in a platform specific way? For example, we might not do it by default on macOS due to the mentioned issues. It depends whether we see this as a performance optimization (smaller binaries by default and slightly faster compilation on Linux) or as a "correctness fix" (cargo should do what it promises in its documentation, and which it doesn't do currently), which might warran a small perf. hit on some platforms.
  • (Note that this is not strictly related to this proposed change, but more to the behavior of the strip option). When at least one dependency wants to produce debug symbols using Cargo overrides, we should probably either not apply strip = "debuginfo" by default, or we should make sure that strip will not actually strip everything when this situation happens. However, it seems to me that overriding debuginfo with overrides currently does not even work. When I tried this:
[dependencies]
serde_json = "1.0.108"

[profile.release]
debug = 1

[profile.release.package.serde_json]
debug = 0

then debug symbols were generated for serde_json. Conversely, when I set debug = 0 for release and debug = 1 for serde_json, then debug symbols were not generated for serde_json. So it seems that overrides for debug symbols don't currently work, and therefore this concern seems orthogonal to this proposal.

@weihanglo weihanglo added the T-cargo Team: Cargo label Dec 23, 2023
@weihanglo
Copy link
Member

Thank you, @Kobzol. Going to kick off an FCP.

@rfcbot fcp merge

Propose to accept this proposal #4122 (comment) for all platforms. For concerns on macOS, we had issues like #11641, which shows the symptom of incompatible strip. To minimize a potential maintenance burden, we could document a troubleshooting process somewhere to let people help themselves.

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 23, 2023

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Dec 23, 2023
@weihanglo weihanglo added the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label Jan 15, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jan 15, 2024

Implementation PR: #13257.

@bors bors closed this as completed in fbf9251 Jan 15, 2024
matte1 added a commit to matte1/rules_rust that referenced this issue Feb 22, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Feb 22, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Feb 22, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Feb 22, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Feb 22, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Mar 26, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
@retep998
Copy link
Member

I consider it to be a process failure that this was proposed and implemented without a single consideration or mention of Windows. That something so dependent on toolchain specifics could go through without even considering all tier1 platforms is embarrassing.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 28, 2024

The problem with backtraces wasn't caused by this change, it only exposed a pre-existing issue, which has been existing in Rust for a long time (4 years, rust-lang/rust@a6c2f73). It's just that no one has noticed it until now, since it started appearing more often. That probably means that not many people use msvc targets on Windows combined with debuginfo stripping.

That something so dependent on toolchain specifics could go through without even considering all tier1 platforms is embarrassing.

I think that's unnecessarily strong wording :) We have a lot of tests for tier 1 platforms, of course, but we didn't have a test for this specific situation.

@retep998
Copy link
Member

retep998 commented Mar 28, 2024

The problem with backtraces wasn't caused by this change, it only exposed a pre-existing issue, which has been existing in Rust for a long time (4 years, rust-lang/rust@a6c2f73). It's just that no one has noticed it until now, since it started appearing more often. That probably means that not many people use msvc targets on Windows combined with debuginfo stripping.

Nobody on Windows MSVC should ever have a reason to consider stripping as binary files never contain debuginfo to begin with. That PR from 4 years ago was definitely wrong to have stripping affect PDB file generation given we previously made the decision to always generate PDB files regardless of whether debuginfo is enabled.

I think that's unnecessarily strong wording :) We have a lot of tests for tier 1 platforms, of course, but we didn't have a test for this specific situation.

Then we should definitely add tests to ensure backtraces work on Windows MSVC in cargo release builds.

@Kobzol
Copy link
Contributor

Kobzol commented Mar 28, 2024

Yeah, we should add tests to rust-lang/rust#115120.

@workingjubilee
Copy link

workingjubilee commented Mar 28, 2024

Porting backtrace-rs tests (which would have caught this if backtrace's CI ran nightly Windows... it's not clear why we weren't, given the CI files had comments about "we only test Windows on nightly") to always run in rustc's test suite is:

I fully intend to prevent this regression from happening ever again. I am also recommending the Rust teams investigate shipping our own strip tool to solve the "some platforms don't believe in providing a good binutils even when they provide binutils" issue:

@workingjubilee
Copy link

backtrace-rs will now also be running Windows CI on nightly.

matte1 added a commit to matte1/rules_rust that referenced this issue Mar 29, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Apr 2, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
github-merge-queue bot pushed a commit to bazelbuild/rules_rust that referenced this issue Apr 2, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Apr 2, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
matte1 added a commit to matte1/rules_rust that referenced this issue Apr 2, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
github-merge-queue bot pushed a commit to bazelbuild/rules_rust that referenced this issue Apr 2, 2024
Attempts to follow the cargo proposal linked below to remove debug info
for release builds. Furthermore this should uphold the expected behavior
of bazel for cc builds which automatically strips debug symbols for
optimized builds.

rust-lang/cargo#4122 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-build disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review S-needs-team-input Status: Needs input from team on whether/how to proceed. T-cargo Team: Cargo
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.