Skip to content

Commit fbf9251

Browse files
borsbors
committed
Auto merge of #13257 - Kobzol:profile-strip-debuginfo, r=ehuss
Strip debuginfo when debuginfo is not requested ### What does this PR try to resolve? This PR implements [this proposal](#4122 (comment)). It contains a detailed description of the change. As a summary, this PR modifies Cargo so that if the user doesn't set `strip` explicitly, and debuginfo is not enabled for any package being compiled, Cargo will implicitly set `strip = "debuginfo"`, to strip pre-existing debuginfo coming from the standard library. This reduces the default size of release binaries considerably (~4.5 MiB => ~450 KiB for helloworld on Linux x64). Perhaps we could only add the `-Cstrip` option for the leaf/root target (i.e., a binary), but cargo already passes `-Cstrip` to libraries if it's set by `[profile.<...>.package.<lib>]`, so it seems harmless. Fixes: #4122 ### How should we test and review this PR? Best reviewed commit by commit. There is one commit that fixes an existing related test that was using wrong assertion. ### Additional information The implementation of the deferred option was inspired by `DebugInfo`, which already uses a similar concept. ### Unresolved questions Should we also do this on macOS by default? It [seems](#11641) that there can be some issues with `strip` there. If it doesn't work, it basically inhibits compilation in release mode, with no easy way to opt out (unless the user explicitly requests debuginfo, but that's not the same as the previous state).
2 parents d90d321 + e954099 commit fbf9251

File tree

6 files changed

+174
-18
lines changed

6 files changed

+174
-18
lines changed

‎src/cargo/core/compiler/mod.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ use self::unit_graph::UnitDep;
8888
use crate::core::compiler::future_incompat::FutureIncompatReport;
8989
pub use crate::core::compiler::unit::{Unit, UnitInterner};
9090
use crate::core::manifest::TargetSourcePath;
91-
use crate::core::profiles::{PanicStrategy, Profile, Strip};
91+
use crate::core::profiles::{PanicStrategy, Profile, StripInner};
9292
use crate::core::{Feature, PackageId, Target, Verbosity};
9393
use crate::util::errors::{CargoResult, VerboseError};
9494
use crate::util::interning::InternedString;
@@ -1135,7 +1135,8 @@ fn build_base_args(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, unit: &Unit)
11351135
opt(cmd, "-C", "incremental=", Some(dir));
11361136
}
11371137

1138-
if strip != Strip::None {
1138+
let strip = strip.into_inner();
1139+
if strip != StripInner::None {
11391140
cmd.arg("-C").arg(format!("strip={}", strip));
11401141
}
11411142

‎src/cargo/core/profiles.rs

+77-11
Original file line numberDiff line numberDiff line change
@@ -573,10 +573,17 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
573573
profile.trim_paths = Some(trim_paths.clone());
574574
}
575575
profile.strip = match toml.strip {
576-
Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")),
577-
None | Some(StringOrBool::Bool(false)) => Strip::None,
578-
Some(StringOrBool::String(ref n)) if n.as_str() == "none" => Strip::None,
579-
Some(StringOrBool::String(ref n)) => Strip::Named(InternedString::new(n)),
576+
Some(StringOrBool::Bool(true)) => {
577+
Strip::Resolved(StripInner::Named(InternedString::new("symbols")))
578+
}
579+
Some(StringOrBool::Bool(false)) => Strip::Resolved(StripInner::None),
580+
Some(StringOrBool::String(ref n)) if n.as_str() == "none" => {
581+
Strip::Resolved(StripInner::None)
582+
}
583+
Some(StringOrBool::String(ref n)) => {
584+
Strip::Resolved(StripInner::Named(InternedString::new(n)))
585+
}
586+
None => Strip::Deferred(StripInner::None),
580587
};
581588
}
582589

@@ -636,7 +643,7 @@ impl Default for Profile {
636643
rpath: false,
637644
incremental: false,
638645
panic: PanicStrategy::Unwind,
639-
strip: Strip::None,
646+
strip: Strip::Deferred(StripInner::None),
640647
rustflags: vec![],
641648
trim_paths: None,
642649
}
@@ -873,28 +880,87 @@ impl fmt::Display for PanicStrategy {
873880
}
874881
}
875882

876-
/// The setting for choosing which symbols to strip
877883
#[derive(
878884
Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize,
879885
)]
880-
#[serde(rename_all = "lowercase")]
881-
pub enum Strip {
886+
pub enum StripInner {
882887
/// Don't remove any symbols
883888
None,
884889
/// Named Strip settings
885890
Named(InternedString),
886891
}
887892

888-
impl fmt::Display for Strip {
893+
impl fmt::Display for StripInner {
889894
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
890895
match *self {
891-
Strip::None => "none",
892-
Strip::Named(s) => s.as_str(),
896+
StripInner::None => "none",
897+
StripInner::Named(s) => s.as_str(),
893898
}
894899
.fmt(f)
895900
}
896901
}
897902

903+
/// The setting for choosing which symbols to strip.
904+
///
905+
/// This is semantically a [`StripInner`], and should be used as so via the
906+
/// [`Strip::into_inner`] method for all intents and purposes.
907+
///
908+
/// Internally, it's used to model a strip option whose value can be deferred
909+
/// for optimization purposes: when no package being compiled requires debuginfo,
910+
/// then we can strip debuginfo to remove pre-existing debug symbols from the
911+
/// standard library.
912+
#[derive(Clone, Copy, Debug, Eq, serde::Serialize, serde::Deserialize)]
913+
#[serde(rename_all = "lowercase")]
914+
pub enum Strip {
915+
/// A strip option that is fixed and will not change.
916+
Resolved(StripInner),
917+
/// A strip option that might be overridden by Cargo for optimization
918+
/// purposes.
919+
Deferred(StripInner),
920+
}
921+
922+
impl Strip {
923+
/// The main way to interact with this strip option, turning it into a [`StripInner`].
924+
pub fn into_inner(self) -> StripInner {
925+
match self {
926+
Strip::Resolved(v) | Strip::Deferred(v) => v,
927+
}
928+
}
929+
930+
pub(crate) fn is_deferred(&self) -> bool {
931+
matches!(self, Strip::Deferred(_))
932+
}
933+
934+
/// Reset to stripping debuginfo.
935+
pub(crate) fn strip_debuginfo(self) -> Self {
936+
Strip::Resolved(StripInner::Named("debuginfo".into()))
937+
}
938+
}
939+
940+
impl PartialEq for Strip {
941+
fn eq(&self, other: &Self) -> bool {
942+
self.into_inner().eq(&other.into_inner())
943+
}
944+
}
945+
946+
impl Hash for Strip {
947+
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
948+
self.into_inner().hash(state);
949+
}
950+
}
951+
952+
impl PartialOrd for Strip {
953+
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
954+
self.into_inner().partial_cmp(&other.into_inner())
955+
}
956+
}
957+
958+
impl Ord for Strip {
959+
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
960+
self.into_inner().cmp(&other.into_inner())
961+
}
962+
}
963+
898964
/// Flags used in creating `Unit`s to indicate the purpose for the target, and
899965
/// to ensure the target's dependencies have the correct settings.
900966
///

‎src/cargo/ops/cargo_compile/mod.rs

+12
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,18 @@ fn traverse_and_share(
657657
};
658658

659659
let mut profile = unit.profile.clone();
660+
if profile.strip.is_deferred() {
661+
// If strip was not manually set, and all dependencies of this unit together
662+
// with this unit have debuginfo turned off, we enable debuginfo stripping.
663+
// This will remove pre-existing debug symbols coming from the standard library.
664+
if !profile.debuginfo.is_turned_on()
665+
&& new_deps
666+
.iter()
667+
.all(|dep| !dep.unit.profile.debuginfo.is_turned_on())
668+
{
669+
profile.strip = profile.strip.strip_debuginfo();
670+
}
671+
}
660672

661673
// If this is a build dependency, and it's not shared with runtime dependencies, we can weaken
662674
// its debuginfo level to optimize build times. We do nothing if it's an artifact dependency,

‎tests/testsuite/profiles.rs

+76-1
Original file line numberDiff line numberDiff line change
@@ -607,7 +607,82 @@ fn strip_accepts_false_to_disable_strip() {
607607
.build();
608608

609609
p.cargo("build --release -v")
610-
.with_stderr_does_not_contain("-C strip")
610+
.with_stderr_does_not_contain("[RUNNING] `rustc [..] -C strip[..]`")
611+
.run();
612+
}
613+
614+
#[cargo_test]
615+
fn strip_debuginfo_in_release() {
616+
let p = project()
617+
.file(
618+
"Cargo.toml",
619+
r#"
620+
[package]
621+
name = "foo"
622+
version = "0.1.0"
623+
"#,
624+
)
625+
.file("src/main.rs", "fn main() {}")
626+
.build();
627+
628+
p.cargo("build --release -v")
629+
.with_stderr_contains("[RUNNING] `rustc [..] -C strip=debuginfo[..]`")
630+
.run();
631+
}
632+
633+
#[cargo_test]
634+
fn strip_debuginfo_without_debug() {
635+
let p = project()
636+
.file(
637+
"Cargo.toml",
638+
r#"
639+
[package]
640+
name = "foo"
641+
version = "0.1.0"
642+
643+
[profile.dev]
644+
debug = 0
645+
"#,
646+
)
647+
.file("src/main.rs", "fn main() {}")
648+
.build();
649+
650+
p.cargo("build -v")
651+
.with_stderr_contains("[RUNNING] `rustc [..] -C strip=debuginfo[..]`")
652+
.run();
653+
}
654+
655+
#[cargo_test]
656+
fn do_not_strip_debuginfo_with_requested_debug() {
657+
let p = project()
658+
.file(
659+
"Cargo.toml",
660+
r#"
661+
[package]
662+
name = "foo"
663+
version = "0.1.0"
664+
665+
[dependencies]
666+
bar = { path = "bar" }
667+
668+
[profile.release.package.bar]
669+
debug = 1
670+
"#,
671+
)
672+
.file("src/main.rs", "fn main() {}")
673+
.file(
674+
"bar/Cargo.toml",
675+
r#"
676+
[package]
677+
name = "bar"
678+
verison = "0.1.0"
679+
"#,
680+
)
681+
.file("bar/src/lib.rs", "")
682+
.build();
683+
684+
p.cargo("build --release -v")
685+
.with_stderr_does_not_contain("[RUNNING] `rustc [..] -C strip=debuginfo[..]`")
611686
.run();
612687
}
613688

‎tests/testsuite/run.rs

+2
Original file line numberDiff line numberDiff line change
@@ -790,13 +790,15 @@ fn example_with_release_flag() {
790790
-C opt-level=3[..]\
791791
-C metadata=[..] \
792792
--out-dir [CWD]/target/release/deps \
793+
-C strip=debuginfo \
793794
-L dependency=[CWD]/target/release/deps`
794795
[COMPILING] foo v0.0.1 ([CWD])
795796
[RUNNING] `rustc --crate-name a examples/a.rs [..]--crate-type bin \
796797
--emit=[..]link \
797798
-C opt-level=3[..]\
798799
-C metadata=[..] \
799800
--out-dir [CWD]/target/release/examples \
801+
-C strip=debuginfo \
800802
-L dependency=[CWD]/target/release/deps \
801803
--extern bar=[CWD]/target/release/deps/libbar-[..].rlib`
802804
[FINISHED] release [optimized] target(s) in [..]

‎tests/testsuite/unit_graph.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ fn simple() {
8181
"panic": "unwind",
8282
"rpath": false,
8383
"split_debuginfo": "{...}",
84-
"strip": "none"
84+
"strip": "{...}"
8585
},
8686
"target": {
8787
"crate_types": [
@@ -126,7 +126,7 @@ fn simple() {
126126
"panic": "unwind",
127127
"rpath": false,
128128
"split_debuginfo": "{...}",
129-
"strip": "none"
129+
"strip": "{...}"
130130
},
131131
"target": {
132132
"crate_types": [
@@ -164,7 +164,7 @@ fn simple() {
164164
"panic": "unwind",
165165
"rpath": false,
166166
"split_debuginfo": "{...}",
167-
"strip": "none"
167+
"strip": "{...}"
168168
},
169169
"target": {
170170
"crate_types": [
@@ -207,7 +207,7 @@ fn simple() {
207207
"panic": "unwind",
208208
"rpath": false,
209209
"split_debuginfo": "{...}",
210-
"strip": "none"
210+
"strip": "{...}"
211211
},
212212
"target": {
213213
"crate_types": [

0 commit comments

Comments
 (0)