diff options
Diffstat (limited to 'src/main')
10 files changed, 317 insertions, 133 deletions
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 a26585e614..c4c49af695 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 @@ -682,6 +682,25 @@ public final class CcCommon { } } requestedFeatures.addAll(ruleSpecificRequestedFeatures); + + // Enable FDO related features requested by options. + CppConfiguration cppConfiguration = + ruleContext.getConfiguration().getFragment(CppConfiguration.class); + FdoSupport fdoSupport = cppConfiguration.getFdoSupport(); + if (fdoSupport.getFdoInstrument() != null) { + requestedFeatures.add(CppRuleClasses.FDO_INSTRUMENT); + } + if (fdoSupport.getFdoOptimizeProfile() != null + && !fdoSupport.isAutoFdoEnabled()) { + requestedFeatures.add(CppRuleClasses.FDO_OPTIMIZE); + } + if (fdoSupport.isAutoFdoEnabled()) { + requestedFeatures.add(CppRuleClasses.AUTOFDO); + } + if (cppConfiguration.isLipoOptimizationOrInstrumentation()) { + requestedFeatures.add(CppRuleClasses.LIPO); + } + FeatureConfiguration configuration = toolchain.getFeatures().getFeatureConfiguration(requestedFeatures.build()); for (String feature : unsupportedFeatures) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java index b9db265ba3..4a0fc051e1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java @@ -70,7 +70,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -203,6 +202,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable // will be provided by 'featureConfiguration'. ImmutableList<String> features, FeatureConfiguration featureConfiguration, + CcToolchainFeatures.Variables variables, Artifact sourceFile, Label sourceLabel, NestedSet<Artifact> mandatoryInputs, @@ -244,8 +244,8 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable inputsKnown = true; } this.cppCompileCommandLine = new CppCompileCommandLine(sourceFile, dotdFile, - context.getCppModuleMap(), copts, coptsFilter, pluginOpts, - (gcnoFile != null), features, featureConfiguration, fdoBuildStamp); + copts, coptsFilter, pluginOpts, (gcnoFile != null), features, + featureConfiguration, variables, fdoBuildStamp); this.actionContext = actionContext; this.lipoScannables = lipoScannables; this.actionClassId = actionClassId; @@ -1166,31 +1166,32 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable public final class CppCompileCommandLine { private final Artifact sourceFile; private final DotdFile dotdFile; - private final CppModuleMap cppModuleMap; private final List<String> copts; private final Predicate<String> coptsFilter; private final List<String> pluginOpts; private final boolean isInstrumented; private final Collection<String> features; private final FeatureConfiguration featureConfiguration; + private final CcToolchainFeatures.Variables variables; // The value of the BUILD_FDO_TYPE macro to be defined on command line @Nullable private final String fdoBuildStamp; - public CppCompileCommandLine(Artifact sourceFile, DotdFile dotdFile, CppModuleMap cppModuleMap, + public CppCompileCommandLine(Artifact sourceFile, DotdFile dotdFile, ImmutableList<String> copts, Predicate<String> coptsFilter, ImmutableList<String> pluginOpts, boolean isInstrumented, Collection<String> features, FeatureConfiguration featureConfiguration, + CcToolchainFeatures.Variables variables, @Nullable String fdoBuildStamp) { this.sourceFile = Preconditions.checkNotNull(sourceFile); this.dotdFile = Preconditions.checkNotNull(dotdFile); - this.cppModuleMap = cppModuleMap; this.copts = Preconditions.checkNotNull(copts); this.coptsFilter = coptsFilter; this.pluginOpts = Preconditions.checkNotNull(pluginOpts); this.isInstrumented = isInstrumented; this.features = Preconditions.checkNotNull(features); this.featureConfiguration = featureConfiguration; + this.variables = variables; this.fdoBuildStamp = fdoBuildStamp; } @@ -1264,9 +1265,6 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable addFilteredOptions(options, toolchain.getCxxOptions(features)); } - // Users don't expect the explicit copts to be filtered by coptsFilter, add them verbatim. - options.addAll(copts); - for (String warn : cppConfiguration.getCWarns()) { options.add("-W" + warn); } @@ -1279,35 +1277,21 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable options.add("-D" + CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\""); } - CcToolchainFeatures.Variables.Builder buildVariables = - new CcToolchainFeatures.Variables.Builder(); - if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS) && cppModuleMap != null) { - // If the feature is enabled and cppModuleMap is null, we are about to fail during analysis - // in any case, but don't crash. - buildVariables.addVariable("module_name", cppModuleMap.getName()); - buildVariables.addVariable("module_map_file", - cppModuleMap.getArtifact().getExecPathString()); - } - if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) { - buildVariables.addSequenceVariable("module_files", getHeaderModulePaths()); - } - if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) { - buildVariables.addSequenceVariable("include_paths", - getSafePathStrings(context.getIncludeDirs())); - buildVariables.addSequenceVariable("quote_include_paths", - getSafePathStrings(context.getQuoteIncludeDirs())); - buildVariables.addSequenceVariable("system_include_paths", - getSafePathStrings(context.getSystemIncludeDirs())); - } // TODO(bazel-team): This needs to be before adding getUnfilteredCompilerOptions() and after // adding the warning flags until all toolchains are migrated; currently toolchains use the // unfiltered compiler options to inject include paths, which is superseded by the feature // configuration; on the other hand toolchains switch off warnings for the layering check // that will be re-added by the feature flags. - options.addAll(featureConfiguration.getCommandLine(getActionName(), buildVariables.build())); + addFilteredOptions(options, + featureConfiguration.getCommandLine(getActionName(), variables)); options.addAll(toolchain.getUnfilteredCompilerOptions(features)); + // Users don't expect the explicit copts to be filtered by coptsFilter, add them verbatim. + // Make sure these are added after the options from the feature configuration, so that + // those options can be overriden. + options.addAll(copts); + // GCC gives randomized names to symbols which are defined in // an anonymous namespace but have external linkage. To make // computation of these deterministic, we want to override the @@ -1359,42 +1343,6 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable return options; } - /** - * Get the safe path strings for a list of paths to use in the build variables. - */ - private Collection<String> getSafePathStrings(Collection<PathFragment> paths) { - ImmutableSet.Builder<String> result = ImmutableSet.builder(); - for (PathFragment path : paths) { - result.add(path.getSafePathString()); - } - return result.build(); - } - - /** - * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-pic - * mode. - */ - private Collection<String> getHeaderModulePaths() { - Collection<String> result = new LinkedHashSet<>(); - NestedSet<Artifact> artifacts = featureConfiguration.isEnabled( - CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES) - ? context.getTopLevelHeaderModules() - : context.getAdditionalInputs(); - for (Artifact artifact : artifacts) { - String filename = artifact.getFilename(); - if (!filename.endsWith(".pcm")) { - continue; - } - // Depending on whether this specific compile action is pic or non-pic, select the - // corresponding header modules. Note that the compilation context might give us both - // from targets that are built in both modes. - if (usePic == filename.endsWith(".pic.pcm")) { - result.add(artifact.getExecPathString()); - } - } - return result; - } - // For each option in 'in', add it to 'out' unless it is matched by the 'coptsFilter' regexp. private void addFilteredOptions(List<String> out, List<String> in) { Iterables.addAll(out, Iterables.filter(in, coptsFilter)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java index 216acb8ed6..3fdda79b5d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java @@ -54,6 +54,7 @@ public class CppCompileActionBuilder { private final ActionOwner owner; private final List<String> features = new ArrayList<>(); private CcToolchainFeatures.FeatureConfiguration featureConfiguration; + private CcToolchainFeatures.Variables variables; private final Artifact sourceFile; private final Label sourceLabel; private final NestedSetBuilder<Artifact> mandatoryInputsBuilder; @@ -261,15 +262,15 @@ public class CppCompileActionBuilder { // Copying the collections is needed to make the builder reusable. if (fake) { return new FakeCppCompileAction(owner, ImmutableList.copyOf(features), featureConfiguration, - sourceFile, sourceLabel, realMandatoryInputsBuilder.build(), outputFile, tempOutputFile, - dotdFile, configuration, cppConfiguration, context, actionContext, + variables, sourceFile, sourceLabel, realMandatoryInputsBuilder.build(), outputFile, + tempOutputFile, dotdFile, configuration, cppConfiguration, context, actionContext, ImmutableList.copyOf(copts), ImmutableList.copyOf(pluginOpts), getNocoptPredicate(nocopts), extraSystemIncludePrefixes, fdoBuildStamp, ruleContext); } else { NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build(); return new CppCompileAction(owner, ImmutableList.copyOf(features), featureConfiguration, - sourceFile, sourceLabel, realMandatoryInputs, outputFile, dotdFile, + variables, sourceFile, sourceLabel, realMandatoryInputs, outputFile, dotdFile, gcnoFile, getDwoFile(outputFile, analysisEnvironment, cppConfiguration), optionalSourceFile, configuration, cppConfiguration, context, actionContext, ImmutableList.copyOf(copts), @@ -289,6 +290,14 @@ public class CppCompileActionBuilder { this.featureConfiguration = featureConfiguration; return this; } + + /** + * Sets the feature build variables to be used for the action. + */ + public CppCompileActionBuilder setVariables(CcToolchainFeatures.Variables variables) { + this.variables = variables; + return this; + } public CppCompileActionBuilder setIncludeResolver(IncludeResolver includeResolver) { this.includeResolver = includeResolver; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java index 398c276838..af342ea241 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java @@ -680,6 +680,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return result; } + // TODO(bazel-team): Remove this once bazel supports all crosstool flags through + // feature configuration, and all crosstools have been converted. private CToolchain addLegacyFeatures(CToolchain toolchain) { CToolchain.Builder toolchainBuilder = CToolchain.newBuilder(); ImmutableSet.Builder<String> featuresBuilder = ImmutableSet.builder(); @@ -833,6 +835,68 @@ public class CppConfiguration extends BuildConfiguration.Fragment { + "}", toolchainBuilder); } + if (!features.contains("fdo_instrument")) { + TextFormat.merge("" + + "feature {" + + " name: 'fdo_instrument'" + + " flag_set {" + + " action: 'c-compile'" + + " action: 'c++-compile'" + + " action: 'c++-link'" + + " flag_group {" + + " flag: '-fprofile-generate=%{fdo_instrument_path}'" + + " }" + + " flag_group {" + + " flag: '-fno-data-sections'" + + " }" + + " }" + + "}", + toolchainBuilder); + } + if (!features.contains("fdo_optimize")) { + TextFormat.merge("" + + "feature {" + + " name: 'fdo_optimize'" + + " flag_set {" + + " action: 'c-compile'" + + " action: 'c++-compile'" + + " flag_group {" + + " flag: '-fprofile-use=%{fdo_profile_path}'" + + " flag: '-fprofile-correction'" + + " }" + + " }" + + "}", + toolchainBuilder); + } + if (!features.contains("autofdo")) { + TextFormat.merge("" + + "feature {" + + " name: 'autofdo'" + + " flag_set {" + + " action: 'c-compile'" + + " action: 'c++-compile'" + + " flag_group {" + + " flag: '-fauto-profile=%{fdo_profile_path}'" + + " flag: '-fprofile-correction'" + + " }" + + " }" + + "}", + toolchainBuilder); + } + if (!features.contains("lipo")) { + TextFormat.merge("" + + "feature {" + + " name: 'lipo'" + + " flag_set {" + + " action: 'c-compile'" + + " action: 'c++-compile'" + + " flag_group {" + + " flag: '-fripa'" + + " }" + + " }" + + "}", + toolchainBuilder); + } } catch (ParseException e) { // Can only happen if we change the proto definition without changing our configuration above. throw new RuntimeException(e); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java index 46988c91bb..b6e9f24407 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java @@ -445,7 +445,8 @@ public final class CppLinkAction extends AbstractAction { */ public static class Builder { // Builder-only - private final RuleContext ruleContext; + // Null when invoked from tests (e.g. via createTestBuilder). + @Nullable private final RuleContext ruleContext; private final AnalysisEnvironment analysisEnvironment; private final PathFragment outputPath; @@ -508,7 +509,7 @@ public final class CppLinkAction extends AbstractAction { * @param configuration the configuration used to determine the tool chain * and the default link options */ - private Builder(RuleContext ruleContext, PathFragment outputPath, + private Builder(@Nullable RuleContext ruleContext, PathFragment outputPath, BuildConfiguration configuration, AnalysisEnvironment analysisEnvironment, CcToolchainProvider toolchain) { this.ruleContext = ruleContext; @@ -642,7 +643,7 @@ public final class CppLinkAction extends AbstractAction { : null; LinkCommandLine linkCommandLine = - new LinkCommandLine.Builder(configuration, getOwner()) + new LinkCommandLine.Builder(configuration, getOwner(), ruleContext) .setOutput(outputLibrary.getArtifact()) .setInterfaceOutput(interfaceOutput) .setSymbolCountsOutput(symbolCountOutput) 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 08683fd4e0..4ea8355e8d 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 @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; @@ -23,6 +24,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.rules.cpp.CcCompilationOutputs.Builder; import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; @@ -37,6 +39,7 @@ import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; import java.util.Collection; +import java.util.LinkedHashSet; import java.util.List; import java.util.regex.Pattern; @@ -290,6 +293,77 @@ public final class CppModel { } /** + * Get the safe path strings for a list of paths to use in the build variables. + */ + private Collection<String> getSafePathStrings(Collection<PathFragment> paths) { + ImmutableSet.Builder<String> result = ImmutableSet.builder(); + for (PathFragment path : paths) { + result.add(path.getSafePathString()); + } + return result.build(); + } + + /** + * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-pic + * mode. + */ + private Collection<String> getHeaderModulePaths(CppCompileActionBuilder builder, + boolean usePic) { + Collection<String> result = new LinkedHashSet<>(); + NestedSet<Artifact> artifacts = featureConfiguration.isEnabled( + CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES) + ? builder.getContext().getTopLevelHeaderModules() + : builder.getContext().getAdditionalInputs(); + for (Artifact artifact : artifacts) { + String filename = artifact.getFilename(); + if (!filename.endsWith(".pcm")) { + continue; + } + // Depending on whether this specific compile action is pic or non-pic, select the + // corresponding header modules. Note that the compilation context might give us both + // from targets that are built in both modes. + if (usePic == filename.endsWith(".pic.pcm")) { + result.add(artifact.getExecPathString()); + } + } + return result; + } + + private void setupBuildVariables(CppCompileActionBuilder builder, + boolean usePic, PathFragment ccRelativeName) { + CcToolchainFeatures.Variables.Builder buildVariables = + new CcToolchainFeatures.Variables.Builder(); + + CppModuleMap cppModuleMap = context.getCppModuleMap(); + if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS) && cppModuleMap != null) { + // If the feature is enabled and cppModuleMap is null, we are about to fail during analysis + // in any case, but don't crash. + buildVariables.addVariable("module_name", cppModuleMap.getName()); + buildVariables.addVariable("module_map_file", + cppModuleMap.getArtifact().getExecPathString()); + } + if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) { + buildVariables.addSequenceVariable("module_files", getHeaderModulePaths(builder, usePic)); + } + if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) { + buildVariables.addSequenceVariable("include_paths", + getSafePathStrings(context.getIncludeDirs())); + buildVariables.addSequenceVariable("quote_include_paths", + getSafePathStrings(context.getQuoteIncludeDirs())); + buildVariables.addSequenceVariable("system_include_paths", + getSafePathStrings(context.getSystemIncludeDirs())); + } + + if (ccRelativeName != null) { + cppConfiguration.getFdoSupport().configureCompilation(builder, buildVariables, ruleContext, + ccRelativeName, usePic, featureConfiguration, cppConfiguration); + } + + CcToolchainFeatures.Variables variables = buildVariables.build(); + builder.setVariables(variables); + } + + /** * Constructs the C++ compiler actions. It generally creates one action for every specified source * file. It takes into account LIPO, fake-ness, coverage, and PIC, in addition to using the * settings specified on the current object. This method should only be called once. @@ -338,6 +412,7 @@ public final class CppModel { .setDotdFile(outputName, ".h.d") // If we generate pic actions, we prefer the header actions to use the pic artifacts. .setPicMode(this.getGeneratePicActions()); + setupBuildVariables(builder, this.getGeneratePicActions(), null); semantics.finalizeCompileActionBuilder(ruleContext, builder); CppCompileAction compileAction = builder.build(); env.registerAction(compileAction); @@ -354,12 +429,11 @@ public final class CppModel { String dependencyFileExtension, boolean addObject) { PathFragment ccRelativeName = semantics.getEffectiveSourcePath(sourceArtifact); - LipoContextProvider lipoProvider = null; if (cppConfiguration.isLipoOptimization()) { // TODO(bazel-team): we shouldn't be needing this, merging context with the binary // is a superset of necessary information. - lipoProvider = Preconditions.checkNotNull(CppHelper.getLipoContextProvider(ruleContext), - outputName); + LipoContextProvider lipoProvider = + Preconditions.checkNotNull(CppHelper.getLipoContextProvider(ruleContext), outputName); builder.setContext(CppCompilationContext.mergeForLipo(lipoProvider.getLipoContext(), context)); } @@ -375,6 +449,7 @@ public final class CppModel { .setOutputFile(outputFile) .setDotdFile(outputName, dependencyFileExtension) .setTempOutputFile(tempOutputName); + setupBuildVariables(builder, getGeneratePicActions(), ccRelativeName); semantics.finalizeCompileActionBuilder(ruleContext, builder); CppCompileAction action = builder.build(); env.registerAction(action); @@ -392,13 +467,12 @@ public final class CppModel { if (generatePicAction) { CppCompileActionBuilder picBuilder = copyAsPicBuilder(builder, outputName, outputExtension, dependencyFileExtension); - cppConfiguration.getFdoSupport().configureCompilation(picBuilder, ruleContext, env, - ruleContext.getLabel(), ccRelativeName, nocopts, /*usePic=*/true, - lipoProvider); + setupBuildVariables(picBuilder, /*usePic=*/true, ccRelativeName); if (maySaveTemps) { result.addTemps( - createTempsActions(sourceArtifact, outputName, picBuilder, /*usePic=*/true)); + createTempsActions(sourceArtifact, outputName, picBuilder, /*usePic=*/true, + ccRelativeName)); } if (isCodeCoverageEnabled()) { @@ -425,13 +499,12 @@ public final class CppModel { .setOutputFile(ruleContext.getRelatedArtifact(outputName, outputExtension)) .setDotdFile(outputName, dependencyFileExtension); // Create non-PIC compile actions - cppConfiguration.getFdoSupport().configureCompilation(builder, ruleContext, env, - ruleContext.getLabel(), ccRelativeName, nocopts, /*usePic=*/false, - lipoProvider); + setupBuildVariables(builder, /*usePic=*/false, ccRelativeName); if (maySaveTemps) { result.addTemps( - createTempsActions(sourceArtifact, outputName, builder, /*usePic=*/false)); + createTempsActions(sourceArtifact, outputName, builder, /*usePic=*/false, + ccRelativeName)); } if (!cppConfiguration.isLipoOptimization() && isCodeCoverageEnabled()) { @@ -630,7 +703,7 @@ public final class CppModel { * Create the actions for "--save_temps". */ private ImmutableList<Artifact> createTempsActions(Artifact source, PathFragment outputName, - CppCompileActionBuilder builder, boolean usePic) { + CppCompileActionBuilder builder, boolean usePic, PathFragment ccRelativeName) { if (!cppConfiguration.getSaveTemps()) { return ImmutableList.of(); } @@ -646,7 +719,9 @@ public final class CppModel { String iExt = isCFile ? ".i" : ".ii"; String picExt = usePic ? ".pic" : ""; CppCompileActionBuilder dBuilder = new CppCompileActionBuilder(builder); + setupBuildVariables(dBuilder, usePic, ccRelativeName); CppCompileActionBuilder sdBuilder = new CppCompileActionBuilder(builder); + setupBuildVariables(sdBuilder, usePic, ccRelativeName); dBuilder .setOutputFile(ruleContext.getRelatedArtifact(outputName, picExt + iExt)) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index c6b3a216d6..98a6568a9f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java @@ -145,4 +145,24 @@ public class CppRuleClasses { * A string constant for the include_paths feature. */ public static final String INCLUDE_PATHS = "include_paths"; + + /** + * A string constant for the fdo_instrument feature. + */ + public static final String FDO_INSTRUMENT = "fdo_instrument"; + + /** + * A string constant for the fdo_optimize feature. + */ + public static final String FDO_OPTIMIZE = "fdo_optimize"; + + /** + * A string constant for the autofdo feature. + */ + public static final String AUTOFDO = "autofdo"; + + /** + * A string constant for the lipo feature. + */ + public static final String LIPO = "lipo"; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java index 994d93ad35..a7ed3da2df 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java @@ -62,6 +62,7 @@ public class FakeCppCompileAction extends CppCompileAction { FakeCppCompileAction(ActionOwner owner, ImmutableList<String> features, FeatureConfiguration featureConfiguration, + CcToolchainFeatures.Variables variables, Artifact sourceFile, Label sourceLabel, NestedSet<Artifact> mandatoryInputs, @@ -78,8 +79,8 @@ public class FakeCppCompileAction extends CppCompileAction { ImmutableList<PathFragment> extraSystemIncludePrefixes, @Nullable String fdoBuildStamp, RuleContext ruleContext) { - super(owner, features, featureConfiguration, sourceFile, sourceLabel, mandatoryInputs, - outputFile, dotdFile, null, null, null, + super(owner, features, featureConfiguration, variables, sourceFile, sourceLabel, + mandatoryInputs, outputFile, dotdFile, null, null, null, configuration, cppConfiguration, // We only allow inclusion of header files explicitly declared in // "srcs", so we only use declaredIncludeSrcs, not declaredIncludeDirs. diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java index c10eea9edc..6474d7f01d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupport.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules.cpp; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.base.Predicate; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMultimap; @@ -31,6 +30,7 @@ import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadHostile; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.skyframe.FileValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.syntax.Label; @@ -44,10 +44,8 @@ import com.google.devtools.build.skyframe.SkyFunction; import java.io.IOException; import java.io.Serializable; -import java.util.ArrayList; import java.util.Collection; -import java.util.List; -import java.util.regex.Pattern; +import java.util.HashSet; import java.util.zip.ZipException; /** @@ -440,16 +438,18 @@ public class FdoSupport implements Serializable { } /** - * Configures a compile action builder by adding command line options and + * Configures a compile action builder by setting up command line options and * auxiliary inputs according to the FDO configuration. This method does * nothing If FDO is disabled. */ @ThreadSafe - public void configureCompilation(CppCompileActionBuilder builder, RuleContext ruleContext, - AnalysisEnvironment env, Label lipoLabel, PathFragment sourceName, final Pattern nocopts, - boolean usePic, LipoContextProvider lipoInputProvider) { + public void configureCompilation(CppCompileActionBuilder builder, + CcToolchainFeatures.Variables.Builder buildVariables, RuleContext ruleContext, + PathFragment sourceName, boolean usePic, FeatureConfiguration featureConfiguration, + CppConfiguration cppConfiguration) { // It is a bug if this method is called with useLipo if lipo is disabled. However, it is legal // if is is called with !useLipo, even though lipo is enabled. + LipoContextProvider lipoInputProvider = CppHelper.getLipoContextProvider(ruleContext); Preconditions.checkArgument(lipoInputProvider == null || isLipoEnabled()); // FDO is disabled -> do nothing. @@ -457,55 +457,62 @@ public class FdoSupport implements Serializable { return; } - List<String> fdoCopts = new ArrayList<>(); - // Instrumentation phase - if (fdoInstrument != null) { - fdoCopts.add("-fprofile-generate=" + fdoInstrument.getPathString()); - fdoCopts.add("-fno-data-sections"); - if (lipoMode != LipoMode.OFF) { - fdoCopts.add("-fripa"); - } + if (featureConfiguration.isEnabled(CppRuleClasses.FDO_INSTRUMENT)) { + buildVariables.addVariable("fdo_instrument_path", + fdoInstrument.getPathString()); } // Optimization phase if (fdoRoot != null) { + AnalysisEnvironment env = ruleContext.getAnalysisEnvironment(); // Declare dependency on contents of zip file. if (env.getSkyframeEnv().valuesMissing()) { return; } Iterable<Artifact> auxiliaryInputs = getAuxiliaryInputs( - ruleContext, env, lipoLabel, sourceName, usePic, lipoInputProvider); + ruleContext, env, sourceName, usePic, lipoInputProvider); builder.addMandatoryInputs(auxiliaryInputs); if (!Iterables.isEmpty(auxiliaryInputs)) { - if (useAutoFdo) { - fdoCopts.add("-fauto-profile=" + getAutoProfilePath().getPathString()); - } else { - fdoCopts.add("-fprofile-use=" + fdoRootExecPath); + if (featureConfiguration.isEnabled(CppRuleClasses.AUTOFDO)) { + buildVariables.addVariable("fdo_profile_path", + getAutoProfilePath().getPathString()); } - fdoCopts.add("-fprofile-correction"); - if (lipoInputProvider != null) { - fdoCopts.add("-fripa"); + if (featureConfiguration.isEnabled(CppRuleClasses.FDO_OPTIMIZE)) { + buildVariables.addVariable("fdo_profile_path", + fdoRootExecPath.getPathString()); } - } - } - Iterable<String> filteredCopts = fdoCopts; - if (nocopts != null) { - // Filter fdoCopts with nocopts if they exist. - filteredCopts = Iterables.filter(fdoCopts, new Predicate<String>() { - @Override - public boolean apply(String copt) { - return !nocopts.matcher(copt).matches(); + } else { + // TODO(bazel-team): Remove this workaround once the feature configuration + // supports per-file feature enabling. + // The feature configuration was created based on blaze options, which + // enabled the fdo optimize options since the fdoRoot was set. + // In this case the source file doesn't have an associated profile, + // so we need to disable these features so that we don't add the FDO options. + // However, the list of features is immutable and set on the CppModel. + // Create a new feature config here, enabling just what we want, + // and set it in this builder. + Collection<String> featureNames = cppConfiguration.getFeatures().getFeatureNames(); + Collection<String> newFeatureNames = new HashSet<>(); + for (String name : featureNames) { + if (featureConfiguration.isEnabled(name)) { + newFeatureNames.add(name); + } } - }); + newFeatureNames.remove(CppRuleClasses.FDO_OPTIMIZE); + newFeatureNames.remove(CppRuleClasses.AUTOFDO); + newFeatureNames.remove(CppRuleClasses.LIPO); + FeatureConfiguration newFeatureConfiguration = + cppConfiguration.getFeatures().getFeatureConfiguration(newFeatureNames); + builder.setFeatureConfiguration(newFeatureConfiguration); + } } - builder.addCopts(0, filteredCopts); } /** * Returns the auxiliary files that need to be added to the {@link CppCompileAction}. */ private Iterable<Artifact> getAuxiliaryInputs( - RuleContext ruleContext, AnalysisEnvironment env, Label lipoLabel, PathFragment sourceName, + RuleContext ruleContext, AnalysisEnvironment env, PathFragment sourceName, boolean usePic, LipoContextProvider lipoContextProvider) { // If --fdo_optimize was not specified, we don't have any additional inputs. if (fdoProfile == null) { @@ -527,6 +534,7 @@ public class FdoSupport implements Serializable { PathFragment objectName = FileSystemUtils.replaceExtension(sourceName, usePic ? ".pic.o" : ".o"); + Label lipoLabel = ruleContext.getLabel(); auxiliaryInputs.addAll( getGcdaArtifactsForObjectFileName(ruleContext, env, objectName, lipoLabel)); @@ -634,14 +642,17 @@ public class FdoSupport implements Serializable { } /** - * Returns an immutable list of command line arguments to add to the linker - * command line. If FDO is disabled, and empty list is returned. + * Adds the FDO profile output path to the variable builder. + * If FDO is disabled, no build variable is added. */ @ThreadSafe - public ImmutableList<String> getLinkOptions() { - return fdoInstrument != null - ? ImmutableList.of("-fprofile-generate=" + fdoInstrument.getPathString()) - : ImmutableList.<String>of(); + public void getLinkOptions(FeatureConfiguration featureConfiguration, + CcToolchainFeatures.Variables.Builder buildVariables + ) { + if (featureConfiguration.isEnabled(CppRuleClasses.FDO_INSTRUMENT)) { + buildVariables.addVariable("fdo_instrument_path", + fdoInstrument.getPathString()); + } } /** 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 e597907015..66b2722e83 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 @@ -31,6 +31,7 @@ import com.google.devtools.build.lib.analysis.actions.CommandLine; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.collect.CollectionUtils; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness; import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType; import com.google.devtools.build.lib.syntax.Label; @@ -58,6 +59,9 @@ public final class LinkCommandLine extends CommandLine { private final BuildConfiguration configuration; private final CppConfiguration cppConfiguration; private final ActionOwner owner; + private final CcToolchainFeatures.Variables variables; + // The feature config can be null for tests. + @Nullable private final FeatureConfiguration featureConfiguration; @Nullable private final Artifact output; @Nullable private final Artifact interfaceOutput; @Nullable private final Artifact symbolCountsOutput; @@ -77,6 +81,12 @@ public final class LinkCommandLine extends CommandLine { @Nullable private final Artifact paramFile; @Nullable private final Artifact interfaceSoBuilder; + /** + * A string constant for the c++ link action, used to access the feature + * configuration. + */ + public static final String CPP_LINK = "c++-link"; + private LinkCommandLine( BuildConfiguration configuration, ActionOwner owner, @@ -97,7 +107,9 @@ public final class LinkCommandLine extends CommandLine { boolean useTestOnlyFlags, boolean needWholeArchive, @Nullable Artifact paramFile, - Artifact interfaceSoBuilder) { + Artifact interfaceSoBuilder, + CcToolchainFeatures.Variables variables, + @Nullable FeatureConfiguration featureConfiguration) { Preconditions.checkArgument(linkTargetType != LinkTargetType.INTERFACE_DYNAMIC_LIBRARY, "you can't link an interface dynamic library directly"); if (linkTargetType != LinkTargetType.DYNAMIC_LIBRARY) { @@ -123,6 +135,8 @@ public final class LinkCommandLine extends CommandLine { this.configuration = Preconditions.checkNotNull(configuration); this.cppConfiguration = configuration.getFragment(CppConfiguration.class); + this.variables = variables; + this.featureConfiguration = featureConfiguration; this.owner = Preconditions.checkNotNull(owner); this.output = output; this.interfaceOutput = interfaceOutput; @@ -658,7 +672,10 @@ public final class LinkCommandLine extends CommandLine { } argv.addAll(cppConfiguration.getLinkOptions()); - argv.addAll(cppConfiguration.getFdoSupport().getLinkOptions()); + // The feature config can be null for tests. + if (featureConfiguration != null) { + argv.addAll(featureConfiguration.getCommandLine(CPP_LINK, variables)); + } } private static boolean isDynamicLibrary(LinkerInput linkInput) { @@ -933,6 +950,7 @@ public final class LinkCommandLine extends CommandLine { private final BuildConfiguration configuration; private final ActionOwner owner; + @Nullable private final RuleContext ruleContext; @Nullable private Artifact output; @Nullable private Artifact interfaceOutput; @@ -953,13 +971,18 @@ public final class LinkCommandLine extends CommandLine { @Nullable private Artifact paramFile; @Nullable private Artifact interfaceSoBuilder; - public Builder(BuildConfiguration configuration, ActionOwner owner) { + // This interface is needed to support tests that don't create a + // ruleContext, in which case the configuration and action owner + // cannot be accessed off of the give ruleContext. + public Builder(BuildConfiguration configuration, ActionOwner owner, + @Nullable RuleContext ruleContext) { this.configuration = configuration; this.owner = owner; + this.ruleContext = ruleContext; } public Builder(RuleContext ruleContext) { - this(ruleContext.getConfiguration(), ruleContext.getActionOwner()); + this(ruleContext.getConfiguration(), ruleContext.getActionOwner(), ruleContext); } public LinkCommandLine build() { @@ -970,6 +993,17 @@ public final class LinkCommandLine extends CommandLine { actualLinkstampCompileOptions = ImmutableList.copyOf( Iterables.concat(DEFAULT_LINKSTAMP_OPTIONS, linkstampCompileOptions)); } + CcToolchainFeatures.Variables variables = null; + FeatureConfiguration featureConfiguration = null; + // The ruleContext can be null for some tests. + if (ruleContext != null) { + featureConfiguration = CcCommon.configureFeatures(ruleContext); + CcToolchainFeatures.Variables.Builder buildVariables = + new CcToolchainFeatures.Variables.Builder(); + CppConfiguration cppConfiguration = configuration.getFragment(CppConfiguration.class); + cppConfiguration.getFdoSupport().getLinkOptions(featureConfiguration, buildVariables); + variables = buildVariables.build(); + } return new LinkCommandLine( configuration, owner, @@ -990,7 +1024,9 @@ public final class LinkCommandLine extends CommandLine { useTestOnlyFlags, needWholeArchive, paramFile, - interfaceSoBuilder); + interfaceSoBuilder, + variables, + featureConfiguration); } /** |