Auto merge of #11252 - lqd:deferred-debuginfo, r=ehuss

Turn off debuginfo for build dependencies v2

This PR is an alternative to #10493, fixing its most important issue: the unit graph optimization to reuse already built artifacts for dependencies shared between the build-time and runtime subgraphs is preserved (most of the time).

By deferring the default debuginfo level in `dev.build-override` until unit graph sharing, we check whether re-use would happen. If so, we use the debuginfo level to ensure reuse does happen. Otherwise, we can avoid emitting debuginfo and improve compile times (on clean, debug and check builds -- although reuse only happens on debug builds).

I've kept the message explaining how to bump the debuginfo level if an error occurs while running a build script (and backtraces were requested) that was in the previous PR.

I've ran benchmarks on 800 crates, at different parallelism levels, and published the (surprisingly good) results with visualizations, summaries, and raw data [here](https://github.com/lqd/rustc-benchmarking-data/tree/main/experiments/cargo-build-defaults).

Opening this PR as discussed in [Eric's message](https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Defaults.20for.20faster.20compilation.20of.20.22for.20host.22.20units/near/304236576l) as draft since 2 tests won't pass. That fixes the `cargo-crev` case we saw as a blocker last time, but doesn't fix all such cases of reuse, like the 2 failing tests:

- [`optional_build_dep_and_required_normal_dep`](https://github.com/rust-lang/cargo/blob/642a0e625d10099a0ca289827de85499d073c572/tests/testsuite/build_script.rs#L4449)
- and [`proc_macro_ws`](https://github.com/rust-lang/cargo/blob/bd5db301b0c45ae540afcb19e030dd7c29d2ea4f/tests/testsuite/features2.rs#L1051)

These failures make sense, since the debuginfo optimization is done during traversal, it depends on the contents of the unit graph. These tests ensure that sharing happens even on finer-grained invocations: respectively, with an optional shared dependency that is later enabled, and building shared dependencies by themselves first and later as part of a complete workspace build.

In both these situations, there is no unit that is shared in the graph during the first invocation, so we do the optimization and remove the debuginfo. When the graph changes in the following invocation, sharing is present and we have to build the shared units again with debuginfo.

These cases feel rarer than `cargo-crev`'s case, but I do wonder if there's a way to fix them, or if it's acceptable to not support them.

Author: bors

src/cargo/core/compiler/custom_build.rs

@@ -187,7 +187,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
     // carried over.
     let to_exec = to_exec.into_os_string();
     let mut cmd = cx.compilation.host_process(to_exec, &unit.pkg)?;
-    let debug = unit.profile.debuginfo.unwrap_or(0) != 0;
+    let debug = unit.profile.debuginfo.is_turned_on();
     cmd.env("OUT_DIR", &script_out_dir)
         .env("CARGO_MANIFEST_DIR", unit.pkg.root())
         .env("NUM_JOBS", &bcx.jobs().to_string())
@@ -332,6 +332,15 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
     // Need a separate copy for the fresh closure.
     let targets_fresh = targets.clone();
 
+    let env_profile_name = unit.profile.name.to_uppercase();
+    let built_with_debuginfo = cx
+        .bcx
+        .unit_graph
+        .get(unit)
+        .and_then(|deps| deps.iter().find(|dep| dep.unit.target == unit.target))
+        .map(|dep| dep.unit.profile.debuginfo.is_turned_on())
+        .unwrap_or(false);
+
     // Prepare the unit of "dirty work" which will actually run the custom build
     // command.
     //
@@ -405,7 +414,26 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
                 },
                 true,
             )
-            .with_context(|| format!("failed to run custom build command for `{}`", pkg_descr));
+            .with_context(|| {
+                let mut build_error_context =
+                    format!("failed to run custom build command for `{}`", pkg_descr);
+
+                // If we're opting into backtraces, mention that build dependencies' backtraces can
+                // be improved by requesting debuginfo to be built, if we're not building with
+                // debuginfo already.
+                if let Ok(show_backtraces) = std::env::var("RUST_BACKTRACE") {
+                    if !built_with_debuginfo && show_backtraces != "0" {
+                        build_error_context.push_str(&format!(
+                            "\n\
+                            note: To improve backtraces for build dependencies, set the \
+                            CARGO_PROFILE_{env_profile_name}_BUILD_OVERRIDE_DEBUG=true environment \
+                            variable to enable debug information generation.",
+                        ));
+                    }
+                }
+
+                build_error_context
+            });
 
         if let Err(error) = output {
             insert_warnings_in_build_outputs(

src/cargo/core/compiler/job_queue.rs

@@ -985,7 +985,7 @@ impl<'cfg> DrainState<'cfg> {
         } else {
             "optimized"
         });
-        if profile.debuginfo.unwrap_or(0) != 0 {
+        if profile.debuginfo.is_turned_on() {
             opt_type += " + debuginfo";
         }
 

src/cargo/core/compiler/mod.rs

@@ -550,7 +550,7 @@ fn link_targets(cx: &mut Context<'_, '_>, unit: &Unit, fresh: bool) -> CargoResu
         if json_messages {
             let art_profile = machine_message::ArtifactProfile {
                 opt_level: profile.opt_level.as_str(),
-                debuginfo: profile.debuginfo,
+                debuginfo: profile.debuginfo.to_option(),
                 debug_assertions: profile.debug_assertions,
                 overflow_checks: profile.overflow_checks,
                 test: unit_mode.is_any_test(),
@@ -1003,7 +1003,7 @@ fn build_base_args(
         cmd.arg("-C").arg(&format!("codegen-units={}", n));
     }
 
-    if let Some(debuginfo) = debuginfo {
+    if let Some(debuginfo) = debuginfo.to_option() {
         cmd.arg("-C").arg(format!("debuginfo={}", debuginfo));
     }
 

src/cargo/core/profiles.rs

@@ -276,7 +276,7 @@ impl Profiles {
         // platform which has a stable `-Csplit-debuginfo` option for rustc,
         // and it's typically much faster than running `dsymutil` on all builds
         // in incremental cases.
-        if let Some(debug) = profile.debuginfo {
+        if let Some(debug) = profile.debuginfo.to_option() {
             if profile.split_debuginfo.is_none() && debug > 0 {
                 let target = match &kind {
                     CompileKind::Host => self.rustc_host.as_str(),
@@ -438,6 +438,20 @@ impl ProfileMaker {
             // well as enabling parallelism by not constraining codegen units.
             profile.opt_level = InternedString::new("0");
             profile.codegen_units = None;
+
+            // 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.
+            if let Some(debuginfo) = profile.debuginfo.to_option() {
+                profile.debuginfo = DebugInfo::Deferred(debuginfo);
+            }
         }
         // ... and next comes any other sorts of overrides specified in
         // profiles, such as `[profile.release.build-override]` or
@@ -515,9 +529,9 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
         profile.codegen_units = toml.codegen_units;
     }
     match toml.debug {
-        Some(U32OrBool::U32(debug)) => profile.debuginfo = Some(debug),
-        Some(U32OrBool::Bool(true)) => profile.debuginfo = Some(2),
-        Some(U32OrBool::Bool(false)) => profile.debuginfo = None,
+        Some(U32OrBool::U32(debug)) => profile.debuginfo = DebugInfo::Explicit(debug),
+        Some(U32OrBool::Bool(true)) => profile.debuginfo = DebugInfo::Explicit(2),
+        Some(U32OrBool::Bool(false)) => profile.debuginfo = DebugInfo::None,
         None => {}
     }
     if let Some(debug_assertions) = toml.debug_assertions {
@@ -578,7 +592,7 @@ pub struct Profile {
     pub codegen_backend: Option<InternedString>,
     // `None` means use rustc default.
     pub codegen_units: Option<u32>,
-    pub debuginfo: Option<u32>,
+    pub debuginfo: DebugInfo,
     pub split_debuginfo: Option<InternedString>,
     pub debug_assertions: bool,
     pub overflow_checks: bool,
@@ -600,7 +614,7 @@ impl Default for Profile {
             lto: Lto::Bool(false),
             codegen_backend: None,
             codegen_units: None,
-            debuginfo: None,
+            debuginfo: DebugInfo::None,
             debug_assertions: false,
             split_debuginfo: None,
             overflow_checks: false,
@@ -669,7 +683,7 @@ impl Profile {
         Profile {
             name: InternedString::new("dev"),
             root: ProfileRoot::Debug,
-            debuginfo: Some(2),
+            debuginfo: DebugInfo::Explicit(2),
             debug_assertions: true,
             overflow_checks: true,
             incremental: true,
@@ -708,6 +722,99 @@ 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`
+///
+/// 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
+/// level as target dependencies. For dependencies that are shared between the
+/// two however, that value also affects reuse: different debuginfo levels would
+/// cause to build a unit twice. By deferring the choice until we know
+/// whether to choose the optimized value or the default value, we can make sure
+/// the unit is only built once and the unit graph is still optimized.
+#[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(u32),
+    /// 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
+    /// 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]).
+    ///
+    /// In all other situations, this level value will be the one to use.
+    Deferred(u32),
+}
+
+impl DebugInfo {
+    /// The main way to interact with this debuginfo level, turning it into an Option.
+    pub fn to_option(&self) -> Option<u32> {
+        match self {
+            DebugInfo::None => None,
+            DebugInfo::Explicit(v) | DebugInfo::Deferred(v) => Some(*v),
+        }
+    }
+
+    /// Returns true if the debuginfo level is high enough (at least 1). Helper
+    /// for a common operation on the usual `Option` representation.
+    pub(crate) fn is_turned_on(&self) -> bool {
+        self.to_option().unwrap_or(0) != 0
+    }
+
+    pub(crate) fn is_deferred(&self) -> bool {
+        matches!(self, DebugInfo::Deferred(_))
+    }
+
+    /// 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),
+            _ => self,
+        }
+    }
+
+    /// Reset to the lowest level: no debuginfo.
+    pub(crate) fn weaken(self) -> Self {
+        DebugInfo::None
+    }
+}
+
+impl PartialEq for DebugInfo {
+    fn eq(&self, other: &DebugInfo) -> bool {
+        self.to_option().eq(&other.to_option())
+    }
+}
+
+impl Eq for DebugInfo {}
+
+impl Hash for DebugInfo {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.to_option().hash(state);
+    }
+}
+
+impl PartialOrd for DebugInfo {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        self.to_option().partial_cmp(&other.to_option())
+    }
+}
+
+impl Ord for DebugInfo {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.to_option().cmp(&other.to_option())
+    }
+}
+
 /// The link-time-optimization setting.
 #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash, PartialOrd, Ord)]
 pub enum Lto {

src/cargo/ops/cargo_compile/mod.rs

@@ -428,17 +428,13 @@ pub fn create_bcx<'a, 'cfg>(
         // Rebuild the unit graph, replacing the explicit host targets with
         // CompileKind::Host, removing `artifact_target_for_features` and merging any dependencies
         // shared with build and artifact dependencies.
-        let new_graph = rebuild_unit_graph_shared(
+        (units, scrape_units, unit_graph) = rebuild_unit_graph_shared(
             interner,
             unit_graph,
             &units,
             &scrape_units,
             host_kind_requested.then_some(explicit_host_kind),
         );
-        // This would be nicer with destructuring assignment.
-        units = new_graph.0;
-        scrape_units = new_graph.1;
-        unit_graph = new_graph.2;
     }
 
     let mut extra_compiler_args = HashMap::new();
@@ -578,7 +574,15 @@ fn rebuild_unit_graph_shared(
     let new_roots = roots
         .iter()
         .map(|root| {
-            traverse_and_share(interner, &mut memo, &mut result, &unit_graph, root, to_host)
+            traverse_and_share(
+                interner,
+                &mut memo,
+                &mut result,
+                &unit_graph,
+                root,
+                false,
+                to_host,
+            )
         })
         .collect();
     // If no unit in the unit graph ended up having scrape units attached as dependencies,
@@ -602,6 +606,7 @@ fn traverse_and_share(
     new_graph: &mut UnitGraph,
     unit_graph: &UnitGraph,
     unit: &Unit,
+    unit_is_for_host: bool,
     to_host: Option<CompileKind>,
 ) -> Unit {
     if let Some(new_unit) = memo.get(unit) {
@@ -612,25 +617,83 @@ fn traverse_and_share(
     let new_deps: Vec<_> = unit_graph[unit]
         .iter()
         .map(|dep| {
-            let new_dep_unit =
-                traverse_and_share(interner, memo, new_graph, unit_graph, &dep.unit, to_host);
+            let new_dep_unit = traverse_and_share(
+                interner,
+                memo,
+                new_graph,
+                unit_graph,
+                &dep.unit,
+                dep.unit_for.is_for_host(),
+                to_host,
+            );
             new_dep_unit.hash(&mut dep_hash);
             UnitDep {
                 unit: new_dep_unit,
                 ..dep.clone()
             }
         })
         .collect();
+    // Here, we have recursively traversed this unit's dependencies, and hashed them: we can
+    // finalize the dep hash.
     let new_dep_hash = dep_hash.finish();
-    let new_kind = match to_host {
+
+    // This is the key part of the sharing process: if the unit is a runtime dependency, whose
+    // target is the same as the host, we canonicalize the compile kind to `CompileKind::Host`.
+    // A possible host dependency counterpart to this unit would have that kind, and if such a unit
+    // exists in the current `unit_graph`, they will unify in the new unit graph map `new_graph`.
+    // The resulting unit graph will be optimized with less units, thanks to sharing these host
+    // dependencies.
+    let canonical_kind = match to_host {
         Some(to_host) if to_host == unit.kind => CompileKind::Host,
         _ => unit.kind,
     };
+
+    let mut profile = unit.profile.clone();
+
+    // 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()
+        }
+    }
+
     let new_unit = interner.intern(
         &unit.pkg,
         &unit.target,
-        unit.profile.clone(),
-        new_kind,
+        profile,
+        canonical_kind,
         unit.mode,
         unit.features.clone(),
         unit.is_std,