Auto merge of #8701 - ehuss:unique-unit-dep-hash, r=alexcrichton

Fix non-determinism with new feature resolver.

This fixes a problem where Cargo was getting confused when two units were identical, but linked to different dependencies. Cargo generally assumes `Unit` is unique, but the new feature resolver can introduce a situation where two identical `Unit`s need to link to different dependencies. In particular, when building without the `--target` flag, the difference between a host unit and a target unit is not captured in the `Unit` structure. A dependency shared between normal dependencies and build dependencies can need to link to a second shared dependency whose features may be different.

The solution here is to build the unit graph pretending that `--target` was specified. Then, after the graph has been built, a second pass replaces `CompileKind::Target(host)` with `CompileKind::Host`, and adds a hash of the dependencies to the `Unit` to ensure it stays unique when necessary. This is done to ensure that dependencies are shared if possible.

I did a little performance testing, and I couldn't measure an appreciable difference. I also ran the tests in a loop for a few hours without problems.

An alternate solution here is to assume `--target=host` if `--target` isn't specified, and then have some kind of backwards-compatible linking in the `target` directory to retain the old directory layout. However, this would result in building shared host/normal dependencies twice. For *most* projects, this isn't a problem. This already happens when `--target` is specified, or `--release` is used (due to #8500). I'm just being very cautious because in a few projects this can be a large increase in build times. Maybe some day in the future we can be more bold and force this division, but I'm a little hesitant to make that jump.

Fixes #8549

Author: bors

src/cargo/core/compiler/build_context/target_info.rs

@@ -683,6 +683,17 @@ impl RustcTargetData {
             }
         }
 
+        // This is a hack. The unit_dependency graph builder "pretends" that
+        // `CompileKind::Host` is `CompileKind::Target(host)` if the
+        // `--target` flag is not specified. Since the unit_dependency code
+        // needs access to the target config data, create a copy so that it
+        // can be found. See `rebuild_unit_graph_shared` for why this is done.
+        if requested_kinds.iter().any(CompileKind::is_host) {
+            let ct = CompileTarget::new(&rustc.host)?;
+            target_info.insert(ct, host_info.clone());
+            target_config.insert(ct, host_config.clone());
+        }
+
         Ok(RustcTargetData {
             rustc,
             target_config,

src/cargo/core/compiler/standard_lib.rs

@@ -179,6 +179,7 @@ pub fn generate_std_roots(
                 mode,
                 features.clone(),
                 /*is_std*/ true,
+                /*dep_hash*/ 0,
             ));
         }
     }

src/cargo/core/compiler/unit.rs

@@ -57,6 +57,18 @@ pub struct UnitInner {
     pub features: Vec<InternedString>,
     /// Whether this is a standard library unit.
     pub is_std: bool,
+    /// A hash of all dependencies of this unit.
+    ///
+    /// This is used to keep the `Unit` unique in the situation where two
+    /// otherwise identical units need to link to different dependencies. This
+    /// can happen, for example, when there are shared dependencies that need
+    /// to be built with different features between normal and build
+    /// dependencies. See `rebuild_unit_graph_shared` for more on why this is
+    /// done.
+    ///
+    /// This value initially starts as 0, and then is filled in via a
+    /// second-pass after all the unit dependencies have been computed.
+    pub dep_hash: u64,
 }
 
 impl UnitInner {
@@ -123,6 +135,8 @@ impl fmt::Debug for Unit {
             .field("kind", &self.kind)
             .field("mode", &self.mode)
             .field("features", &self.features)
+            .field("is_std", &self.is_std)
+            .field("dep_hash", &self.dep_hash)
             .finish()
     }
 }
@@ -164,6 +178,7 @@ impl UnitInterner {
         mode: CompileMode,
         features: Vec<InternedString>,
         is_std: bool,
+        dep_hash: u64,
     ) -> Unit {
         let target = match (is_std, target.kind()) {
             // This is a horrible hack to support build-std. `libstd` declares
@@ -194,6 +209,7 @@ impl UnitInterner {
             mode,
             features,
             is_std,
+            dep_hash,
         });
         Unit { inner }
     }

src/cargo/core/compiler/unit_dependencies.rs

@@ -612,7 +612,7 @@ fn new_unit_dep_with_profile(
     let features = state.activated_features(pkg.package_id(), features_for);
     let unit = state
         .interner
-        .intern(pkg, target, profile, kind, mode, features, state.is_std);
+        .intern(pkg, target, profile, kind, mode, features, state.is_std, 0);
     Ok(UnitDep {
         unit,
         unit_for,

src/cargo/core/compiler/unit_graph.rs

@@ -16,7 +16,7 @@ pub struct UnitDep {
     /// The dependency unit.
     pub unit: Unit,
     /// The purpose of this dependency (a dependency for a test, or a build
-    /// script, etc.).
+    /// script, etc.). Do not use this after the unit graph has been built.
     pub unit_for: UnitFor,
     /// The name the parent uses to refer to this dependency.
     pub extern_crate_name: InternedString,

src/cargo/ops/cargo_compile.rs

@@ -23,13 +23,15 @@
 //!       repeats until the queue is empty.
 
 use std::collections::{BTreeSet, HashMap, HashSet};
+use std::hash::{Hash, Hasher};
 use std::iter::FromIterator;
 use std::sync::Arc;
 
+use crate::core::compiler::standard_lib;
 use crate::core::compiler::unit_dependencies::build_unit_dependencies;
-use crate::core::compiler::{standard_lib, unit_graph};
+use crate::core::compiler::unit_graph::{self, UnitDep, UnitGraph};
 use crate::core::compiler::{BuildConfig, BuildContext, Compilation, Context};
-use crate::core::compiler::{CompileKind, CompileMode, RustcTargetData, Unit};
+use crate::core::compiler::{CompileKind, CompileMode, CompileTarget, RustcTargetData, Unit};
 use crate::core::compiler::{DefaultExecutor, Executor, UnitInterner};
 use crate::core::profiles::{Profiles, UnitFor};
 use crate::core::resolver::features::{self, FeaturesFor};
@@ -39,7 +41,7 @@ use crate::core::{PackageId, PackageIdSpec, TargetKind, Workspace};
 use crate::ops;
 use crate::ops::resolve::WorkspaceResolve;
 use crate::util::config::Config;
-use crate::util::{closest_msg, profile, CargoResult};
+use crate::util::{closest_msg, profile, CargoResult, StableHasher};
 
 /// Contains information about how a package should be compiled.
 ///
@@ -410,11 +412,24 @@ pub fn create_bcx<'a, 'cfg>(
         workspace_resolve.as_ref().unwrap_or(&resolve),
     )?;
 
-    let units = generate_targets(
+    // If `--target` has not been specified, then the unit graph is built
+    // assuming `--target $HOST` was specified. See
+    // `rebuild_unit_graph_shared` for more on why this is done.
+    let explicit_host_kind = CompileKind::Target(CompileTarget::new(&target_data.rustc.host)?);
+    let explicit_host_kinds: Vec<_> = build_config
+        .requested_kinds
+        .iter()
+        .map(|kind| match kind {
+            CompileKind::Host => explicit_host_kind,
+            CompileKind::Target(t) => CompileKind::Target(*t),
+        })
+        .collect();
+
+    let mut units = generate_targets(
         ws,
         &to_builds,
         filter,
-        &build_config.requested_kinds,
+        &explicit_host_kinds,
         build_config.mode,
         &resolve,
         &workspace_resolve,
@@ -442,7 +457,7 @@ pub fn create_bcx<'a, 'cfg>(
             &crates,
             std_resolve,
             std_features,
-            &build_config.requested_kinds,
+            &explicit_host_kinds,
             &pkg_set,
             interner,
             &profiles,
@@ -451,6 +466,34 @@ pub fn create_bcx<'a, 'cfg>(
         Default::default()
     };
 
+    let mut unit_graph = build_unit_dependencies(
+        ws,
+        &pkg_set,
+        &resolve,
+        &resolved_features,
+        std_resolve_features.as_ref(),
+        &units,
+        &std_roots,
+        build_config.mode,
+        &target_data,
+        &profiles,
+        interner,
+    )?;
+
+    if build_config
+        .requested_kinds
+        .iter()
+        .any(CompileKind::is_host)
+    {
+        // Rebuild the unit graph, replacing the explicit host targets with
+        // CompileKind::Host, merging any dependencies shared with build
+        // dependencies.
+        let new_graph = rebuild_unit_graph_shared(interner, unit_graph, &units, explicit_host_kind);
+        // This would be nicer with destructuring assignment.
+        units = new_graph.0;
+        unit_graph = new_graph.1;
+    }
+
     let mut extra_compiler_args = HashMap::new();
     if let Some(args) = extra_args {
         if units.len() != 1 {
@@ -485,20 +528,6 @@ pub fn create_bcx<'a, 'cfg>(
         }
     }
 
-    let unit_graph = build_unit_dependencies(
-        ws,
-        &pkg_set,
-        &resolve,
-        &resolved_features,
-        std_resolve_features.as_ref(),
-        &units,
-        &std_roots,
-        build_config.mode,
-        &target_data,
-        &profiles,
-        interner,
-    )?;
-
     let bcx = BuildContext::new(
         ws,
         pkg_set,
@@ -789,6 +818,7 @@ fn generate_targets(
                     target_mode,
                     features.clone(),
                     /*is_std*/ false,
+                    /*dep_hash*/ 0,
                 );
                 units.insert(unit);
             }
@@ -1169,3 +1199,93 @@ fn filter_targets<'a>(
     }
     proposals
 }
+
+/// This is used to rebuild the unit graph, sharing host dependencies if possible.
+///
+/// This will translate any unit's `CompileKind::Target(host)` to
+/// `CompileKind::Host` if the kind is equal to `to_host`. This also handles
+/// generating the unit `dep_hash`, and merging shared units if possible.
+///
+/// This is necessary because if normal dependencies used `CompileKind::Host`,
+/// there would be no way to distinguish those units from build-dependency
+/// units. This can cause a problem if a shared normal/build dependency needs
+/// to link to another dependency whose features differ based on whether or
+/// not it is a normal or build dependency. If both units used
+/// `CompileKind::Host`, then they would end up being identical, causing a
+/// collision in the `UnitGraph`, and Cargo would end up randomly choosing one
+/// value or the other.
+///
+/// The solution is to keep normal and build dependencies separate when
+/// building the unit graph, and then run this second pass which will try to
+/// combine shared dependencies safely. By adding a hash of the dependencies
+/// to the `Unit`, this allows the `CompileKind` to be changed back to `Host`
+/// without fear of an unwanted collision.
+fn rebuild_unit_graph_shared(
+    interner: &UnitInterner,
+    unit_graph: UnitGraph,
+    roots: &[Unit],
+    to_host: CompileKind,
+) -> (Vec<Unit>, UnitGraph) {
+    let mut result = UnitGraph::new();
+    // Map of the old unit to the new unit, used to avoid recursing into units
+    // that have already been computed to improve performance.
+    let mut memo = HashMap::new();
+    let new_roots = roots
+        .iter()
+        .map(|root| {
+            traverse_and_share(interner, &mut memo, &mut result, &unit_graph, root, to_host)
+        })
+        .collect();
+    (new_roots, result)
+}
+
+/// Recursive function for rebuilding the graph.
+///
+/// This walks `unit_graph`, starting at the given `unit`. It inserts the new
+/// units into `new_graph`, and returns a new updated version of the given
+/// unit (`dep_hash` is filled in, and `kind` switched if necessary).
+fn traverse_and_share(
+    interner: &UnitInterner,
+    memo: &mut HashMap<Unit, Unit>,
+    new_graph: &mut UnitGraph,
+    unit_graph: &UnitGraph,
+    unit: &Unit,
+    to_host: CompileKind,
+) -> Unit {
+    if let Some(new_unit) = memo.get(unit) {
+        // Already computed, no need to recompute.
+        return new_unit.clone();
+    }
+    let mut dep_hash = StableHasher::new();
+    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);
+            new_dep_unit.hash(&mut dep_hash);
+            UnitDep {
+                unit: new_dep_unit,
+                ..dep.clone()
+            }
+        })
+        .collect();
+    let new_dep_hash = dep_hash.finish();
+    let new_kind = if unit.kind == to_host {
+        CompileKind::Host
+    } else {
+        unit.kind
+    };
+    let new_unit = interner.intern(
+        &unit.pkg,
+        &unit.target,
+        unit.profile,
+        new_kind,
+        unit.mode,
+        unit.features.clone(),
+        unit.is_std,
+        new_dep_hash,
+    );
+    assert!(memo.insert(unit.clone(), new_unit.clone()).is_none());
+    new_graph.entry(new_unit.clone()).or_insert(new_deps);
+    new_unit
+}

src/cargo/util/config/mod.rs

@@ -1788,7 +1788,7 @@ pub struct CargoBuildConfig {
 /// a = 'a b c'
 /// b = ['a', 'b', 'c']
 /// ```
-#[derive(Debug, Deserialize)]
+#[derive(Debug, Deserialize, Clone)]
 pub struct StringList(Vec<String>);
 
 impl StringList {

src/cargo/util/config/target.rs

@@ -20,7 +20,7 @@ pub struct TargetCfgConfig {
 }
 
 /// Config definition of a `[target]` table.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct TargetConfig {
     /// Process to run as a wrapper for `cargo run`, `test`, and `bench` commands.
     pub runner: OptValue<PathAndArgs>,