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).

Author: bors

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;
@@ -1135,7 +1135,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));
     }
 

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::Deferred(StripInner::None),
             rustflags: vec![],
             trim_paths: None,
         }
@@ -873,28 +880,87 @@ 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,
+        }
+    }
+
+    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 {
+    fn eq(&self, other: &Self) -> bool {
+        self.into_inner().eq(&other.into_inner())
+    }
+}
+
+impl Hash for Strip {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.into_inner().hash(state);
+    }
+}
+
+impl PartialOrd for Strip {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        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.
 ///

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,

tests/testsuite/profiles.rs

@@ -607,7 +607,82 @@ 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();
+}
+
+#[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();
 }
 

tests/testsuite/run.rs

@@ -790,13 +790,15 @@ 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 \
         --emit=[..]link \
         -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 [..]

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": [