From 037bbcf67e707098bf8733108847b5d317be3af2 Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Tue, 17 Jan 2017 23:47:24 +0000 Subject: Change TargetContext.findDirectPrerequisite from config1 == config2 to config1.equals(c2). This fixes an obscure bug between dynamic configurations, host configuration caching, and Skyframe skyKey interning that makes Bazel crash under certain sequences of builds. See changes for details. -- PiperOrigin-RevId: 144766296 MOS_MIGRATED_REVID=144766296 --- .../com/google/devtools/build/lib/analysis/TargetContext.java | 4 +++- .../google/devtools/build/lib/skyframe/SkyframeBuildView.java | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) (limited to 'src/main/java/com/google/devtools/build/lib') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java index fc263efbab..f943e6ec76 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetContext.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.analysis; +import com.google.common.base.Objects; import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -104,7 +105,8 @@ public class TargetContext { */ TransitiveInfoCollection maybeFindDirectPrerequisite(Label label, BuildConfiguration config) { for (ConfiguredTarget prerequisite : directPrerequisites) { - if (prerequisite.getLabel().equals(label) && (prerequisite.getConfiguration() == config)) { + if (prerequisite.getLabel().equals(label) + && (Objects.equal(prerequisite.getConfiguration(), config))) { return prerequisite; } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index a905a9f68a..c84cf2726a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -528,6 +528,16 @@ public final class SkyframeBuildView { if (hostConfig != null) { return hostConfig; } + // TODO(bazel-team): investigate getting the trimmed config from Skyframe instead of cloning. + // This is the only place we instantiate BuildConfigurations outside of Skyframe, This can + // produce surprising effects, such as requesting a configuration that's in the Skyframe cache + // but still produces a unique instance because we don't check that cache. It'd be nice to + // guarantee that *all* instantiations happen through Skyframe. That could, for example, + // guarantee that config1.equals(config2) implies config1 == config2, which is nice for + // verifying we don't accidentally create extra configurations. But unfortunately, + // hostConfigurationCache was specifically created because Skyframe is too slow for this use + // case. So further optimization is necessary to make that viable (proto_library in particular + // contributes to much of the difference). BuildConfiguration trimmedConfig = topLevelHostConfiguration.clone(fragmentClasses, ruleClassProvider); hostConfigurationCache.put(fragmentClasses, trimmedConfig); -- cgit v1.2.3