From 375f72b29ab28db3df093531b978c4370b059d54 Mon Sep 17 00:00:00 2001
From: mstaib <mstaib@google.com>
Date: Thu, 18 Apr 2019 14:29:04 -0700
Subject: [PATCH] Add safety checks for toolchain resolution in preparation for
 trimming.

Trimming has to make assumptions about what configuration is used in the
process of calculating toolchains. These checks enforce those assumptions.

Progress on #6524.

PiperOrigin-RevId: 244258819
---
 .../build/lib/analysis/ToolchainResolver.java | 22 ++++++++--
 .../skyframe/ConfiguredTargetFunction.java    |  2 +
 .../lib/skyframe/PlatformLookupUtil.java      | 43 +++++++++++++++----
 .../RegisteredExecutionPlatformsFunction.java | 22 +++++++++-
 .../RegisteredToolchainsFunction.java         | 19 ++++++++
 .../SingleToolchainResolutionFunction.java    |  5 ++-
 .../lib/skyframe/PlatformLookupUtilTest.java  |  2 +-
 7 files changed, 100 insertions(+), 15 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
index ead2d5268edce5..04e548a82b5f67 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ToolchainResolver.java
@@ -66,6 +66,7 @@ public class ToolchainResolver {
   // Optional data.
   private ImmutableSet<Label> requiredToolchainTypeLabels = ImmutableSet.of();
   private ImmutableSet<Label> execConstraintLabels = ImmutableSet.of();
+  private boolean shouldSanityCheckConfiguration = false;
 
   // Determined during execution.
   private boolean debug = false;
@@ -100,6 +101,17 @@ public ToolchainResolver setExecConstraintLabels(Set<Label> execConstraintLabels
     return this;
   }
 
+  /**
+   * Sets whether the experimental retroactive trimming mode is in use. This determines whether
+   * sanity checks regarding the fragments in use for the configurations of platforms and toolchains
+   * are used - specifically, whether platforms use only the PlatformConfiguration, and toolchains
+   * do not use any configuration at all.
+   */
+  public ToolchainResolver setShouldSanityCheckConfiguration(boolean useSanityChecks) {
+    this.shouldSanityCheckConfiguration = useSanityChecks;
+    return this;
+  }
+
   /**
    * Determines the specific toolchains that are required, given the requested toolchain types,
    * target platform, and configuration.
@@ -187,7 +199,9 @@ private PlatformKeys loadPlatformKeys(
 
     // Load the host and target platforms early, to check for errors.
     PlatformLookupUtil.getPlatformInfo(
-        ImmutableList.of(hostPlatformKey, targetPlatformKey), environment);
+        ImmutableList.of(hostPlatformKey, targetPlatformKey),
+        environment,
+        shouldSanityCheckConfiguration);
     if (environment.valuesMissing()) {
       throw new ValueMissingException();
     }
@@ -245,7 +259,8 @@ private ImmutableList<ConfiguredTargetKey> filterAvailablePlatforms(
     // platform is the host platform), Skyframe will return the correct results immediately without
     // need of a restart.
     Map<ConfiguredTargetKey, PlatformInfo> platformInfoMap =
-        PlatformLookupUtil.getPlatformInfo(platformKeys, environment);
+        PlatformLookupUtil.getPlatformInfo(
+            platformKeys, environment, shouldSanityCheckConfiguration);
     if (platformInfoMap == null) {
       throw new ValueMissingException();
     }
@@ -367,7 +382,8 @@ private void determineToolchainImplementations(
     Map<ConfiguredTargetKey, PlatformInfo> platforms =
         PlatformLookupUtil.getPlatformInfo(
             ImmutableList.of(selectedExecutionPlatformKey.get(), platformKeys.targetPlatformKey()),
-            environment);
+            environment,
+            shouldSanityCheckConfiguration);
     if (platforms == null) {
       throw new ValueMissingException();
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 854ffe35ed1b12..5c6481b2fafb56 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -313,6 +313,8 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc
               new ToolchainResolver(env, configuredTargetKey.getConfigurationKey())
                   .setRequiredToolchainTypes(requiredToolchains)
                   .setExecConstraintLabels(execConstraintLabels)
+                  .setShouldSanityCheckConfiguration(
+                      configuration.trimConfigurationsRetroactively())
                   .resolve();
           if (env.valuesMissing()) {
             return null;
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
index d9f998b8030c55..4c1c306eb1f2e9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtil.java
@@ -14,8 +14,12 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.common.base.Predicates.equalTo;
+import static com.google.common.base.Predicates.not;
+
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.PlatformConfiguration;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -33,7 +37,7 @@ public class PlatformLookupUtil {
 
   @Nullable
   public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
-      Iterable<ConfiguredTargetKey> platformKeys, Environment env)
+      Iterable<ConfiguredTargetKey> platformKeys, Environment env, boolean sanityCheckConfiguration)
       throws InterruptedException, InvalidPlatformException {
 
     Map<
@@ -49,7 +53,7 @@ public static Map<ConfiguredTargetKey, PlatformInfo> getPlatformInfo(
     boolean valuesMissing = env.valuesMissing();
     Map<ConfiguredTargetKey, PlatformInfo> platforms = valuesMissing ? null : new HashMap<>();
     for (ConfiguredTargetKey key : platformKeys) {
-      PlatformInfo platformInfo = findPlatformInfo(key, values.get(key));
+      PlatformInfo platformInfo = findPlatformInfo(key, values.get(key), sanityCheckConfiguration);
       if (!valuesMissing && platformInfo != null) {
         platforms.put(key, platformInfo);
       }
@@ -72,7 +76,8 @@ private static PlatformInfo findPlatformInfo(
       ConfiguredTargetKey key,
       ValueOrException3<
               ConfiguredValueCreationException, NoSuchThingException, ActionConflictException>
-          valueOrException)
+          valueOrException,
+      boolean sanityCheckConfiguration)
       throws InvalidPlatformException {
 
     try {
@@ -82,6 +87,21 @@ private static PlatformInfo findPlatformInfo(
       }
 
       ConfiguredTarget configuredTarget = ctv.getConfiguredTarget();
+      BuildConfigurationValue.Key configurationKey = configuredTarget.getConfigurationKey();
+      // This check is necessary because trimming for other rules assumes that platform resolution
+      // uses the platform fragment and _only_ the platform fragment. Without this check, it's
+      // possible another fragment could slip in without us realizing, and thus break this
+      // assumption.
+      if (sanityCheckConfiguration
+          && configurationKey.getFragments().stream()
+              .anyMatch(not(equalTo(PlatformConfiguration.class)))) {
+        // Only the PlatformConfiguration fragment may be present on a platform rule in retroactive
+        // trimming mode.
+        throw new InvalidPlatformException(
+            configuredTarget.getLabel(),
+            "has fragments other than PlatformConfiguration, "
+                + "which is forbidden in retroactive trimming mode");
+      }
       PlatformInfo platformInfo = PlatformProviderUtils.platform(configuredTarget);
       if (platformInfo == null) {
         throw new InvalidPlatformException(configuredTarget.getLabel());
@@ -99,12 +119,14 @@ private static PlatformInfo findPlatformInfo(
 
   /** Exception used when a platform label is not a valid platform. */
   public static final class InvalidPlatformException extends ToolchainException {
+    private static final String DEFAULT_ERROR = "does not provide PlatformInfo";
+
     InvalidPlatformException(Label label) {
-      super(formatError(label));
+      super(formatError(label, DEFAULT_ERROR));
     }
 
     InvalidPlatformException(Label label, ConfiguredValueCreationException e) {
-      super(formatError(label), e);
+      super(formatError(label, DEFAULT_ERROR), e);
     }
 
     public InvalidPlatformException(Label label, NoSuchThingException e) {
@@ -113,12 +135,15 @@ public InvalidPlatformException(Label label, NoSuchThingException e) {
     }
 
     public InvalidPlatformException(Label label, ActionConflictException e) {
-      super(formatError(label), e);
+      super(formatError(label, DEFAULT_ERROR), e);
+    }
+
+    InvalidPlatformException(Label label, String error) {
+      super(formatError(label, error));
     }
 
-    private static String formatError(Label label) {
-      return String.format(
-          "Target %s was referenced as a platform, but does not provide PlatformInfo", label);
+    private static String formatError(Label label, String error) {
+      return String.format("Target %s was referenced as a platform, but %s", label, error);
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
index 9348cecf1d0248..255fe0e959cd65 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredExecutionPlatformsFunction.java
@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.common.base.Predicates.equalTo;
+import static com.google.common.base.Predicates.not;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.auto.value.AutoValue;
@@ -92,7 +94,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
     // Load the configured target for each, and get the declared execution platforms providers.
     ImmutableList<ConfiguredTargetKey> registeredExecutionPlatformKeys =
-        configureRegisteredExecutionPlatforms(env, configuration, platformLabels);
+        configureRegisteredExecutionPlatforms(
+            env, configuration, configuration.trimConfigurationsRetroactively(), platformLabels);
     if (env.valuesMissing()) {
       return null;
     }
@@ -122,6 +125,7 @@ public static List<String> getWorkspaceExecutionPlatforms(Environment env)
   private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms(
       Environment env,
       BuildConfiguration configuration,
+      boolean sanityCheckConfiguration,
       List<Label> labels)
       throws InterruptedException, RegisteredExecutionPlatformsFunctionException {
 
@@ -145,6 +149,22 @@ private ImmutableList<ConfiguredTargetKey> configureRegisteredExecutionPlatforms
         }
         ConfiguredTarget target =
             ((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget();
+        // This check is necessary because trimming for other rules assumes that platform resolution
+        // uses the platform fragment and _only_ the platform fragment. Without this check, it's
+        // possible another fragment could slip in without us realizing, and thus break this
+        // assumption.
+        if (sanityCheckConfiguration
+            && target.getConfigurationKey().getFragments().stream()
+                .anyMatch(not(equalTo(PlatformConfiguration.class)))) {
+          // Only the PlatformConfiguration fragment may be present on a platform rule in
+          // retroactive trimming mode.
+          throw new RegisteredExecutionPlatformsFunctionException(
+              new InvalidPlatformException(
+                  target.getLabel(),
+                  "has fragments other than PlatformConfiguration, "
+                      + "which is forbidden in retroactive trimming mode"),
+              Transience.PERSISTENT);
+        }
         PlatformInfo platformInfo = PlatformProviderUtils.platform(target);
         if (platformInfo == null) {
           throw new RegisteredExecutionPlatformsFunctionException(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
index 69ac1b77acc44e..7d7b054abfd4ac 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RegisteredToolchainsFunction.java
@@ -145,8 +145,23 @@ private ImmutableList<DeclaredToolchainInfo> configureRegisteredToolchains(
           valuesMissing = true;
           continue;
         }
+
         ConfiguredTarget target =
             ((ConfiguredTargetValue) valueOrException.get()).getConfiguredTarget();
+        if (configuration.trimConfigurationsRetroactively()
+            && !target.getConfigurationKey().getFragments().isEmpty()) {
+          // No fragment may be present on a toolchain rule in retroactive trimming mode.
+          // This is because trimming expects that platform and toolchain resolution uses only
+          // the platform configuration. In theory, this means toolchains could use platforms, but
+          // the current expectation is that toolchains should not use anything at all, so better
+          // to go with the stricter expectation for now.
+          throw new RegisteredToolchainsFunctionException(
+              new InvalidToolchainLabelException(
+                  toolchainLabel,
+                  "this toolchain uses configuration, which is forbidden in retroactive trimming "
+                      + "mode"),
+              Transience.PERSISTENT);
+        }
         DeclaredToolchainInfo toolchainInfo = target.getProvider(DeclaredToolchainInfo.class);
 
         if (toolchainInfo == null) {
@@ -185,6 +200,10 @@ public InvalidToolchainLabelException(Label invalidLabel) {
               "target does not provide the DeclaredToolchainInfo provider"));
     }
 
+    public InvalidToolchainLabelException(Label invalidLabel, String reason) {
+      super(formatMessage(invalidLabel.getCanonicalForm(), reason));
+    }
+
     public InvalidToolchainLabelException(TargetPatternUtil.InvalidTargetPatternException e) {
       this(e.getInvalidPattern(), e.getTpe());
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
index 2313d704a456f4..1e37fe43a2675e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
@@ -82,6 +82,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         key.toolchainTypeLabel(),
         key.availableExecutionPlatformKeys(),
         key.targetPlatformKey(),
+        configuration.trimConfigurationsRetroactively(),
         toolchains.registeredToolchains(),
         env,
         debug ? env.getListener() : null);
@@ -97,6 +98,7 @@ private static SingleToolchainResolutionValue resolveConstraints(
       Label toolchainTypeLabel,
       List<ConfiguredTargetKey> availableExecutionPlatformKeys,
       ConfiguredTargetKey targetPlatformKey,
+      boolean sanityCheckConfigurations,
       ImmutableList<DeclaredToolchainInfo> toolchains,
       Environment env,
       @Nullable EventHandler eventHandler)
@@ -112,7 +114,8 @@ private static SingleToolchainResolutionValue resolveConstraints(
                   .add(targetPlatformKey)
                   .addAll(availableExecutionPlatformKeys)
                   .build(),
-              env);
+              env,
+              sanityCheckConfigurations);
       if (env.valuesMissing()) {
         return null;
       }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
index cf3c5908a34452..a851e8bbe0e73e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PlatformLookupUtilTest.java
@@ -175,7 +175,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       GetPlatformInfoKey key = (GetPlatformInfoKey) skyKey;
       try {
         Map<ConfiguredTargetKey, PlatformInfo> platforms =
-            PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env);
+            PlatformLookupUtil.getPlatformInfo(key.platformKeys(), env, false);
         if (env.valuesMissing()) {
           return null;
         }