From e2eacc64148107b817ba21d608403e0175a81cd0 Mon Sep 17 00:00:00 2001 From: jyn Date: Tue, 30 May 2023 16:47:16 -0500 Subject: [PATCH] Don't distinguish `Debuginfo::None` and `Debuginfo::Explicit(None)` Previously, `Debuginfo::None` meant "don't pass -C debuginfo" and `Explicit(None)` meant "-C debuginfo=0", which occasionally led to caching bugs where cargo would sometimes pass `-C debuginfo=0` and sometimes not. There are no such bugs currently that we know of, but representing them the same within cargo avoids the possibility of the bug popping up again in the future. I tested the `with_stderr_does_not_contain_tests` with this diff to ensure they did not pass: ```diff diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 55ec17182..c186dd00a 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -1073,9 +1073,7 @@ fn build_base_args( let debuginfo = debuginfo.into_inner(); // Shorten the number of arguments if possible. - if debuginfo != TomlDebugInfo::None { cmd.arg("-C").arg(format!("debuginfo={}", debuginfo)); - } cmd.args(unit.pkg.manifest().lint_rustflags()); if !rustflags.is_empty() { ``` --- src/cargo/core/compiler/mod.rs | 10 +++--- src/cargo/core/profiles.rs | 54 ++++++++++++------------------ tests/testsuite/build.rs | 2 +- tests/testsuite/profile_config.rs | 11 +++--- tests/testsuite/profile_targets.rs | 19 ++++++----- tests/testsuite/profiles.rs | 23 ++++++++----- 6 files changed, 61 insertions(+), 58 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 7e49f0079d3..55ec17182d1 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -604,7 +604,7 @@ fn link_targets(cx: &mut Context<'_, '_>, unit: &Unit, fresh: bool) -> CargoResu } if json_messages { - let debuginfo = profile.debuginfo.to_option().map(|d| match d { + let debuginfo = match profile.debuginfo.into_inner() { TomlDebugInfo::None => machine_message::ArtifactDebuginfo::Int(0), TomlDebugInfo::Limited => machine_message::ArtifactDebuginfo::Int(1), TomlDebugInfo::Full => machine_message::ArtifactDebuginfo::Int(2), @@ -614,10 +614,10 @@ fn link_targets(cx: &mut Context<'_, '_>, unit: &Unit, fresh: bool) -> CargoResu TomlDebugInfo::LineTablesOnly => { machine_message::ArtifactDebuginfo::Named("line-tables-only") } - }); + }; let art_profile = machine_message::ArtifactProfile { opt_level: profile.opt_level.as_str(), - debuginfo, + debuginfo: Some(debuginfo), debug_assertions: profile.debug_assertions, overflow_checks: profile.overflow_checks, test: unit_mode.is_any_test(), @@ -1071,7 +1071,9 @@ fn build_base_args( cmd.arg("-C").arg(&format!("codegen-units={}", n)); } - if let Some(debuginfo) = debuginfo.to_option() { + let debuginfo = debuginfo.into_inner(); + // Shorten the number of arguments if possible. + if debuginfo != TomlDebugInfo::None { cmd.arg("-C").arg(format!("debuginfo={}", debuginfo)); } diff --git a/src/cargo/core/profiles.rs b/src/cargo/core/profiles.rs index 3831f18c2ea..963dbd1fd17 100644 --- a/src/cargo/core/profiles.rs +++ b/src/cargo/core/profiles.rs @@ -449,9 +449,7 @@ impl ProfileMaker { // a unit is shared. If that's the case, we'll use the deferred value // below so the unit can be reused, otherwise we can avoid emitting // the unit's debuginfo. - if let Some(debuginfo) = profile.debuginfo.to_option() { - profile.debuginfo = DebugInfo::Deferred(debuginfo); - } + profile.debuginfo = DebugInfo::Deferred(profile.debuginfo.into_inner()); } // ... and next comes any other sorts of overrides specified in // profiles, such as `[profile.release.build-override]` or @@ -529,7 +527,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) { profile.codegen_units = toml.codegen_units; } if let Some(debuginfo) = toml.debug { - profile.debuginfo = DebugInfo::Explicit(debuginfo); + profile.debuginfo = DebugInfo::Resolved(debuginfo); } if let Some(debug_assertions) = toml.debug_assertions { profile.debug_assertions = debug_assertions; @@ -611,7 +609,7 @@ impl Default for Profile { lto: Lto::Bool(false), codegen_backend: None, codegen_units: None, - debuginfo: DebugInfo::None, + debuginfo: DebugInfo::Resolved(TomlDebugInfo::None), debug_assertions: false, split_debuginfo: None, overflow_checks: false, @@ -680,7 +678,7 @@ impl Profile { Profile { name: InternedString::new("dev"), root: ProfileRoot::Debug, - debuginfo: DebugInfo::Explicit(TomlDebugInfo::Full), + debuginfo: DebugInfo::Resolved(TomlDebugInfo::Full), debug_assertions: true, overflow_checks: true, incremental: true, @@ -720,11 +718,8 @@ impl Profile { /// The debuginfo level setting. /// -/// This is semantically an `Option`, and should be used as so via the -/// [DebugInfo::to_option] method for all intents and purposes: -/// - `DebugInfo::None` corresponds to `None` -/// - `DebugInfo::Explicit(u32)` and `DebugInfo::Deferred` correspond to -/// `Option::Some` +/// This is semantically a [`TomlDebugInfo`], and should be used as so via the +/// [DebugInfo::into_inner] method for all intents and purposes. /// /// Internally, it's used to model a debuginfo level whose value can be deferred /// for optimization purposes: host dependencies usually don't need the same @@ -736,35 +731,34 @@ impl Profile { #[derive(Debug, Copy, Clone, serde::Serialize)] #[serde(untagged)] pub enum DebugInfo { - /// No debuginfo level was set. - None, - /// A debuginfo level that is explicitly set, by a profile or a user. - Explicit(TomlDebugInfo), + /// A debuginfo level that is fixed and will not change. + /// + /// This can be set by a profile, user, or default value. + Resolved(TomlDebugInfo), /// For internal purposes: a deferred debuginfo level that can be optimized /// away, but has this value otherwise. /// - /// Behaves like `Explicit` in all situations except for the default build + /// Behaves like `Resolved` in all situations except for the default build /// dependencies profile: whenever a build dependency is not shared with /// runtime dependencies, this level is weakened to a lower level that is - /// faster to build (see [DebugInfo::weaken]). + /// faster to build (see [`DebugInfo::weaken`]). /// /// In all other situations, this level value will be the one to use. Deferred(TomlDebugInfo), } impl DebugInfo { - /// The main way to interact with this debuginfo level, turning it into an Option. - pub fn to_option(self) -> Option { + /// The main way to interact with this debuginfo level, turning it into a [`TomlDebugInfo`]. + pub fn into_inner(self) -> TomlDebugInfo { match self { - DebugInfo::None => None, - DebugInfo::Explicit(v) | DebugInfo::Deferred(v) => Some(v), + DebugInfo::Resolved(v) | DebugInfo::Deferred(v) => v, } } /// Returns true if any debuginfo will be generated. Helper /// for a common operation on the usual `Option` representation. pub(crate) fn is_turned_on(&self) -> bool { - !matches!(self.to_option(), None | Some(TomlDebugInfo::None)) + !matches!(self.into_inner(), TomlDebugInfo::None) } pub(crate) fn is_deferred(&self) -> bool { @@ -774,24 +768,20 @@ impl DebugInfo { /// Force the deferred, preferred, debuginfo level to a finalized explicit value. pub(crate) fn finalize(self) -> Self { match self { - DebugInfo::Deferred(v) => DebugInfo::Explicit(v), + DebugInfo::Deferred(v) => DebugInfo::Resolved(v), _ => self, } } /// Reset to the lowest level: no debuginfo. - /// If it is explicitly set, keep it explicit. pub(crate) fn weaken(self) -> Self { - match self { - DebugInfo::None => DebugInfo::None, - _ => DebugInfo::Explicit(TomlDebugInfo::None), - } + DebugInfo::Resolved(TomlDebugInfo::None) } } impl PartialEq for DebugInfo { fn eq(&self, other: &DebugInfo) -> bool { - self.to_option().eq(&other.to_option()) + self.into_inner().eq(&other.into_inner()) } } @@ -799,19 +789,19 @@ impl Eq for DebugInfo {} impl Hash for DebugInfo { fn hash(&self, state: &mut H) { - self.to_option().hash(state); + self.into_inner().hash(state); } } impl PartialOrd for DebugInfo { fn partial_cmp(&self, other: &Self) -> Option { - self.to_option().partial_cmp(&other.to_option()) + self.into_inner().partial_cmp(&other.into_inner()) } } impl Ord for DebugInfo { fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.to_option().cmp(&other.to_option()) + self.into_inner().cmp(&other.into_inner()) } } diff --git a/tests/testsuite/build.rs b/tests/testsuite/build.rs index 7b555a71b20..44a452fd81f 100644 --- a/tests/testsuite/build.rs +++ b/tests/testsuite/build.rs @@ -4074,7 +4074,7 @@ fn message_format_json_forward_stderr() { }, "profile":{ "debug_assertions":false, - "debuginfo":null, + "debuginfo":0, "opt_level":"3", "overflow_checks": false, "test":false diff --git a/tests/testsuite/profile_config.rs b/tests/testsuite/profile_config.rs index cf9807964e2..ee087d516f7 100644 --- a/tests/testsuite/profile_config.rs +++ b/tests/testsuite/profile_config.rs @@ -437,7 +437,7 @@ fn named_config_profile() { assert_eq!(p.name, "foo"); assert_eq!(p.codegen_units, Some(2)); // "foo" from config assert_eq!(p.opt_level, "1"); // "middle" from manifest - assert_eq!(p.debuginfo.to_option(), Some(TomlDebugInfo::Limited)); // "bar" from config + assert_eq!(p.debuginfo.into_inner(), TomlDebugInfo::Limited); // "bar" from config assert_eq!(p.debug_assertions, true); // "dev" built-in (ignore build-override) assert_eq!(p.overflow_checks, true); // "dev" built-in (ignore package override) @@ -446,7 +446,7 @@ fn named_config_profile() { assert_eq!(bo.name, "foo"); assert_eq!(bo.codegen_units, Some(6)); // "foo" build override from config assert_eq!(bo.opt_level, "0"); // default to zero - assert_eq!(bo.debuginfo.to_option(), Some(TomlDebugInfo::Limited)); // SAME as normal + assert_eq!(bo.debuginfo.into_inner(), TomlDebugInfo::Limited); // SAME as normal assert_eq!(bo.debug_assertions, false); // "foo" build override from manifest assert_eq!(bo.overflow_checks, true); // SAME as normal @@ -455,7 +455,7 @@ fn named_config_profile() { assert_eq!(po.name, "foo"); assert_eq!(po.codegen_units, Some(7)); // "foo" package override from config assert_eq!(po.opt_level, "1"); // SAME as normal - assert_eq!(po.debuginfo.to_option(), Some(TomlDebugInfo::Limited)); // SAME as normal + assert_eq!(po.debuginfo.into_inner(), TomlDebugInfo::Limited); // SAME as normal assert_eq!(po.debug_assertions, true); // SAME as normal assert_eq!(po.overflow_checks, false); // "middle" package override from manifest } @@ -509,12 +509,13 @@ fn test_with_dev_profile() { [DOWNLOADING] [..] [DOWNLOADED] [..] [COMPILING] somedep v1.0.0 -[RUNNING] `rustc --crate-name somedep [..]-C debuginfo=0[..] +[RUNNING] `rustc --crate-name somedep [..] [COMPILING] foo v0.1.0 [..] -[RUNNING] `rustc --crate-name foo [..]-C debuginfo=0[..] +[RUNNING] `rustc --crate-name foo [..] [FINISHED] [..] [EXECUTABLE] `[..]/target/debug/deps/foo-[..][EXE]` ", ) + .with_stdout_does_not_contain("[..] -C debuginfo=0[..]") .run(); } diff --git a/tests/testsuite/profile_targets.rs b/tests/testsuite/profile_targets.rs index 0449e8ab37d..0f0e51e7af8 100644 --- a/tests/testsuite/profile_targets.rs +++ b/tests/testsuite/profile_targets.rs @@ -88,11 +88,11 @@ fn profile_selection_build() { .with_stderr_unordered("\ [COMPILING] bar [..] [RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C codegen-units=1 -C debuginfo=2 [..] -[RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 [..] [COMPILING] bdep [..] -[RUNNING] `[..] rustc --crate-name bdep bdep/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name bdep bdep/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 [..] [COMPILING] foo [..] -[RUNNING] `[..] rustc --crate-name build_script_build build.rs [..]--crate-type bin --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name build_script_build build.rs [..]--crate-type bin --emit=[..]link[..]-C codegen-units=5 [..] [RUNNING] `[..]/target/debug/build/foo-[..]/build-script-build` [foo 0.0.1] foo custom build PROFILE=debug DEBUG=true OPT_LEVEL=0 [RUNNING] `[..] rustc --crate-name foo src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C codegen-units=1 -C debuginfo=2 [..] @@ -100,6 +100,7 @@ fn profile_selection_build() { [FINISHED] dev [unoptimized + debuginfo] [..] " ) + .with_stdout_does_not_contain("[..] -C debuginfo=0[..]") .run(); p.cargo("build -vv") .with_stderr_unordered( @@ -154,8 +155,9 @@ fn profile_selection_build_all_targets() { // `build.rs` is a plugin. // - Benchmark dependencies are compiled in `dev` mode, which may be // surprising. See issue rust-lang/cargo#4929. - // - We make sure that the build dependencies bar, bdep, and build.rs - // are built with debuginfo=0. + // - We make sure that the build dependencies bar, bdep, and build.rs are built with + // debuginfo=0; but since we don't pass `-C debuginfo` when it's set to 0, we have to test + // explicitly that there's no `-C debuginfo` flag. // // - Dependency profiles: // Pkg Target Profile Reason @@ -181,11 +183,11 @@ fn profile_selection_build_all_targets() { [COMPILING] bar [..] [RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=1 -C debuginfo=2 [..] [RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C codegen-units=1 -C debuginfo=2 [..] -[RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name bar bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 [..] [COMPILING] bdep [..] -[RUNNING] `[..] rustc --crate-name bdep bdep/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name bdep bdep/src/lib.rs [..]--crate-type lib --emit=[..]link[..]-C codegen-units=5 [..] [COMPILING] foo [..] -[RUNNING] `[..] rustc --crate-name build_script_build build.rs [..]--crate-type bin --emit=[..]link[..]-C codegen-units=5 -C debuginfo=0[..] +[RUNNING] `[..] rustc --crate-name build_script_build build.rs [..]--crate-type bin --emit=[..]link[..]-C codegen-units=5 [..] [RUNNING] `[..]/target/debug/build/foo-[..]/build-script-build` [foo 0.0.1] foo custom build PROFILE=debug DEBUG=true OPT_LEVEL=0 [RUNNING] `[..] rustc --crate-name foo src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C codegen-units=1 -C debuginfo=2 [..]` @@ -199,6 +201,7 @@ fn profile_selection_build_all_targets() { [FINISHED] dev [unoptimized + debuginfo] [..] " ) + .with_stdout_does_not_contain("[..] -C debuginfo=0[..]") .run(); p.cargo("build -vv") .with_stderr_unordered( diff --git a/tests/testsuite/profiles.rs b/tests/testsuite/profiles.rs index 2404b65d1f9..ca72d9d0fea 100644 --- a/tests/testsuite/profiles.rs +++ b/tests/testsuite/profiles.rs @@ -467,10 +467,11 @@ fn debug_0_report() { .with_stderr( "\ [COMPILING] foo v0.1.0 [..] -[RUNNING] `rustc --crate-name foo src/lib.rs [..]-C debuginfo=0 [..] +[RUNNING] `rustc --crate-name foo src/lib.rs [..] [FINISHED] dev [unoptimized] target(s) in [..] ", ) + .with_stderr_does_not_contain("-C debuginfo") .run(); } @@ -745,13 +746,7 @@ Caused by: #[cargo_test(nightly, reason = "debug options stabilized in 1.70")] fn debug_options_valid() { - for (option, cli) in [ - ("line-directives-only", "line-directives-only"), - ("line-tables-only", "line-tables-only"), - ("none", "0"), - ("limited", "1"), - ("full", "2"), - ] { + let build = |option| { let p = project() .file( "Cargo.toml", @@ -771,7 +766,19 @@ fn debug_options_valid() { .build(); p.cargo("build -v") + }; + + for (option, cli) in [ + ("line-directives-only", "line-directives-only"), + ("line-tables-only", "line-tables-only"), + ("limited", "1"), + ("full", "2"), + ] { + build(option) .with_stderr_contains(&format!("[RUNNING] `rustc [..]-C debuginfo={cli} [..]")) .run(); } + build("none") + .with_stderr_does_not_contain("[..]-C debuginfo[..]") + .run(); }