Do not duplicate build variables, reuse variables from cc toolchain

Before this cl each linking and compilation action would contain a full copy of
all build variables. However, some build variables can be reused, for the
memory consumption benefit. With this cl, we contruct a Variables instance in
the CcToolchain, and make it a parent of all per-linking and per-compilation
variables.

RELNOTES: None.
PiperOrigin-RevId: 169233756

11 files changed, 79 insertions, 69 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/apple/cpp/AppleCcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/apple/cpp/AppleCcToolchain.java
index 5623b49835..5c78577ead 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/apple/cpp/AppleCcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/apple/cpp/AppleCcToolchain.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.rules.apple.ApplePlatform;
 import com.google.devtools.build.lib.rules.apple.AppleToolchain;
 import com.google.devtools.build.lib.rules.apple.XcodeConfig;
 import com.google.devtools.build.lib.rules.cpp.CcToolchain;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -55,7 +56,7 @@ public class AppleCcToolchain extends CcToolchain {
   public static final String APPLE_SDK_PLATFORM_VALUE_KEY = "apple_sdk_platform_value";
 
   @Override
-  protected Map<String, String> getBuildVariables(RuleContext ruleContext)
+  protected void addBuildVariables(RuleContext ruleContext, Variables.Builder variables)
       throws RuleErrorException {
     AppleConfiguration appleConfiguration = ruleContext.getFragment(AppleConfiguration.class);
 
@@ -67,44 +68,44 @@ public class AppleCcToolchain extends CcToolchain {
 
     Map<String, String> appleEnv = getEnvironmentBuildVariables(ruleContext);
 
-    return ImmutableMap.<String, String>builder()
-        .put(
+    variables
+        .addStringVariable(
             XCODE_VERSION_KEY,
             XcodeConfig.getXcodeVersion(ruleContext).toStringWithMinimumComponents(2))
-        .put(
+        .addStringVariable(
             IOS_SDK_VERSION_KEY,
             XcodeConfig.getSdkVersionForPlatform(ruleContext, ApplePlatform.IOS_SIMULATOR)
                 .toStringWithMinimumComponents(2))
-        .put(
+        .addStringVariable(
             MACOS_SDK_VERSION_KEY,
             XcodeConfig.getSdkVersionForPlatform(ruleContext, ApplePlatform.MACOS)
                 .toStringWithMinimumComponents(2))
-        .put(
+        .addStringVariable(
             TVOS_SDK_VERSION_KEY,
             XcodeConfig.getSdkVersionForPlatform(ruleContext, ApplePlatform.TVOS_SIMULATOR)
                 .toStringWithMinimumComponents(2))
-        .put(
+        .addStringVariable(
             WATCHOS_SDK_VERSION_KEY,
             XcodeConfig.getSdkVersionForPlatform(ruleContext, ApplePlatform.WATCHOS_SIMULATOR)
                 .toStringWithMinimumComponents(2))
-        .put(SDK_DIR_KEY, AppleToolchain.sdkDir())
-        .put(SDK_FRAMEWORK_DIR_KEY, AppleToolchain.sdkFrameworkDir(platform, ruleContext))
-        .put(
+        .addStringVariable(SDK_DIR_KEY, AppleToolchain.sdkDir())
+        .addStringVariable(
+            SDK_FRAMEWORK_DIR_KEY, AppleToolchain.sdkFrameworkDir(platform, ruleContext))
+        .addStringVariable(
             PLATFORM_DEVELOPER_FRAMEWORK_DIR,
             AppleToolchain.platformDeveloperFrameworkDir(appleConfiguration))
-        .put(
+        .addStringVariable(
             XCODE_VERISON_OVERRIDE_VALUE_KEY,
             appleEnv.getOrDefault(AppleConfiguration.XCODE_VERSION_ENV_NAME, ""))
-        .put(
+        .addStringVariable(
             APPLE_SDK_VERSION_OVERRIDE_VALUE_KEY,
             appleEnv.getOrDefault(AppleConfiguration.APPLE_SDK_VERSION_ENV_NAME, ""))
-        .put(
+        .addStringVariable(
             APPLE_SDK_PLATFORM_VALUE_KEY,
             appleEnv.getOrDefault(AppleConfiguration.APPLE_SDK_PLATFORM_ENV_NAME, ""))
-        .put(
+        .addStringVariable(
             VERSION_MIN_KEY,
-            XcodeConfig.getMinimumOsForPlatformType(ruleContext, platform.getType()).toString())
-        .build();
+            XcodeConfig.getMinimumOsForPlatformType(ruleContext, platform.getType()).toString());
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 18c185d432..b04b01a6b6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -692,9 +692,7 @@ public final class CcCommon {
       return null;
     }
 
-    Variables buildVariables = new Variables.Builder()
-        .addAllStringVariables(toolchainProvider.getBuildVariables())
-        .build();
+    Variables buildVariables = toolchainProvider.getBuildVariables();
     String toolchainCcFlags =
         Joiner.on(" ")
             .join(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index 939048d36b..0bd4093864 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -45,6 +45,8 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.License;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.Builder;
 import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Pair;
@@ -58,7 +60,6 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 
 /**
  * Implementation for the cc_toolchain rule.
@@ -526,7 +527,7 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
     TransitiveInfoCollection dep = context.getPrerequisite(attribute, Mode.HOST);
     return dep != null
         ? getFiles(context, attribute)
-        : NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
+        : NestedSetBuilder.emptySet(Order.STABLE_ORDER);
   }
 
   private MakeVariableInfo createMakeVariableProvider(
@@ -546,16 +547,34 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
   }
 
   /**
-   * Returns a map that should be templated into the crosstool as build variables. Is meant to
-   * be overridden by subclasses of CcToolchain.
+   * Returns {@link Variables} instance with build variables that only depend on the toolchain.
    *
    * @param ruleContext the rule context
    * @throws RuleErrorException if there are configuration errors making it impossible to resolve
    *     certain build variables of this toolchain
    */
-  protected Map<String, String> getBuildVariables(RuleContext ruleContext)
+  private final Variables getBuildVariables(RuleContext ruleContext) throws RuleErrorException {
+    Variables.Builder variables = new Variables.Builder();
+
+    PathFragment sysroot = calculateSysroot(ruleContext);
+    if (sysroot != null) {
+      variables.addStringVariable(CppModel.SYSROOT_VARIABLE_NAME, sysroot.getPathString());
+    }
+
+    addBuildVariables(ruleContext, variables);
+
+    return variables.build();
+  }
+
+  /**
+   * Add local build variables from subclasses into {@link Variables} returned from {@link
+   * #getBuildVariables(RuleContext)}.
+   *
+   * <p>This method is meant to be overridden by subclasses of CcToolchain.
+   */
+  protected void addBuildVariables(RuleContext ruleContext, Builder variables)
       throws RuleErrorException {
-    return ImmutableMap.<String, String>of();
+    // To be overridden in subclasses.
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
index c4d8e52ac4..770b7e04fc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -56,6 +56,7 @@ import java.util.Queue;
 import java.util.Set;
 import java.util.Stack;
 import java.util.concurrent.ExecutionException;
+import javax.annotation.Nullable;
 
 /**
  * Provides access to features supported by a specific toolchain.
@@ -842,6 +843,10 @@ public class CcToolchainFeatures implements Serializable {
           .collect(ImmutableList.toImmutableList());
     }
 
+    public Variables getParent() {
+      return parent;
+    }
+
     /**
      * Value of a build variable exposed to the CROSSTOOL used for flag expansion.
      *
@@ -1351,12 +1356,14 @@ public class CcToolchainFeatures implements Serializable {
     public static class Builder {
       private final Map<String, VariableValue> variablesMap = new LinkedHashMap<>();
       private final Map<String, String> stringVariablesMap = new LinkedHashMap<>();
+      private final Variables parent;
 
-      public Builder() {};
+      public Builder() {
+        parent = null;
+      }
 
-      public Builder(Variables variables) {
-        variablesMap.putAll(variables.variablesMap);
-        stringVariablesMap.putAll(variables.stringVariablesMap);
+      public Builder(@Nullable Variables parent) {
+        this.parent = parent;
       }
 
       /** Add an integer variable that expands {@code name} to {@code value}. */
@@ -1471,8 +1478,11 @@ public class CcToolchainFeatures implements Serializable {
             !stringVariablesMap.containsKey(name), "Cannot overwrite variable '%s'", name);
       }
 
-      /** Adds all variables to this builder. Note: cannot override already added variables. */
-      public Builder addAll(Variables variables) {
+      /**
+       * Adds all variables to this builder. Cannot override already added variables. Does not add
+       * variables defined in the {@code parent} variables.
+       */
+      public Builder addAllNonTransitive(Variables variables) {
         SetView<String> intersection =
             Sets.intersection(variables.variablesMap.keySet(), variablesMap.keySet());
         SetView<String> stringIntersection =
@@ -1489,7 +1499,7 @@ public class CcToolchainFeatures implements Serializable {
 
       /**
        * Add all variables to this builder, possibly overriding variables already present in the
-       * builder. Use cautiously, prefer {@code addAll} if possible.
+       * builder. Use cautiously, prefer {@code addAllNonTransitive} if possible.
        * TODO(b/32893861) Clean 'module_files' to be registered only once and remove this method.
        */
       Builder addAndOverwriteAll(Variables overwrittenVariables) {
@@ -1501,7 +1511,7 @@ public class CcToolchainFeatures implements Serializable {
       /** @return a new {@Variables} object. */
       public Variables build() {
         return new Variables(
-            ImmutableMap.copyOf(variablesMap), ImmutableMap.copyOf(stringVariablesMap));
+            parent, ImmutableMap.copyOf(variablesMap), ImmutableMap.copyOf(stringVariablesMap));
       }
     }
     
@@ -1518,11 +1528,12 @@ public class CcToolchainFeatures implements Serializable {
     private final Variables parent;
 
     private Variables(
+        Variables parent,
         ImmutableMap<String, VariableValue> variablesMap,
         ImmutableMap<String, String> stringVariablesMap) {
       this.variablesMap = variablesMap;
       this.stringVariablesMap = stringVariablesMap;
-      this.parent = null;
+      this.parent = parent;
     }
 
     /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
index b79164bbeb..8fc5075c46 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
@@ -23,12 +23,12 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.Map;
 import javax.annotation.Nullable;
 
 /** Information about a C++ compiler used by the <code>cc_*</code> rules. */
@@ -59,7 +59,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
           CppCompilationContext.EMPTY,
           false,
           false,
-          ImmutableMap.<String, String>of(),
+          Variables.EMPTY,
           ImmutableList.<Artifact>of(),
           NestedSetBuilder.<Pair<String, String>>emptySet(Order.COMPILE_ORDER),
           null,
@@ -87,7 +87,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
   private final CppCompilationContext cppCompilationContext;
   private final boolean supportsParamFiles;
   private final boolean supportsHeaderParsing;
-  private final ImmutableMap<String, String> buildVariables;
+  private final Variables buildVariables;
   private final ImmutableList<Artifact> builtinIncludeFiles;
   private final NestedSet<Pair<String, String>> coverageEnvironment;
   @Nullable private final Artifact linkDynamicLibraryTool;
@@ -116,7 +116,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
       CppCompilationContext cppCompilationContext,
       boolean supportsParamFiles,
       boolean supportsHeaderParsing,
-      Map<String, String> buildVariables,
+      Variables buildVariables,
       ImmutableList<Artifact> builtinIncludeFiles,
       NestedSet<Pair<String, String>> coverageEnvironment,
       Artifact linkDynamicLibraryTool,
@@ -144,7 +144,7 @@ public final class CcToolchainProvider extends ToolchainInfo {
     this.cppCompilationContext = Preconditions.checkNotNull(cppCompilationContext);
     this.supportsParamFiles = supportsParamFiles;
     this.supportsHeaderParsing = supportsHeaderParsing;
-    this.buildVariables = ImmutableMap.copyOf(buildVariables);
+    this.buildVariables = buildVariables;
     this.builtinIncludeFiles = builtinIncludeFiles;
     this.coverageEnvironment = coverageEnvironment;
     this.linkDynamicLibraryTool = linkDynamicLibraryTool;
@@ -298,11 +298,9 @@ public final class CcToolchainProvider extends ToolchainInfo {
   public CppConfiguration getCppConfiguration() {
     return cppConfiguration;
   }
-  
-  /**
-   * Returns build variables to be templated into the crosstool.
-   */
-  public ImmutableMap<String, String> getBuildVariables() {
+
+  /** Returns build variables to be templated into the crosstool. */
+  public Variables getBuildVariables() {
     return buildVariables;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
index 3215cbb36e..472701ceec 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
@@ -108,9 +108,8 @@ public final class CompileCommandLine {
     CcToolchainFeatures.Variables updatedVariables = variables;
     if (variables != null && overwrittenVariables != null) {
       CcToolchainFeatures.Variables.Builder variablesBuilder =
-          new CcToolchainFeatures.Variables.Builder();
-      variablesBuilder.addAll(variables);
-      variablesBuilder.addAndOverwriteAll(overwrittenVariables);
+          new CcToolchainFeatures.Variables.Builder(variables);
+      variablesBuilder.addAllNonTransitive(overwrittenVariables);
       updatedVariables = variablesBuilder.build();
     }
     addFilteredOptions(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index d244df8460..7c48a9ff3a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -644,7 +644,7 @@ public class CppHelper {
         Preconditions.checkNotNull(
             featureConfiguration.getToolForAction(CppCompileAction.STRIP_ACTION_NAME));
     Variables variables =
-        new Variables.Builder()
+        new Variables.Builder(toolchain.getBuildVariables())
             .addStringVariable(CppModel.OUTPUT_FILE_VARIABLE_NAME, output.getExecPathString())
             .addStringSequenceVariable(
                 CppModel.STRIPOPTS_VARIABLE_NAME, cppConfiguration.getStripOpts())
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index 7a21660d3c..194bb00484 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -716,7 +716,7 @@ public class CppLinkActionBuilder {
             : null;
 
     // Add build variables necessary to template link args into the crosstool.
-    Variables.Builder buildVariablesBuilder = new Variables.Builder();
+    Variables.Builder buildVariablesBuilder = new Variables.Builder(toolchain.getBuildVariables());
     CppLinkVariablesExtension variablesExtension =
         isLtoIndexing
             ? new CppLinkVariablesExtension(
@@ -1450,11 +1450,6 @@ public class CppLinkActionBuilder {
         buildVariables.addStringVariable(FORCE_PIC_VARIABLE, "");
       }
 
-      if (toolchain.getSysroot() != null) {
-        buildVariables.addStringVariable(
-            CppModel.SYSROOT_VARIABLE_NAME, toolchain.getSysroot().getPathString());
-      }
-
       if (cppConfiguration.shouldStripBinaries()) {
         buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS_VARIABLE, "");
       }
@@ -1551,10 +1546,6 @@ public class CppLinkActionBuilder {
         buildVariables.addStringVariable(DEF_FILE_PATH_VARIABLE, defFile.getExecPathString());
       }
 
-      // Variables arising from the toolchain
-      buildVariables
-          .addAllStringVariables(toolchain.getBuildVariables())
-          .build();
       fdoSupport.getFdoSupport().getLinkOptions(featureConfiguration, buildVariables);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index b4c12ca346..10b3aea37a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -527,7 +527,7 @@ public final class CppModel {
       CppModuleMap cppModuleMap,
       Map<String, String> sourceSpecificBuildVariables) {
     CcToolchainFeatures.Variables.Builder buildVariables =
-        new CcToolchainFeatures.Variables.Builder();
+        new CcToolchainFeatures.Variables.Builder(ccToolchain.getBuildVariables());
 
     CppCompilationContext builderContext = builder.getContext();
     Artifact sourceFile = builder.getSourceFile();
@@ -651,13 +651,6 @@ public final class CppModel {
           LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME, ltoIndexingFile.getExecPathString());
     }
 
-    if (ccToolchain.getSysroot() != null) {
-      buildVariables.addStringVariable(
-          SYSROOT_VARIABLE_NAME, ccToolchain.getSysroot().getPathString());
-    }
-
-    buildVariables.addAllStringVariables(ccToolchain.getBuildVariables());
-
     buildVariables.addAllStringVariables(sourceSpecificBuildVariables);
 
     for (VariablesExtension extension : variablesExtensions) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
index 4d4a6c9286..c171c9fe01 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
@@ -397,8 +397,7 @@ public final class LinkCommandLine extends CommandLine {
     argv.addAll(
         featureConfiguration.getCommandLine(
             actionName,
-            new Variables.Builder()
-                .addAll(variables)
+            new Variables.Builder(variables)
                 .addStringSequenceVariable(
                     CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE, getToolchainFlags())
                 .build()));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java
index e5f84c59c7..3a09d3ab60 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendArtifacts.java
@@ -133,7 +133,8 @@ public final class LtoBackendArtifacts {
     PathFragment compiler = cppConfiguration.getCppExecutable();
 
     builder.setExecutable(compiler);
-    Variables.Builder buildVariablesBuilder = new Variables.Builder();
+    Variables.Builder buildVariablesBuilder =
+        new Variables.Builder(ccToolchain.getBuildVariables());
     buildVariablesBuilder.addStringVariable("thinlto_index", index.getExecPath().toString());
     // The output from the LTO backend step is a native object file.
     buildVariablesBuilder.addStringVariable(