From 9efa0d55265a740d850ef2d5e6bec72014d497eb Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 12 Sep 2020 13:43:04 -0700 Subject: [PATCH] Fix non-determinism with new feature resolver. --- .../compiler/build_context/target_info.rs | 11 ++ src/cargo/core/compiler/standard_lib.rs | 1 + src/cargo/core/compiler/unit.rs | 16 ++ src/cargo/core/compiler/unit_dependencies.rs | 2 +- src/cargo/core/compiler/unit_graph.rs | 2 +- src/cargo/ops/cargo_compile.rs | 160 +++++++++++++++--- src/cargo/util/config/mod.rs | 2 +- src/cargo/util/config/target.rs | 2 +- tests/testsuite/features2.rs | 115 +++++++++++++ 9 files changed, 287 insertions(+), 24 deletions(-) diff --git a/src/cargo/core/compiler/build_context/target_info.rs b/src/cargo/core/compiler/build_context/target_info.rs index a36017f3e38..53fe197707c 100644 --- a/src/cargo/core/compiler/build_context/target_info.rs +++ b/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, diff --git a/src/cargo/core/compiler/standard_lib.rs b/src/cargo/core/compiler/standard_lib.rs index ed3dd77336c..214b4b43703 100644 --- a/src/cargo/core/compiler/standard_lib.rs +++ b/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, )); } } diff --git a/src/cargo/core/compiler/unit.rs b/src/cargo/core/compiler/unit.rs index b4422f50474..71b4538c4f9 100644 --- a/src/cargo/core/compiler/unit.rs +++ b/src/cargo/core/compiler/unit.rs @@ -57,6 +57,18 @@ pub struct UnitInner { pub features: Vec, /// 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, 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 } } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 5bd8c0aff87..8ebe21b9002 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/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, diff --git a/src/cargo/core/compiler/unit_graph.rs b/src/cargo/core/compiler/unit_graph.rs index 243a32a6ae2..5d7e4cde607 100644 --- a/src/cargo/core/compiler/unit_graph.rs +++ b/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, diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index b8dbc211d72..c6d453f9516 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/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, 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, + 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 +} diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index b3d1d7b5e86..dde02ac6253 100644 --- a/src/cargo/util/config/mod.rs +++ b/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); impl StringList { diff --git a/src/cargo/util/config/target.rs b/src/cargo/util/config/target.rs index 956b5817231..7ae029f02f4 100644 --- a/src/cargo/util/config/target.rs +++ b/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, diff --git a/tests/testsuite/features2.rs b/tests/testsuite/features2.rs index 3790ab4be31..7a5f0c087b2 100644 --- a/tests/testsuite/features2.rs +++ b/tests/testsuite/features2.rs @@ -1749,3 +1749,118 @@ foo v0.1.0 ([..]/foo) ) .run(); } + +#[cargo_test] +fn shared_dep_same_but_dependencies() { + // Checks for a bug of nondeterminism. This scenario creates a shared + // dependency `dep` which needs to be built twice (once as normal, and + // once as a build dep). However, in both cases the flags to `dep` are the + // same, the only difference is what it links to. The normal dependency + // should link to `subdep` with the feature disabled, and the build + // dependency should link to it with it enabled. Crucially, the `--target` + // flag should not be specified, otherwise Unit.kind would be different + // and avoid the collision, and this bug won't manifest. + let p = project() + .file( + "Cargo.toml", + r#" + [workspace] + members = ["bin1", "bin2"] + "#, + ) + .file( + "bin1/Cargo.toml", + r#" + [package] + name = "bin1" + version = "0.1.0" + + [dependencies] + dep = { path = "../dep" } + "#, + ) + .file("bin1/src/main.rs", "fn main() { dep::feat_func(); }") + .file( + "bin2/Cargo.toml", + r#" + [package] + name = "bin2" + version = "0.1.0" + + [build-dependencies] + dep = { path = "../dep" } + subdep = { path = "../subdep", features = ["feat"] } + "#, + ) + .file("bin2/build.rs", "fn main() { dep::feat_func(); }") + .file("bin2/src/main.rs", "fn main() {}") + .file( + "dep/Cargo.toml", + r#" + [package] + name = "dep" + version = "0.1.0" + + [dependencies] + subdep = { path = "../subdep" } + "#, + ) + .file( + "dep/src/lib.rs", + "pub fn feat_func() { subdep::feat_func(); }", + ) + .file( + "subdep/Cargo.toml", + r#" + [package] + name = "subdep" + version = "0.1.0" + + [features] + feat = [] + "#, + ) + .file( + "subdep/src/lib.rs", + r#" + pub fn feat_func() { + #[cfg(feature = "feat")] println!("cargo:warning=feat: enabled"); + #[cfg(not(feature = "feat"))] println!("cargo:warning=feat: not enabled"); + } + "#, + ) + .build(); + + p.cargo("build --bin bin1 --bin bin2 -Zfeatures=all") + .masquerade_as_nightly_cargo() + // unordered because bin1 and bin2 build at the same time + .with_stderr_unordered( + "\ +[COMPILING] subdep [..] +[COMPILING] dep [..] +[COMPILING] bin2 [..] +[COMPILING] bin1 [..] +warning: feat: enabled +[FINISHED] [..] +", + ) + .run(); + p.process(p.bin("bin1")) + .with_stdout("cargo:warning=feat: not enabled") + .run(); + + // Make sure everything stays cached. + p.cargo("build -v --bin bin1 --bin bin2 -Zfeatures=all") + .masquerade_as_nightly_cargo() + .with_stderr_unordered( + "\ +[FRESH] subdep [..] +[FRESH] dep [..] +[FRESH] bin1 [..] +warning: feat: enabled +[FRESH] bin2 [..] +[FINISHED] [..] +", + ) + .run(); +}