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

Strip debuginfo when debuginfo is not requested #13257

Merged
merged 5 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}

Expand Down
88 changes: 77 additions & 11 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
};
}

Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would make sense to introduce some kind of generic Resolved/Deferred wrapper and use it both here and in DebugInfo?

/// 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.
///
Expand Down
12 changes: 12 additions & 0 deletions src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
77 changes: 76 additions & 1 deletion tests/testsuite/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Kobzol marked this conversation as resolved.
Show resolved Hide resolved
"#,
)
.file("bar/src/lib.rs", "")
.build();

p.cargo("build --release -v")
.with_stderr_does_not_contain("[RUNNING] `rustc [..] -C strip=debuginfo[..]`")
.run();
}

Expand Down
2 changes: 2 additions & 0 deletions tests/testsuite/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 [..]
Expand Down
8 changes: 4 additions & 4 deletions tests/testsuite/unit_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ fn simple() {
"panic": "unwind",
"rpath": false,
"split_debuginfo": "{...}",
"strip": "none"
"strip": "{...}"
},
"target": {
"crate_types": [
Expand Down Expand Up @@ -126,7 +126,7 @@ fn simple() {
"panic": "unwind",
"rpath": false,
"split_debuginfo": "{...}",
"strip": "none"
"strip": "{...}"
},
"target": {
"crate_types": [
Expand Down Expand Up @@ -164,7 +164,7 @@ fn simple() {
"panic": "unwind",
"rpath": false,
"split_debuginfo": "{...}",
"strip": "none"
"strip": "{...}"
},
"target": {
"crate_types": [
Expand Down Expand Up @@ -207,7 +207,7 @@ fn simple() {
"panic": "unwind",
"rpath": false,
"split_debuginfo": "{...}",
"strip": "none"
"strip": "{...}"
},
"target": {
"crate_types": [
Expand Down