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

3 files changed, 83 insertions, 1 deletions

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);
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTargetTest.java
index 0243d36273..228472c30f 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTargetTest.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.lib.analysis;
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.devtools.build.lib.analysis.util.BuildViewTestBase;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
+import com.google.devtools.build.lib.skyframe.SkyframeBuildView;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -124,4 +126,72 @@ public class OutputFileConfiguredTargetTest extends BuildViewTestBase {
         getConfiguredTarget("//foo:host_src2.cc", getHostConfiguration());
     assertThat(hostSrc2.getGeneratingRule()).isNotNull();
   }
+
+  /**
+   * Dynamic configurations maintain a host configuration cache that stores configurations
+   * instantiated outside of Skyframe. We shouldn't, in general, instantiate configurations
+   * outside of {@link BuildConfigurationValue}. But in this specific case Bazel performance
+   * suffers through the Skyframe interface and maintaining a local cache is much faster.
+   * See {@link SkyframeBuildView} for details.
+   *
+   * <p>One consequence of this is that requesting a config that would be a Skyframe cache hit
+   * can still produce a distinct instance. Meaning you can get cases where {@code
+   * config1.equals(config2) && config1 != config2}.
+   *
+   * <p>This test checks for such a case: perform three consecutive Bazel builds. The first builds
+   * with default options, producing top-level host config H1 and configured target
+   * <host_generated_file_producer, H1>. The second builds with {@code --host_copt=a=b},
+   * producing host config H2 (and clearing the top-level host config cache since the host config
+   * changed). The third builds back with default options. This once again clears the host config
+   * cache, since the host config changed again. So that cache creates a new config H3 where
+   * H3.equals(H1) and instantiates new configured target <host_src3.cc, H3>. It then requests
+   * dependency <host_generated_file_producer, H3> from Skyframe, but the Skyframe SkyKey interner
+   * reduces this to the previously seen <host_generated_file_producer, H1> and returns that
+   * instead.
+   *
+   * <p>This produces the expected scenario where the output file's config is value-equal but not
+   * reference-equal to its generating rule's config.
+   */
+  @Test
+  public void dynamicConfigsWithHostConfigSwitch() throws Exception {
+    scratch.file("foo/BUILD",
+        "genrule(",
+        "    name = 'host_generated_file_producer',",
+        "    srcs = [],",
+        "    outs = [",
+        "        'host_src1.cc',",
+        "        'host_src2.cc',",
+        "        'host_src3.cc',",
+        "    ],",
+        "    cmd = 'echo hi > $(location host_src1.cc); echo hi > $(location host_src2.cc); "
+            + "echo hi > $(location host_src3.cc)')",
+        "",
+        "cc_binary(name = 'host_generated_file_consumer1', srcs = ['host_src1.cc'])",
+        "cc_binary(name = 'host_generated_file_consumer2', srcs = ['host_src2.cc'])",
+        "cc_binary(name = 'host_generated_file_consumer3', srcs = ['host_src3.cc'])",
+        "",
+        "genrule(name = 'gen1', srcs = [], outs = ['gen1.out'], cmd = 'echo hi > $@',",
+        "    tools = [':host_generated_file_consumer1'])",
+        "genrule(name = 'gen2', srcs = [], outs = ['gen2.out'], cmd = 'echo hi > $@',",
+        "    tools = [':host_generated_file_consumer2'])",
+        "genrule(name = 'gen3', srcs = [], outs = ['gen3.out'], cmd = 'echo hi > $@',",
+        "    tools = [':host_generated_file_consumer3'])");
+
+    String dynamicConfigsMode = "--experimental_dynamic_configs=notrim";
+    useConfiguration(dynamicConfigsMode);
+    update("//foo:gen1");
+    useConfiguration(dynamicConfigsMode, "--host_copt", "a=b");
+    update("//foo:gen2");
+    useConfiguration(dynamicConfigsMode);
+    update("//foo:gen3");
+
+    OutputFileConfiguredTarget hostSrc3 = (OutputFileConfiguredTarget)
+        getConfiguredTarget("//foo:host_src3.cc", getHostConfiguration());
+    TransitiveInfoCollection hostGeneratedFileConsumer3 = hostSrc3.getGeneratingRule();
+    assertThat(hostSrc3.getConfiguration())
+        .isEqualTo(hostGeneratedFileConsumer3.getConfiguration());
+    // TODO(gregce): enable below for Bazel tests, which for some reason realize the same instance
+//    assertThat(hostSrc3.getConfiguration())
+//        .isNotSameAs(hostGeneratedFileConsumer3.getConfiguration());
+  }
 }