Don't distinguish Debuginfo::None and Debuginfo::Explicit(None)

[jyn514]

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 --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() {

What does this PR try to resolve?

This reduces the likelihood of a latent caching bug. See #12163 (comment) for details.

How should we test and review this PR?

Run cargo test. To verify the with_stderr_does_not_contain tests are actually doing something, apply the diff in the description above and rerun cargo test. You should see 5 failures in the profiles and profile_targets modules.

Additional information

The only subtle bit of logic this changes is

cargo/src/cargo/core/profiles.rs

Lines 442 to 452 in c35fdbf

	// For build dependencies, we usually don't need debuginfo, and
	// removing it will compile faster. However, that can conflict with
	// a unit graph optimization, reusing units that are shared between
	// build dependencies and runtime dependencies: when the runtime
	// target is the same as the build host, we only need to build a
	// dependency once and reuse the results, instead of building twice.
	// We defer the choice of the debuginfo level until we can check if
	// 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.
	profile.debuginfo = DebugInfo::Deferred(profile.debuginfo.into_inner());

I think this is the same behavior as before: the only time profile.debuginfo.to_option() would be None is when it was set to Debuginfo::None. In that case today, we'll now set it to Deferred(None). The only time that can be modified is in

cargo/src/cargo/ops/cargo_compile/mod.rs

Lines 661 to 698 in ae252b5

	// 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,
	// as it and its debuginfo may end up embedded in the main program.
	if unit_is_for_host
	&& to_host.is_some()
	&& profile.debuginfo.is_deferred()
	&& !unit.artifact.is_true()
	{
	// We create a "probe" test to see if a unit with the same explicit debuginfo level exists
	// in the graph. This is the level we'd expect if it was set manually or the default value
	// set by a profile for a runtime dependency: its canonical value.
	let canonical_debuginfo = profile.debuginfo.finalize();
	let mut canonical_profile = profile.clone();
	canonical_profile.debuginfo = canonical_debuginfo;
	let unit_probe = interner.intern(
	&unit.pkg,
	&unit.target,
	canonical_profile,
	to_host.unwrap(),
	unit.mode,
	unit.features.clone(),
	unit.is_std,
	unit.dep_hash,
	unit.artifact,
	unit.artifact_target_for_features,
	);
	// We can now turn the deferred value into its actual final value.
	profile.debuginfo = if unit_graph.contains_key(&unit_probe) {
	// The unit is present in both build time and runtime subgraphs: we canonicalize its
	// level to the other unit's, thus ensuring reuse between the two to optimize build times.
	canonical_debuginfo
	} else {
	// The unit is only present in the build time subgraph, we can weaken its debuginfo
	// level to optimize build times.
	canonical_debuginfo.weaken()
	}
	}

where it will be canonicalized to Resolved(None) regardless of whether the unit graph has a matching unit in the runtime graph or not.

I think there are existing tests for this behavior but I'm not sure.

r? @weihanglo

[weihanglo]

Beautiful!!! Thank you!
Only one minor doc comment issue left.

BTW I found split-debuginfo is also redundant when debuginfo != TomlDebugInfo::None. Just FYI. No need to change.

cargo/src/cargo/core/compiler/mod.rs

Lines 1055 to 1064 in e2eacc6

	if let Some(split) = split_debuginfo {
	if cx
	.bcx
	.target_data
	.info(unit.kind)
	.supports_debuginfo_split(split)
	{
	cmd.arg("-C").arg(format!("split-debuginfo={}", split));
	}
	}

[Mark-Simulacrum]

Do we test for / does this break setting debug info via RUSTFLAGS? That use case is probably not well supported, but it's sometimes more convenient than alternatives (e.g., because of target-dependent debuginfo which I'm not sure is otherwise file configuration placeable).

[jyn514]

I would be extremely surprised if that worked before this PR; the reason I made #11958 originally is I couldn't figure out how to do it through RUSTFLAGS in bootstrap.

That said, this passes -C debuginfo strictly less than before, so I wouldn't expect it to hurt the situation.

[weihanglo]

Do we test for / does this break setting debug info via RUSTFLAGS?

Thanks for calling out! RUSTFLAGS, build.rustflags and target.<triple>.rustflags should take precedence over profile settings. I don't think this break anything. Profile setting should have the lowest priority.

profile.debuginfo:

cargo/src/cargo/core/compiler/mod.rs

Line 1075 in c9b89c6

cmd.arg("-C").arg(format!("debuginfo={}", debuginfo));

RUStFLAGS:

cargo/src/cargo/core/compiler/mod.rs

Line 322 in c9b89c6

rustc.args(cx.bcx.rustflags_args(unit));

[weihanglo]

Thanks!

@bors r+

[bors]

📌 Commit f74c8df has been approved by weihanglo

It is now in the queue for this repository.

[bors]

⌛ Testing commit f74c8df with merge c0f36b8...

[bors]

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing c0f36b8 to master...