From 58da1cec19acf4c2f1e0537aed878cbfc27fd842 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 6 Jan 2024 16:49:19 +0100 Subject: [PATCH 1/5] Store the `Strip` option in Deferred form --- src/cargo/core/compiler/mod.rs | 5 ++- src/cargo/core/profiles.rs | 79 +++++++++++++++++++++++++++++----- 2 files changed, 71 insertions(+), 13 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 1ac5a6d525c..6e6955382de 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -88,7 +88,7 @@ use self::unit_graph::UnitDep; use crate::core::compiler::future_incompat::FutureIncompatReport; pub use crate::core::compiler::unit::{Unit, UnitInterner}; use crate::core::manifest::TargetSourcePath; -use crate::core::profiles::{PanicStrategy, Profile, Strip}; +use crate::core::profiles::{PanicStrategy, Profile, StripInner}; use crate::core::{Feature, PackageId, Target, Verbosity}; use crate::util::errors::{CargoResult, VerboseError}; use crate::util::interning::InternedString; @@ -1130,7 +1130,8 @@ fn build_base_args(cx: &Context<'_, '_>, cmd: &mut ProcessBuilder, unit: &Unit) opt(cmd, "-C", "incremental=", Some(dir)); } - if strip != Strip::None { + let strip = strip.into_inner(); + if strip != StripInner::None { cmd.arg("-C").arg(format!("strip={}", strip)); } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 090e2183bcd..7b133c61da2 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -573,10 +573,17 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { profile.trim_paths = Some(trim_paths.clone()); } profile.strip = match toml.strip { - Some(StringOrBool::Bool(true)) => Strip::Named(InternedString::new("symbols")), - None | Some(StringOrBool::Bool(false)) => Strip::None, - Some(StringOrBool::String(ref n)) if n.as_str() == "none" => Strip::None, - Some(StringOrBool::String(ref n)) => Strip::Named(InternedString::new(n)), + Some(StringOrBool::Bool(true)) => { + Strip::Resolved(StripInner::Named(InternedString::new("symbols"))) + } + Some(StringOrBool::Bool(false)) => Strip::Resolved(StripInner::None), + Some(StringOrBool::String(ref n)) if n.as_str() == "none" => { + Strip::Resolved(StripInner::None) + } + Some(StringOrBool::String(ref n)) => { + Strip::Resolved(StripInner::Named(InternedString::new(n))) + } + None => Strip::Deferred(StripInner::None), }; } @@ -636,7 +643,7 @@ impl Default for Profile { rpath: false, incremental: false, panic: PanicStrategy::Unwind, - strip: Strip::None, + strip: Strip::Resolved(StripInner::None), rustflags: vec![], trim_paths: None, } @@ -873,28 +880,78 @@ impl fmt::Display for PanicStrategy { } } -/// The setting for choosing which symbols to strip #[derive( Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize, )] -#[serde(rename_all = "lowercase")] -pub enum Strip { +pub enum StripInner { /// Don't remove any symbols None, /// Named Strip settings Named(InternedString), } -impl fmt::Display for Strip { +impl fmt::Display for StripInner { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match *self { - Strip::None => "none", - Strip::Named(s) => s.as_str(), + StripInner::None => "none", + StripInner::Named(s) => s.as_str(), } .fmt(f) } } +/// The setting for choosing which symbols to strip. +/// +/// This is semantically a [`StripInner`], and should be used as so via the +/// [`Strip::into_inner`] method for all intents and purposes. +/// +/// Internally, it's used to model a strip option whose value can be deferred +/// for optimization purposes: when no package being compiled requires debuginfo, +/// then we can strip debuginfo to remove pre-existing debug symbols from the +/// standard library. +#[derive(Clone, Copy, Debug, Eq, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum Strip { + /// A strip option that is fixed and will not change. + Resolved(StripInner), + /// A strip option that might be overridden by Cargo for optimization + /// purposes. + Deferred(StripInner), +} + +impl Strip { + /// The main way to interact with this strip option, turning it into a [`StripInner`]. + pub fn into_inner(self) -> StripInner { + match self { + Strip::Resolved(v) | Strip::Deferred(v) => v, + } + } +} + +impl PartialEq for Strip { + fn eq(&self, other: &Self) -> bool { + self.into_inner().eq(&other.into_inner()) + } +} + +impl Hash for Strip { + fn hash(&self, state: &mut H) { + self.into_inner().hash(state); + } +} + +impl PartialOrd for Strip { + fn partial_cmp(&self, other: &Self) -> Option { + self.into_inner().partial_cmp(&other.into_inner()) + } +} + +impl Ord for Strip { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.into_inner().cmp(&other.into_inner()) + } +} + /// Flags used in creating `Unit`s to indicate the purpose for the target, and /// to ensure the target's dependencies have the correct settings. /// From bc534513dd1cef1625ff30df733141971570e531 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 6 Jan 2024 17:01:10 +0100 Subject: [PATCH 2/5] Strip debuginfo if no debuginfo was requested --- src/cargo/core/profiles.rs | 11 ++++++++++- src/cargo/ops/cargo_compile/mod.rs | 12 ++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 7b133c61da2..98dea9ece40 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -643,7 +643,7 @@ impl Default for Profile { rpath: false, incremental: false, panic: PanicStrategy::Unwind, - strip: Strip::Resolved(StripInner::None), + strip: Strip::Deferred(StripInner::None), rustflags: vec![], trim_paths: None, } @@ -926,6 +926,15 @@ impl Strip { Strip::Resolved(v) | Strip::Deferred(v) => v, } } + + pub(crate) fn is_deferred(&self) -> bool { + matches!(self, Strip::Deferred(_)) + } + + /// Reset to stripping debuginfo. + pub(crate) fn strip_debuginfo(self) -> Self { + Strip::Resolved(StripInner::Named("debuginfo".into())) + } } impl PartialEq for Strip { diff --git a/src/cargo/ops/cargo_compile/mod.rs b/src/cargo/ops/cargo_compile/mod.rs index 3522ef9d34d..0b939d5589d 100644 --- a/src/cargo/ops/cargo_compile/mod.rs +++ b/src/cargo/ops/cargo_compile/mod.rs @@ -657,6 +657,18 @@ fn traverse_and_share( }; let mut profile = unit.profile.clone(); + if profile.strip.is_deferred() { + // If strip was not manually set, and all dependencies of this unit together + // with this unit have debuginfo turned off, we enable debuginfo stripping. + // This will remove pre-existing debug symbols coming from the standard library. + if !profile.debuginfo.is_turned_on() + && new_deps + .iter() + .all(|dep| !dep.unit.profile.debuginfo.is_turned_on()) + { + profile.strip = profile.strip.strip_debuginfo(); + } + } // If this is a build dependency, and it's not shared with runtime dependencies, we can weaken // its debuginfo level to optimize build times. We do nothing if it's an artifact dependency, From 872aa8f4f776a9088c519aff80fe470a8b64050b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 6 Jan 2024 17:17:51 +0100 Subject: [PATCH 3/5] Fix existing test --- tests/testsuite/profiles.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index 531c0270033..ee582fff78f 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -607,7 +607,7 @@ fn strip_accepts_false_to_disable_strip() { .build(); p.cargo("build --release -v") - .with_stderr_does_not_contain("-C strip") + .with_stderr_does_not_contain("[RUNNING] `rustc [..] -C strip[..]`") .run(); } From af46bbd2646c87206996dbc78653bf032a280069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 6 Jan 2024 17:22:10 +0100 Subject: [PATCH 4/5] Add tests --- tests/testsuite/profiles.rs | 75 +++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index ee582fff78f..20368ff3e8f 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -611,6 +611,81 @@ fn strip_accepts_false_to_disable_strip() { .run(); } +#[cargo_test] +fn strip_debuginfo_in_release() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build --release -v") + .with_stderr_contains("[RUNNING] `rustc [..] -C strip=debuginfo[..]`") + .run(); +} + +#[cargo_test] +fn strip_debuginfo_without_debug() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [profile.dev] + debug = 0 + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + p.cargo("build -v") + .with_stderr_contains("[RUNNING] `rustc [..] -C strip=debuginfo[..]`") + .run(); +} + +#[cargo_test] +fn do_not_strip_debuginfo_with_requested_debug() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = { path = "bar" } + + [profile.release.package.bar] + debug = 1 + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar" + verison = "0.1.0" + "#, + ) + .file("bar/src/lib.rs", "") + .build(); + + p.cargo("build --release -v") + .with_stderr_does_not_contain("[RUNNING] `rustc [..] -C strip=debuginfo[..]`") + .run(); +} + #[cargo_test] fn rustflags_works() { let p = project() From e954099394a61258b62531a67a55b222c11a5e38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 6 Jan 2024 17:36:56 +0100 Subject: [PATCH 5/5] Update existing tests --- tests/testsuite/run.rs | 2 ++ tests/testsuite/unit_graph.rs | 8 ++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/testsuite/run.rs b/tests/testsuite/run.rs index 4537b2caa16..8d983b2e02c 100644 --- a/tests/testsuite/run.rs +++ b/tests/testsuite/run.rs @@ -790,6 +790,7 @@ fn example_with_release_flag() { -C opt-level=3[..]\ -C metadata=[..] \ --out-dir [CWD]/target/release/deps \ + -C strip=debuginfo \ -L dependency=[CWD]/target/release/deps` [COMPILING] foo v0.0.1 ([CWD]) [RUNNING] `rustc --crate-name a examples/a.rs [..]--crate-type bin \ @@ -797,6 +798,7 @@ fn example_with_release_flag() { -C opt-level=3[..]\ -C metadata=[..] \ --out-dir [CWD]/target/release/examples \ + -C strip=debuginfo \ -L dependency=[CWD]/target/release/deps \ --extern bar=[CWD]/target/release/deps/libbar-[..].rlib` [FINISHED] release [optimized] target(s) in [..] diff --git a/tests/testsuite/unit_graph.rs b/tests/testsuite/unit_graph.rs index 91451177a51..2b62262da19 100644 --- a/tests/testsuite/unit_graph.rs +++ b/tests/testsuite/unit_graph.rs @@ -81,7 +81,7 @@ fn simple() { "panic": "unwind", "rpath": false, "split_debuginfo": "{...}", - "strip": "none" + "strip": "{...}" }, "target": { "crate_types": [ @@ -126,7 +126,7 @@ fn simple() { "panic": "unwind", "rpath": false, "split_debuginfo": "{...}", - "strip": "none" + "strip": "{...}" }, "target": { "crate_types": [ @@ -164,7 +164,7 @@ fn simple() { "panic": "unwind", "rpath": false, "split_debuginfo": "{...}", - "strip": "none" + "strip": "{...}" }, "target": { "crate_types": [ @@ -207,7 +207,7 @@ fn simple() { "panic": "unwind", "rpath": false, "split_debuginfo": "{...}", - "strip": "none" + "strip": "{...}" }, "target": { "crate_types": [