From e6392cd380fce14d719890c78d5eb2657e8a6cfc Mon Sep 17 00:00:00 2001
From: gregce <gregce@google.com>
Date: Thu, 1 Jun 2017 19:51:00 +0200
Subject: Add .equals(), .hashCode() to EnableLipoTransition.

This fixes a subtle integration bug between PatchTransition, ConfiguredTargetFunction, and DependencyResolver.

Short story: ConfiguredTargetFunction.getDynamicConfigurations assumes at most one <Attribute, Label, Transition> entry per configured target dep. DependencyResolver.dependentNodeMap faithfully supplies this by returning an OrderedSetMultimap<Attribute, Dependency>, where each Dependency is a <Label, Transition>. OrderedSetMultimap guarantees no <key, value> repeats, but if you have two semantically equal EnableLipoTransition instances that don't satisfy .equals(), deduping fails.

Most PatchTransitions don't have this problem because they're singletons. But each EnableLipoTransition is instantiated with a Rule, so singletons don't work here.

This problem can only happen when a rule has multiple instances of the same C++ dep under the same attribute. But this isn't actually possible for normal attributes like "deps" because they don't allow duplicates (see RuleClass#checkForDuplicateLabels). Only non-LABEL_LIST attributes can trigger this.

PiperOrigin-RevId: 157733284
---
 .../build/lib/analysis/config/PatchTransition.java | 32 ++++++++++++++++------
 .../cpp/transitions/EnableLipoTransition.java      | 21 ++++++++++++++
 2 files changed, 45 insertions(+), 8 deletions(-)

(limited to 'src')

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
index 0fae3c07ea..33362b1dac 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/PatchTransition.java
@@ -21,16 +21,32 @@ import com.google.devtools.build.lib.packages.Attribute;
  * <p>The concept is simple: given the input configuration's build options, the
  * transition does whatever it wants to them and returns the modified result.
  *
- * <p>Implementations must be stateless: the transformation logic cannot use
- * any information from any data besides the input build options.
+ * <p>Implementations must be stateless: the output must exclusively depend on the
+ * input build options and any immutable member fields. Implementations must also override
+ * {@link Object#equals} and {@link Object#hashCode} unless exclusively accessed as
+ * singletons. For example:
  *
- * <p>For performance reasons, the input options are passed in as a <i>reference</i>,
- * not a <i>copy</i>. Transition implementations should <i>always</i> treat these
- * options as immutable, and call
+ * <pre>
+ * public class MyTransition implements PatchTransition {
+ *   public MyTransition INSTANCE = new MyTransition();
+ *
+ *   private MyTransition() {}
+ *
+ *   {@literal @}Override
+ *   public BuildOptions apply(BuildOptions options) {
+ *     BuildOptions toOptions = options.clone();
+ *     // Change some setting on toOptions
+ *     return toOptions;
+ *   }
+ * }
+ * </pre>
+ *
+ * <p>For performance reasons, the input options are passed as a <i>reference</i>, not a
+ * <i>copy</i>. Implementations should <i>always</i> treat these as immutable, and call
  * {@link com.google.devtools.build.lib.analysis.config.BuildOptions#clone}
- * before applying any mutations. Unfortunately,
- * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} does not currently
- * enforce immutability, so care must be taken not to modify the wrong instance.
+ * before making changes. Unfortunately,
+ * {@link com.google.devtools.build.lib.analysis.config.BuildOptions} doesn't currently
+ * enforce immutability. So care must be taken not to modify the wrong instance.
  */
 public interface PatchTransition extends Attribute.Transition {
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/transitions/EnableLipoTransition.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/transitions/EnableLipoTransition.java
index 3aacb5dd54..d61b73fd65 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/transitions/EnableLipoTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/transitions/EnableLipoTransition.java
@@ -17,15 +17,20 @@ package com.google.devtools.build.lib.rules.cpp.transitions;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.PatchTransition;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.rules.cpp.CppOptions;
 import com.google.devtools.build.lib.rules.cpp.CppOptions.LipoConfigurationState;
 
+import java.util.Objects;
+
 /**
  * Configuration transition that turns on LIPO/FDO settings for configurations that have them
  * disabled.
  */
+@Immutable
 public class EnableLipoTransition implements PatchTransition {
   private final Label ruleLabel;
+  private final int hashCode;
 
   /**
    * Creates a new transition that only triggers on the given rule. This can be used for
@@ -33,6 +38,7 @@ public class EnableLipoTransition implements PatchTransition {
    */
   public EnableLipoTransition(Label ruleLabel) {
     this.ruleLabel = ruleLabel;
+    this.hashCode = Objects.hashCode(ruleLabel);
   }
 
   @Override
@@ -52,4 +58,19 @@ public class EnableLipoTransition implements PatchTransition {
   public boolean defaultsToSelf() {
     return false;
   }
+
+  @Override
+  public boolean equals(Object other) {
+    if (this == other) {
+      return true;
+    } else if (!(other instanceof EnableLipoTransition)) {
+      return false;
+    }
+    return ruleLabel.equals(((EnableLipoTransition) other).ruleLabel);
+  }
+
+  @Override
+  public int hashCode() {
+    return hashCode;
+  }
 }
-- 
cgit v1.2.3