Skip to content

Commit

Permalink
Don't distinguish Debuginfo::None and Debuginfo::Explicit(None)
Browse files Browse the repository at this point in the history
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() {
```
  • Loading branch information
jyn514 committed May 30, 2023
1 parent f7b95e3 commit e2eacc6
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 58 deletions.
10 changes: 6 additions & 4 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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(),
Expand Down Expand Up @@ -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));
}

Expand Down
54 changes: 22 additions & 32 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -720,11 +718,8 @@ impl Profile {

/// The debuginfo level setting.
///
/// This is semantically an `Option<u32>`, 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<u32>::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
Expand All @@ -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<TomlDebugInfo> {
/// 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 {
Expand All @@ -774,44 +768,40 @@ 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())
}
}

impl Eq for DebugInfo {}

impl Hash for DebugInfo {
fn hash<H: std::hash::Hasher>(&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<std::cmp::Ordering> {
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())
}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 6 additions & 5 deletions tests/testsuite/profile_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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

Expand All @@ -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
}
Expand Down Expand Up @@ -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();
}
19 changes: 11 additions & 8 deletions tests/testsuite/profile_targets.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,19 @@ 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 [..]
[RUNNING] `[..] rustc --crate-name foo src/main.rs [..]--crate-type bin --emit=[..]link -C panic=abort[..]-C codegen-units=1 -C debuginfo=2 [..]
[FINISHED] dev [unoptimized + debuginfo] [..]
"
)
.with_stdout_does_not_contain("[..] -C debuginfo=0[..]")
.run();
p.cargo("build -vv")
.with_stderr_unordered(
Expand Down Expand Up @@ -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
Expand All @@ -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 [..]`
Expand All @@ -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(
Expand Down
23 changes: 15 additions & 8 deletions tests/testsuite/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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",
Expand All @@ -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();
}

0 comments on commit e2eacc6

Please sign in to comment.