diff options
author | 2017-09-19 15:42:58 +0200 | |
---|---|---|
committer | 2017-09-19 17:17:56 +0200 | |
commit | 6434af053cfffa38909bc86afe69e33b573b6179 (patch) | |
tree | cb6ca52ddada1f718089d4ff2e21d5d3eb79eee3 /src/main/java | |
parent | e85b5a7a7c4968ff3451573b4b8ca2a52a3e9961 (diff) |
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
Diffstat (limited to 'src/main/java')
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( |