From c0e5bc50f946c6b127485aeee133c149283ae352 Mon Sep 17 00:00:00 2001 From: Lukacs Berki Date: Wed, 6 Apr 2016 12:31:07 +0000 Subject: Move FDO support to the analysis phase by wrapping FdoSupport in its own SkyFunction. This removes one of the two reasons for the existence of BuildConfiguration#prepareToBuild() which makes implementing dynamic configurations impossible and also makes FDO support halfway sane; now FDO is exactly as ugly as remote repositories, that is to say, reasonably okay. Ideally, we'd implement the zip extraction as an Action and make it a TreeArtifact, but support for TreeArtifacts is not mature yet enough, so it's not possible at the moment. -- MOS_MIGRATED_REVID=119150223 --- .../devtools/build/lib/analysis/BuildView.java | 8 - .../lib/analysis/config/BuildConfiguration.java | 28 --- .../build/lib/bazel/rules/BazelRulesModule.java | 11 + .../devtools/build/lib/rules/cpp/CcCommon.java | 2 +- .../build/lib/rules/cpp/CcLibraryHelper.java | 2 +- .../devtools/build/lib/rules/cpp/CcToolchain.java | 31 ++- .../build/lib/rules/cpp/CppConfiguration.java | 69 ++---- .../devtools/build/lib/rules/cpp/CppHelper.java | 12 +- .../build/lib/rules/cpp/CppLinkAction.java | 18 -- .../devtools/build/lib/rules/cpp/CppModel.java | 6 +- .../devtools/build/lib/rules/cpp/FdoSupport.java | 246 +++++++++++---------- .../build/lib/rules/cpp/FdoSupportFunction.java | 76 +++++++ .../build/lib/rules/cpp/FdoSupportProvider.java | 34 +++ .../build/lib/rules/cpp/FdoSupportValue.java | 101 +++++++++ .../build/lib/rules/cpp/LinkCommandLine.java | 9 +- .../build/lib/skyframe/PrecomputedValue.java | 2 +- .../devtools/build/lib/vfs/ZipFileSystem.java | 54 ++++- src/test/java/com/google/devtools/build/lib/BUILD | 2 + .../build/lib/analysis/mock/BazelAnalysisMock.java | 14 ++ .../build/lib/analysis/util/AnalysisMock.java | 6 + .../build/lib/analysis/util/BuildViewTestCase.java | 2 +- .../skylark/SkylarkRepositoryIntegrationTest.java | 5 +- .../build/lib/rules/cpp/CppLinkActionTest.java | 22 +- 23 files changed, 507 insertions(+), 253 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportProvider.java create mode 100644 src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 7a4f72a4f7..98a57b81ca 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -403,13 +403,6 @@ public class BuildView { }); } - private void prepareToBuild(BuildConfigurationCollection configurations) - throws ViewCreationFailedException { - for (BuildConfiguration config : configurations.getAllConfigurations()) { - config.prepareToBuild(directories.getExecRoot()); - } - } - @ThreadCompatible public AnalysisResult update( LoadingResult loadingResult, @@ -484,7 +477,6 @@ public class BuildView { } } - prepareToBuild(configurations); skyframeExecutor.injectWorkspaceStatusData(); SkyframeAnalysisResult skyframeAnalysisResult; try { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index e0ced04c8d..33aa510f7a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.Dependency; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection.Transitions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; @@ -168,18 +167,6 @@ public final class BuildConfiguration { return implicitLabels; } - /** - * The fragment may use this hook to perform I/O and read data into memory that is used during - * analysis. During the analysis phase disk I/O operations are disallowed. - * - *

This hook is called for all configurations after the loading phase is complete. - * - *

Do not use this method to change your fragment's state. - */ - @SuppressWarnings("unused") - public void prepareHook(Path execPath) throws ViewCreationFailedException { - } - /** * Adds all the roots from this fragment. */ @@ -2331,21 +2318,6 @@ public final class BuildConfiguration { return buildOptions; } - /** - * Prepare the fdo support. It reads data into memory that is used during analysis. The analysis - * phase is generally not allowed to perform disk I/O. This code is here because it is - * conceptually part of the analysis phase, and it needs to happen when the loading phase is - * complete. - * - *

C++ also requires this to resolve artifacts that are unconditionally included in every - * compilation.

- */ - public void prepareToBuild(Path execRoot) throws ViewCreationFailedException { - for (Fragment fragment : fragments.values()) { - fragment.prepareHook(execRoot); - } - } - /** * Declares dependencies on any relevant Skyframe values (for example, relevant FileValues). */ diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java index 2af2575ff9..a0c142e096 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java @@ -24,6 +24,7 @@ import com.google.devtools.build.lib.actions.ActionContextConsumer; import com.google.devtools.build.lib.actions.ActionContextProvider; import com.google.devtools.build.lib.actions.Executor.ActionContext; import com.google.devtools.build.lib.actions.SimpleActionContextProvider; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.actions.FileWriteActionContext; import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses; @@ -31,6 +32,8 @@ import com.google.devtools.build.lib.query2.output.OutputFormatter; import com.google.devtools.build.lib.rules.android.WriteAdbArgsActionContext; import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext; import com.google.devtools.build.lib.rules.cpp.CppLinkActionContext; +import com.google.devtools.build.lib.rules.cpp.FdoSupportFunction; +import com.google.devtools.build.lib.rules.cpp.FdoSupportValue; import com.google.devtools.build.lib.rules.cpp.IncludeScanningContext; import com.google.devtools.build.lib.rules.genquery.GenQuery; import com.google.devtools.build.lib.runtime.BlazeModule; @@ -39,6 +42,8 @@ import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.build.lib.runtime.GotOptionsEvent; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.util.ResourceFileLoader; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.common.options.Converters.AssignmentConverter; import com.google.devtools.common.options.Option; import com.google.devtools.common.options.OptionsBase; @@ -194,4 +199,10 @@ public class BazelRulesModule extends BlazeModule { } })); } + + @Override + public ImmutableMap getSkyFunctions(BlazeDirectories directories) { + return ImmutableMap.of( + FdoSupportValue.SKYFUNCTION, new FdoSupportFunction()); + } } 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 579bb53bfa..b1639ea9a2 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 @@ -178,7 +178,7 @@ public final class CcCommon { } public TransitiveLipoInfoProvider collectTransitiveLipoLabels(CcCompilationOutputs outputs) { - if (cppConfiguration.getFdoSupport().getFdoRoot() == null + if (CppHelper.getFdoSupport(ruleContext).getFdoRoot() == null || !cppConfiguration.isLipoContextCollector()) { return TransitiveLipoInfoProvider.EMPTY; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java index 79d1c8d2dd..1ec2c57387 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java @@ -922,7 +922,7 @@ public final class CcLibraryHelper { } private TransitiveLipoInfoProvider collectTransitiveLipoInfo(CcCompilationOutputs outputs) { - if (ruleContext.getFragment(CppConfiguration.class).getFdoSupport().getFdoRoot() == null) { + if (CppHelper.getFdoSupport(ruleContext).getFdoRoot() == null) { return TransitiveLipoInfoProvider.EMPTY; } NestedSetBuilder scannableBuilder = NestedSetBuilder.stableOrder(); 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 6cc3a5c401..e3efe6c0ce 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 @@ -33,15 +33,21 @@ import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.Runfiles; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; +import com.google.devtools.build.lib.analysis.config.CompilationMode; import com.google.devtools.build.lib.cmdline.Label; 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.RuleConfiguredTargetFactory; +import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException; import com.google.devtools.build.lib.util.Preconditions; +import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyKey; +import java.io.IOException; import java.util.List; import java.util.Map; @@ -57,6 +63,29 @@ public class CcToolchain implements RuleConfiguredTargetFactory { @Override public ConfiguredTarget create(RuleContext ruleContext) { + CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class); + Path fdoZip = ruleContext.getConfiguration().getCompilationMode() == CompilationMode.OPT + ? cppConfiguration.getFdoZip() + : null; + SkyKey fdoKey = FdoSupportValue.key( + cppConfiguration.getLipoMode(), + fdoZip, + cppConfiguration.getFdoInstrument()); + + SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv(); + FdoSupportValue fdoSupport; + try { + fdoSupport = (FdoSupportValue) skyframeEnv.getValueOrThrow( + fdoKey, FdoException.class, IOException.class); + } catch (FdoException | IOException e) { + ruleContext.ruleError("cannot initialize FDO: " + e.getMessage()); + return null; + } + + if (skyframeEnv.valuesMissing()) { + return null; + } + final Label label = ruleContext.getLabel(); final NestedSet crosstool = ruleContext.getPrerequisite("all_files", Mode.HOST) .getProvider(FileProvider.class).getFilesToBuild(); @@ -72,7 +101,6 @@ public class CcToolchain implements RuleConfiguredTargetFactory { final PathFragment runtimeSolibDir = ruleContext.getConfiguration() .getBinFragment().getRelative(runtimeSolibDirBase); - CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class); // Static runtime inputs. TransitiveInfoCollection staticRuntimeLibDep = selectDep(ruleContext, "static_runtime_libs", cppConfiguration.getStaticRuntimeLibsLabel()); @@ -173,6 +201,7 @@ public class CcToolchain implements RuleConfiguredTargetFactory { RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext) .add(CcToolchainProvider.class, provider) + .add(FdoSupportProvider.class, new FdoSupportProvider(fdoSupport.getFdoSupport())) .setFilesToBuild(new NestedSetBuilder(Order.STABLE_ORDER).build()) .add(RunfilesProvider.class, RunfilesProvider.simple(Runfiles.EMPTY)); 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 f151deaf59..c1831ceaf7 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 @@ -25,9 +25,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; import com.google.common.collect.Multimap; -import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.analysis.RuleContext; -import com.google.devtools.build.lib.analysis.ViewCreationFailedException; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.CompilationMode; @@ -40,7 +38,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters; -import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; import com.google.devtools.build.lib.util.Preconditions; @@ -51,7 +48,6 @@ import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LinkingModeFlags; import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode; -import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.common.options.OptionsParsingException; import com.google.protobuf.TextFormat; import com.google.protobuf.TextFormat.ParseException; @@ -64,7 +60,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.zip.ZipException; /** * This class represents the C/C++ parts of the {@link BuildConfiguration}, @@ -284,7 +279,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment { private final boolean toolchainNeedsPic; private final boolean usePicForBinaries; - private final FdoSupport fdoSupport; + private final Path fdoZip; // TODO(bazel-team): All these labels (except for ccCompilerRuleLabel) can be removed once the // transition to the cc_compiler rule is complete. @@ -390,10 +385,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment { } } - this.fdoSupport = new FdoSupport( - cppOptions.fdoInstrument, params.fdoZip, - cppOptions.lipoMode, execRoot); - + this.fdoZip = params.fdoZip; this.stripBinaries = (cppOptions.stripBinaries == StripMode.ALWAYS || (cppOptions.stripBinaries == StripMode.SOMETIMES @@ -1481,8 +1473,9 @@ public class CppConfiguration extends BuildConfiguration.Fragment { * Returns true if it is AutoFDO LIPO build. */ public boolean isAutoFdoLipo() { - return cppOptions.fdoOptimize != null && FdoSupport.isAutoFdo(cppOptions.fdoOptimize) - && getLipoMode() != LipoMode.OFF; + return cppOptions.fdoOptimize != null + && CppFileTypes.GCC_AUTO_PROFILE.matches(cppOptions.fdoOptimize) + && getLipoMode() != LipoMode.OFF; } /** @@ -1638,13 +1631,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment { return cppOptions.useInterfaceSharedObjects; } - /** - * Returns the FDO support object. - */ - public FdoSupport getFdoSupport() { - return fdoSupport; - } - /** * Return the name of the directory (relative to the bin directory) that * holds mangled links to shared libraries. This name is always set to @@ -1891,36 +1877,11 @@ public class CppConfiguration extends BuildConfiguration.Fragment { implicitLabels.put("crosstool", crosstoolTop); } - @Override - public void prepareHook(Path execRoot) throws ViewCreationFailedException { - try { - getFdoSupport().prepareToBuild(execRoot); - } catch (ZipException e) { - throw new ViewCreationFailedException("Error reading provided FDO zip file", e); - } catch (FdoException | IOException e) { - throw new ViewCreationFailedException("Error while initializing FDO support", e); - } - } - - @Override - public void declareSkyframeDependencies(Environment env) { - getFdoSupport().declareSkyframeDependencies(env, execRoot); - } - - @Override - public void addRoots(List roots) { - // Fdo root can only exist for the target configuration. - FdoSupport fdoSupport = getFdoSupport(); - if (fdoSupport.getFdoRoot() != null) { - roots.add(fdoSupport.getFdoRoot()); - } - } - @Override public Map getCoverageEnvironment() { ImmutableMap.Builder env = ImmutableMap.builder(); env.put("COVERAGE_GCOV_PATH", getGcovExecutable().getPathString()); - PathFragment fdoInstrument = getFdoSupport().getFdoInstrument(); + PathFragment fdoInstrument = cppOptions.fdoInstrument; if (fdoInstrument != null) { env.put("FDO_DIR", fdoInstrument.getPathString()); } @@ -1990,6 +1951,14 @@ public class CppConfiguration extends BuildConfiguration.Fragment { ); } + public PathFragment getFdoInstrument() { + return cppOptions.fdoInstrument; + } + + public Path getFdoZip() { + return fdoZip; + } + /** * Return set of features enabled by the CppConfiguration, specifically * the FDO and LIPO related features enabled by options. @@ -1997,15 +1966,15 @@ public class CppConfiguration extends BuildConfiguration.Fragment { @Override public ImmutableSet configurationEnabledFeatures(RuleContext ruleContext) { ImmutableSet.Builder requestedFeatures = ImmutableSet.builder(); - FdoSupport fdoSupport = getFdoSupport(); - if (fdoSupport.getFdoInstrument() != null) { + if (cppOptions.fdoInstrument != null) { requestedFeatures.add(CppRuleClasses.FDO_INSTRUMENT); } - if (fdoSupport.getFdoOptimizeProfile() != null - && !fdoSupport.isAutoFdoEnabled()) { + + boolean isFdo = fdoZip != null && compilationMode == CompilationMode.OPT; + if (isFdo && !CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) { requestedFeatures.add(CppRuleClasses.FDO_OPTIMIZE); } - if (fdoSupport.isAutoFdoEnabled()) { + if (isFdo && CppFileTypes.GCC_AUTO_PROFILE.matches(fdoZip)) { requestedFeatures.add(CppRuleClasses.AUTOFDO); } if (isLipoOptimizationOrInstrumentation()) { 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 97db3a514f..300c87e122 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 @@ -214,6 +214,13 @@ public class CppHelper { return false; } + @Nullable public static FdoSupport getFdoSupport(RuleContext ruleContext) { + return ruleContext + .getPrerequisite(":cc_toolchain", Mode.TARGET) + .getProvider(FdoSupportProvider.class) + .getFdoSupport(); + } + /** * This almost trivial method looks up the :cc_toolchain attribute on the rule context, makes sure * that it refers to a rule that has a {@link CcToolchainProvider} (gives an error otherwise), and @@ -508,8 +515,9 @@ public class CppHelper { /** * Returns the FDO build subtype. */ - public static String getFdoBuildStamp(CppConfiguration cppConfiguration) { - if (cppConfiguration.getFdoSupport().isAutoFdoEnabled()) { + public static String getFdoBuildStamp(RuleContext ruleContext) { + CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class); + if (getFdoSupport(ruleContext).isAutoFdoEnabled()) { return (cppConfiguration.getLipoMode() == LipoMode.BINARY) ? "ALIPO" : "AFDO"; } if (cppConfiguration.isFdo()) { 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 95d6379f18..b0a1b7d825 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 @@ -1213,24 +1213,6 @@ public final class CppLinkAction extends AbstractAction { this.runtimeSolibDir = runtimeSolibDir; return this; } - - /** - * Creates a builder without the need for a {@link RuleContext}. - * This is to be used exclusively for testing purposes. - * - *

Link stamping is not supported if using this method. - */ - @VisibleForTesting - public static Builder createTestBuilder( - final ActionOwner owner, final AnalysisEnvironment analysisEnvironment, - final Artifact output, BuildConfiguration config) { - return new Builder(null, output, config, analysisEnvironment, null) { - @Override - protected ActionOwner getOwner() { - return owner; - } - }; - } } /** 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 c8a8c1a19b..bd8bbfa1a2 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 @@ -287,7 +287,7 @@ public final class CppModel { } builder.setExtraSystemIncludePrefixes(additionalIncludes); - builder.setFdoBuildStamp(CppHelper.getFdoBuildStamp(cppConfiguration)); + builder.setFdoBuildStamp(CppHelper.getFdoBuildStamp(ruleContext)); builder.setFeatureConfiguration(featureConfiguration); return builder; } @@ -367,8 +367,8 @@ public final class CppModel { } if (ccRelativeName != null) { - cppConfiguration.getFdoSupport().configureCompilation(builder, buildVariables, ruleContext, - ccRelativeName, autoFdoImportPath, usePic, featureConfiguration); + CppHelper.getFdoSupport(ruleContext).configureCompilation(builder, buildVariables, + ruleContext, ccRelativeName, autoFdoImportPath, usePic, featureConfiguration); } if (gcnoFile != null) { buildVariables.addVariable("gcov_gcno_file", gcnoFile.getExecPathString()); 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 b182c8cf04..18f2cf18df 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 @@ -25,7 +25,7 @@ import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadHostile; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; 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; @@ -47,20 +47,13 @@ import java.util.zip.ZipException; * Support class for FDO (feedback directed optimization) and LIPO (lightweight inter-procedural * optimization). * - *

There is a 1:1 relationship between {@link CppConfiguration} objects and {@code FdoSupport} - * objects. The FDO support of a build configuration can be retrieved using {@link - * CppConfiguration#getFdoSupport()}. - * - *

With respect to thread-safety, the {@link #prepareToBuild} method is not thread-safe, and must - * not be called concurrently with other methods on this class. - * *

Here follows a quick run-down of how FDO/LIPO builds work (for non-FDO/LIPO builds, none * of this applies): * - *

{@link CppConfiguration#prepareHook} is called before the analysis phase, which calls - * {@link #prepareToBuild}, which extracts the FDO .zip (in case we work with an explicitly - * generated FDO profile file) or analyzes the .afdo.imports file next to the .afdo file (if - * AutoFDO is in effect). + *

{@link FdoSupport#create} is called from {@link FdoSupportFunction} (a {@link SkyFunction}), + * which is requested from Skyframe by the {@code cc_toolchain} rule. It extracts the FDO .zip (in + * case we work with an explicitly generated FDO profile file) or analyzes the .afdo.imports file + * next to the .afdo file (if AutoFDO is in effect). * *

.afdo.imports files contain one import a line. A line is two paths separated by a colon, * with functions in the second path being referenced by functions in the first path. These are @@ -108,8 +101,29 @@ import java.util.zip.ZipException; * {@code CachingAnalysisEnvironment.allowRegisteringActions}, which causes actions to be silently * discarded after configured targets are created. */ +@Immutable public class FdoSupport { + /** + * The FDO mode we are operating in. + * + * LIPO can only be active if this is not OFF, but all of the modes below can work + * with LIPO either off or on. + */ + private enum FdoMode { + /** FDO is turned off. */ + OFF, + + /** Profiling-based FDO using an explicitly recorded profile. */ + VANILLA, + + /** FDO based on automatically collected data. */ + AUTO_FDO, + + /** Instrumentation-based FDO implemented on LLVM. */ + LLVM_FDO, + } + /** * Path within profile data .zip files that is considered the root of the * profile information directory tree. @@ -170,24 +184,15 @@ public class FdoSupport { private final LipoMode lipoMode; /** - * Flag indicating whether to use AutoFDO (as opposed to - * instrumentation-based FDO). + * FDO mode. */ - private final boolean useAutoFdo; - - /** - * Flag indicating whether to use LLVM instrumentation-based FDO (as - * opposed to GCC instrumentation-based FDO). - */ - private final boolean useLLVMFdo; + private final FdoMode fdoMode; /** * The {@code .gcda} files that have been extracted from the ZIP file, * relative to the root of the ZIP file. - * - *

Set only in {@link #prepareToBuild}. */ - private ImmutableSet gcdaFiles = ImmutableSet.of(); + private final ImmutableSet gcdaFiles; /** * Multimap from .gcda file base names to auxiliary input files. @@ -198,10 +203,8 @@ public class FdoSupport { * *

The contents of the multimap are copied verbatim from the .gcda.imports * files and not yet checked for validity. - * - *

Set only in {@link #prepareToBuild}. */ - private ImmutableMultimap imports; + private final ImmutableMultimap imports; /** * Creates an FDO support object. @@ -210,83 +213,122 @@ public class FdoSupport { * @param fdoProfile path to the profile file passed to --fdo_optimize option * @param lipoMode value of the --lipo_mode option */ - public FdoSupport(PathFragment fdoInstrument, Path fdoProfile, LipoMode lipoMode, Path execRoot) { + private FdoSupport(FdoMode fdoMode, LipoMode lipoMode, Root fdoRoot, PathFragment fdoRootExecPath, + PathFragment fdoInstrument, Path fdoProfile, FdoZipContents fdoZipContents) { this.fdoInstrument = fdoInstrument; this.fdoProfile = fdoProfile; - this.fdoRoot = (fdoProfile == null) - ? null - : Root.asDerivedRoot(execRoot, execRoot.getRelative("blaze-fdo")); - this.fdoRootExecPath = fdoProfile == null - ? null - : fdoRoot.getExecPath().getRelative(FileSystemUtils.removeExtension( - new PathFragment("_fdo").getChild(fdoProfile.getBaseName()))); + this.fdoRoot = fdoRoot; + this.fdoRootExecPath = fdoRootExecPath; this.fdoPath = fdoProfile == null ? null : FileSystemUtils.removeExtension(new PathFragment("_fdo").getChild( fdoProfile.getBaseName())); this.lipoMode = lipoMode; - this.useAutoFdo = fdoProfile != null && isAutoFdo(fdoProfile.getBaseName()); - this.useLLVMFdo = fdoProfile != null && isLLVMFdo(fdoProfile.getBaseName()); + this.fdoMode = fdoMode; + this.gcdaFiles = fdoZipContents.gcdaFiles; + this.imports = fdoZipContents.imports; } public Root getFdoRoot() { return fdoRoot; } - public void declareSkyframeDependencies(SkyFunction.Environment env, Path execRoot) { + /** + * Creates an initialized {@link FdoSupport} instance. + */ + static FdoSupport create(SkyFunction.Environment env, PathFragment fdoInstrument, + Path fdoProfile, LipoMode lipoMode, Path execRoot) throws IOException, FdoException { + FdoMode fdoMode; + if (fdoProfile != null && isAutoFdo(fdoProfile.getBaseName())) { + fdoMode = FdoMode.AUTO_FDO; + } else if (fdoProfile != null && isLLVMFdo(fdoProfile.getBaseName())) { + fdoMode = FdoMode.LLVM_FDO; + } else if (fdoProfile != null) { + fdoMode = FdoMode.VANILLA; + } else { + fdoMode = FdoMode.OFF; + } + + if (fdoProfile == null) { + lipoMode = LipoMode.OFF; + } + + Root fdoRoot = (fdoProfile == null) + ? null + : Root.asDerivedRoot(execRoot, execRoot.getRelative("blaze-fdo")); + + PathFragment fdoRootExecPath = fdoProfile == null + ? null + : fdoRoot.getExecPath().getRelative(FileSystemUtils.removeExtension( + new PathFragment("_fdo").getChild(fdoProfile.getBaseName()))); + if (fdoProfile != null) { - if (isLipoEnabled()) { + if (lipoMode != LipoMode.OFF) { // Incrementality is not supported for LIPO builds, see FdoSupport#scannables. // Ensure that the Skyframe value containing the configuration will not be reused to avoid // incrementality issues. PrecomputedValue.dependOnBuildId(env); - return; - } - - // IMPORTANT: Keep the following in sync with #prepareToBuild. - Path path; - if (useAutoFdo) { - path = fdoProfile.getParentDirectory().getRelative( - fdoProfile.getBaseName() + ".imports"); } else { - path = fdoProfile; + Path path = fdoMode == FdoMode.AUTO_FDO ? getAutoFdoImportsPath(fdoProfile) : fdoProfile; + env.getValue(FileValue.key(RootedPath.toRootedPathMaybeUnderRoot(path, + ImmutableList.of(execRoot)))); } - env.getValue(FileValue.key(RootedPath.toRootedPathMaybeUnderRoot(path, - ImmutableList.of(execRoot)))); + } + + if (env.valuesMissing()) { + return null; + } + + FdoZipContents fdoZipContents = extractFdoZip( + fdoMode, lipoMode, execRoot, fdoProfile, fdoRootExecPath); + return new FdoSupport( + fdoMode, lipoMode, fdoRoot, fdoRootExecPath, fdoInstrument, fdoProfile, fdoZipContents); + } + + @Immutable + private static class FdoZipContents { + private final ImmutableSet gcdaFiles; + private final ImmutableMultimap imports; + + private FdoZipContents(ImmutableSet gcdaFiles, + ImmutableMultimap imports) { + this.gcdaFiles = gcdaFiles; + this.imports = imports; } } /** - * Prepares the FDO support for building. + * Extracts the FDO zip file and collects data from it that's needed during analysis. * *

When an {@code --fdo_optimize} compile is requested, unpacks the given * FDO gcda zip file into a clean working directory under execRoot. * * @throws FdoException if the FDO ZIP contains a file of unknown type */ - @ThreadHostile // must be called before starting the build - public void prepareToBuild(Path execRoot) + private static FdoZipContents extractFdoZip(FdoMode fdoMode, LipoMode lipoMode, Path execRoot, + Path fdoProfile, PathFragment fdoRootExecPath) throws IOException, FdoException { // The execRoot != null case is only there for testing. We cannot provide a real ZIP file in // tests because ZipFileSystem does not work with a ZIP on an in-memory file system. // IMPORTANT: Keep in sync with #declareSkyframeDependencies to avoid incrementality issues. + ImmutableSet gcdaFiles = ImmutableSet.of(); + ImmutableMultimap imports = ImmutableMultimap.of(); + if (fdoProfile != null && execRoot != null) { Path fdoDirPath = execRoot.getRelative(fdoRootExecPath); FileSystemUtils.deleteTreesBelow(fdoDirPath); FileSystemUtils.createDirectoryAndParents(fdoDirPath); - if (useAutoFdo) { - Path fdoImports = fdoProfile.getParentDirectory().getRelative( - fdoProfile.getBaseName() + ".imports"); - if (isLipoEnabled()) { - imports = readAutoFdoImports(fdoImports); + if (fdoMode == FdoMode.AUTO_FDO) { + if (lipoMode != LipoMode.OFF) { + imports = readAutoFdoImports(getAutoFdoImportsPath(fdoProfile)); } FileSystemUtils.ensureSymbolicLink( - execRoot.getRelative(getAutoProfilePath()), fdoProfile); - } else if (useLLVMFdo) { + execRoot.getRelative(getAutoProfilePath(fdoProfile, fdoRootExecPath)), fdoProfile); + } else if (fdoMode == FdoMode.LLVM_FDO) { FileSystemUtils.ensureSymbolicLink( - execRoot.getRelative(getLLVMProfilePath()), fdoProfile); + execRoot.getRelative(getLLVMProfilePath(fdoProfile, fdoRootExecPath)), fdoProfile); } else { Path zipFilePath = new ZipFileSystem(fdoProfile).getRootDirectory(); if (!zipFilePath.getRelative("blaze-out").isDirectory()) { @@ -296,11 +338,13 @@ public class FdoSupport { ImmutableSet.Builder gcdaFilesBuilder = ImmutableSet.builder(); ImmutableMultimap.Builder importsBuilder = ImmutableMultimap.builder(); - extractFdoZip(zipFilePath, fdoDirPath, gcdaFilesBuilder, importsBuilder); + extractFdoZipDirectory(zipFilePath, fdoDirPath, gcdaFilesBuilder, importsBuilder); gcdaFiles = gcdaFilesBuilder.build(); imports = importsBuilder.build(); } } + + return new FdoZipContents(gcdaFiles, imports); } /** @@ -320,7 +364,7 @@ public class FdoSupport { * @throws IOException if any of the I/O operations failed * @throws FdoException if the FDO ZIP contains a file of unknown type */ - private void extractFdoZip(Path sourceDir, Path targetDir, + private static void extractFdoZipDirectory(Path sourceDir, Path targetDir, ImmutableSet.Builder gcdaFilesBuilder, ImmutableMultimap.Builder importsBuilder) throws IOException, FdoException { @@ -328,7 +372,7 @@ public class FdoSupport { Path targetFile = targetDir.getRelative(sourceFile.getBaseName()); if (sourceFile.isDirectory()) { targetFile.createDirectory(); - extractFdoZip(sourceFile, targetFile, gcdaFilesBuilder, importsBuilder); + extractFdoZipDirectory(sourceFile, targetFile, gcdaFilesBuilder, importsBuilder); } else { if (CppFileTypes.COVERAGE_DATA.matches(sourceFile)) { FileSystemUtils.copyFile(sourceFile, targetFile); @@ -343,12 +387,13 @@ public class FdoSupport { } } + /** * Reads a .gcda.imports file and stores the imports information. * * @throws FdoException if an auxiliary LIPO input was not found */ - private void readCoverageImports(Path importsFile, + private static void readCoverageImports(Path importsFile, ImmutableMultimap.Builder importsBuilder) throws IOException, FdoException { PathFragment key = importsFile.asFragment().relativeTo(ZIP_ROOT); @@ -373,7 +418,7 @@ public class FdoSupport { /** * Reads a .afdo.imports file and stores the imports information. */ - private ImmutableMultimap readAutoFdoImports(Path importsFile) + private static ImmutableMultimap readAutoFdoImports(Path importsFile) throws IOException, FdoException { ImmutableMultimap.Builder importBuilder = ImmutableMultimap.builder(); @@ -396,6 +441,10 @@ public class FdoSupport { return importBuilder.build(); } + private static Path getAutoFdoImportsPath(Path fdoProfile) { + return fdoProfile.getParentDirectory().getRelative(fdoProfile.getBaseName() + ".imports"); + } + /** * Returns the imports from the .afdo.imports file of a source file. * @@ -403,7 +452,7 @@ public class FdoSupport { */ private Collection getAutoFdoImports(RuleContext ruleContext, PathFragment sourceExecPath, LipoContextProvider lipoContextProvider) { - Preconditions.checkState(isLipoEnabled()); + Preconditions.checkState(lipoMode != LipoMode.OFF); ImmutableCollection afdoImports = imports.get(sourceExecPath); Preconditions.checkState(afdoImports != null, "AutoFDO import data missing for %s", sourceExecPath); @@ -427,7 +476,7 @@ public class FdoSupport { * @param objectName the object file */ private Iterable getImports(PathFragment objDirectory, PathFragment objectName) { - Preconditions.checkState(isLipoEnabled()); + Preconditions.checkState(lipoMode != LipoMode.OFF); Preconditions.checkState(imports != null, "Tried to look up imports of uninitialized FDOSupport"); PathFragment key = objDirectory.getRelative(FileSystemUtils.removeExtension(objectName)); @@ -449,7 +498,7 @@ public class FdoSupport { // 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()); + Preconditions.checkArgument(lipoInputProvider == null || lipoMode != LipoMode.OFF); // FDO is disabled -> do nothing. if ((fdoInstrument == null) && (fdoRoot == null)) { @@ -474,12 +523,12 @@ public class FdoSupport { if (!Iterables.isEmpty(auxiliaryInputs)) { if (featureConfiguration.isEnabled(CppRuleClasses.AUTOFDO)) { buildVariables.addVariable("fdo_profile_path", - getAutoProfilePath().getPathString()); + getAutoProfilePath(fdoProfile, fdoRootExecPath).getPathString()); } if (featureConfiguration.isEnabled(CppRuleClasses.FDO_OPTIMIZE)) { - if (useLLVMFdo) { + if (fdoMode == FdoMode.LLVM_FDO) { buildVariables.addVariable("fdo_profile_path", - getLLVMProfilePath().getPathString()); + getLLVMProfilePath(fdoProfile, fdoRootExecPath).getPathString()); } else { buildVariables.addVariable("fdo_profile_path", fdoRootExecPath.getPathString()); @@ -498,12 +547,12 @@ public class FdoSupport { // If --fdo_optimize was not specified, we don't have any additional inputs. if (fdoProfile == null) { return ImmutableSet.of(); - } else if (useAutoFdo || useLLVMFdo) { + } else if (fdoMode == FdoMode.AUTO_FDO || fdoMode == FdoMode.LLVM_FDO) { ImmutableSet.Builder auxiliaryInputs = ImmutableSet.builder(); - PathFragment profileRootRelativePath = (useLLVMFdo) - ? getLLVMProfileRootRelativePath() - : getAutoProfileRootRelativePath(); + PathFragment profileRootRelativePath = fdoMode == FdoMode.LLVM_FDO + ? getLLVMProfileRootRelativePath(fdoProfile) + : getAutoProfileRootRelativePath(fdoProfile); Artifact artifact = env.getDerivedArtifact( fdoPath.getRelative(profileRootRelativePath), fdoRoot); env.registerAction(new FdoStubAction(ruleContext.getActionOwner(), artifact)); @@ -607,37 +656,29 @@ public class FdoSupport { } - private PathFragment getAutoProfilePath() { - return fdoRootExecPath.getRelative(getAutoProfileRootRelativePath()); + private static PathFragment getAutoProfilePath(Path fdoProfile, PathFragment fdoRootExecPath) { + return fdoRootExecPath.getRelative(getAutoProfileRootRelativePath(fdoProfile)); } - private PathFragment getAutoProfileRootRelativePath() { + private static PathFragment getAutoProfileRootRelativePath(Path fdoProfile) { return new PathFragment(fdoProfile.getBaseName()); } - private PathFragment getLLVMProfilePath() { - return fdoRootExecPath.getRelative(getLLVMProfileRootRelativePath()); + private static PathFragment getLLVMProfilePath(Path fdoProfile, PathFragment fdoRootExecPath) { + return fdoRootExecPath.getRelative(getLLVMProfileRootRelativePath(fdoProfile)); } - private PathFragment getLLVMProfileRootRelativePath() { + private static PathFragment getLLVMProfileRootRelativePath(Path fdoProfile) { return new PathFragment(fdoProfile.getBaseName()); } - /** - * Returns whether LIPO is enabled. - */ - @ThreadSafe - public boolean isLipoEnabled() { - return fdoProfile != null && lipoMode != LipoMode.OFF; - } - /** * Returns whether AutoFDO is enabled. */ @ThreadSafe public boolean isAutoFdoEnabled() { - return useAutoFdo; + return fdoMode == FdoMode.AUTO_FDO; } /** @@ -663,29 +704,6 @@ public class FdoSupport { return fdoRootExecPath; } - /** - * Returns the path of the FDO zip containing the .gcda profile files, or null - * if FDO is not enabled. - */ - @VisibleForTesting - public Path getFdoOptimizeProfile() { - return fdoProfile; - } - - /** - * Returns the path fragment of the instrumentation output dir for gcc when - * FDO is enabled, or null if FDO is not enabled. - */ - @ThreadSafe - public PathFragment getFdoInstrument() { - return fdoInstrument; - } - - @VisibleForTesting - public void setGcdaFilesForTesting(ImmutableSet gcdaFiles) { - this.gcdaFiles = gcdaFiles; - } - /** * An exception indicating an issue with FDO coverage files. */ diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java new file mode 100644 index 0000000000..b232a31582 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportFunction.java @@ -0,0 +1,76 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import com.google.devtools.build.lib.analysis.BlazeDirectories; +import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException; +import com.google.devtools.build.lib.skyframe.PrecomputedValue; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionException; +import com.google.devtools.build.skyframe.SkyFunctionException.Transience; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; + +import java.io.IOException; + +import javax.annotation.Nullable; + +/** + * Wrapper for {@link FdoSupport} that turns it into a {@link SkyFunction}. + */ +public class FdoSupportFunction implements SkyFunction { + + /** + * Wrapper for FDO exceptions. + */ + public class FdoSkyException extends SkyFunctionException { + public FdoSkyException(Exception cause, Transience transience) { + super(cause, transience); + } + } + + @Nullable + @Override + public SkyValue compute(SkyKey skyKey, Environment env) + throws FdoSkyException, InterruptedException { + BlazeDirectories blazeDirectories = PrecomputedValue.BLAZE_DIRECTORIES.get(env); + if (env.valuesMissing()) { + return null; + } + + Path execRoot = blazeDirectories.getExecRoot(); + FdoSupportValue.Key key = (FdoSupportValue.Key) skyKey.argument(); + FdoSupport fdoSupport; + try { + fdoSupport = FdoSupport.create(env, + key.getFdoInstrument(), key.getFdoZip(), key.getLipoMode(), execRoot); + if (env.valuesMissing()) { + return null; + } + } catch (FdoException e) { + throw new FdoSkyException(e, Transience.PERSISTENT); + } catch (IOException e) { + throw new FdoSkyException(e, Transience.TRANSIENT); + } + + return new FdoSupportValue(fdoSupport); + } + + @Nullable + @Override + public String extractTag(SkyKey skyKey) { + return null; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportProvider.java new file mode 100644 index 0000000000..f97621421a --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportProvider.java @@ -0,0 +1,34 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; + +/** + * A {@link TransitiveInfoProvider} so that {@code cc_toolchain} can pass {@link FdoSupport} to the + * C++ rules. + */ +@Immutable +public class FdoSupportProvider implements TransitiveInfoProvider { + private final FdoSupport fdoSupport; + + public FdoSupportProvider(FdoSupport fdoSupport) { + this.fdoSupport = fdoSupport; + } + + public FdoSupport getFdoSupport() { + return fdoSupport; + } +} diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java new file mode 100644 index 0000000000..eb03db6a3f --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoSupportValue.java @@ -0,0 +1,101 @@ +// Copyright 2014 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.rules.cpp; + +import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoMode; +import com.google.devtools.build.skyframe.SkyFunctionName; +import com.google.devtools.build.skyframe.SkyKey; +import com.google.devtools.build.skyframe.SkyValue; + +import java.util.Objects; + +/** + * Wrapper for {@link FdoSupport}. + * + *

Things could probably be refactored such the attributes of {@link FdoSupport} are moved here + * and the code in it to {@link FdoSupportFunction}. This would let us eliminate {@link FdoSupport}. + * + *

The eventual plan is to migrate FDO functionality to the execution phase once directory + * artifacts work better, so this may not be worth it. + */ +@Immutable +public class FdoSupportValue implements SkyValue { + public static final SkyFunctionName SKYFUNCTION = SkyFunctionName.create("FDO_SUPPORT"); + + /** + * {@link SkyKey} for {@link FdoSupportValue}. + */ + @Immutable + public static class Key { + private final LipoMode lipoMode; + private final Path fdoZip; + private final PathFragment fdoInstrument; + + private Key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument) { + this.lipoMode = lipoMode; + this.fdoZip = fdoZip; + this.fdoInstrument = fdoInstrument; + } + + public LipoMode getLipoMode() { + return lipoMode; + } + + public Path getFdoZip() { + return fdoZip; + } + + public PathFragment getFdoInstrument() { + return fdoInstrument; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + + if (!(o instanceof Key)) { + return false; + } + + Key that = (Key) o; + return Objects.equals(this.lipoMode, that.lipoMode) + && Objects.equals(this.fdoZip, that.fdoZip) + && Objects.equals(this.fdoInstrument, that.fdoInstrument); + } + + @Override + public int hashCode() { + return Objects.hash(lipoMode, fdoZip, fdoInstrument); + } + } + + private final FdoSupport fdoSupport; + + FdoSupportValue(FdoSupport fdoSupport) { + this.fdoSupport = fdoSupport; + } + + public FdoSupport getFdoSupport() { + return fdoSupport; + } + + public static SkyKey key(LipoMode lipoMode, Path fdoZip, PathFragment fdoInstrument) { + return SkyKey.create(SKYFUNCTION, new Key(lipoMode, fdoZip, fdoInstrument)); + } +} 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 98ed76fa10..79c24e6bef 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 @@ -75,6 +75,7 @@ public final class LinkCommandLine extends CommandLine { private final ImmutableSet features; private final ImmutableMap linkstamps; private final ImmutableList linkstampCompileOptions; + @Nullable private final String fdoBuildStamp; @Nullable private final PathFragment runtimeSolibDir; private final boolean nativeDeps; private final boolean useTestOnlyFlags; @@ -84,6 +85,7 @@ 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. @@ -105,6 +107,7 @@ public final class LinkCommandLine extends CommandLine { ImmutableSet features, ImmutableMap linkstamps, ImmutableList linkstampCompileOptions, + @Nullable String fdoBuildStamp, @Nullable PathFragment runtimeSolibDir, boolean nativeDeps, boolean useTestOnlyFlags, @@ -161,6 +164,7 @@ public final class LinkCommandLine extends CommandLine { this.features = Preconditions.checkNotNull(features); this.linkstamps = Preconditions.checkNotNull(linkstamps); this.linkstampCompileOptions = linkstampCompileOptions; + this.fdoBuildStamp = fdoBuildStamp; this.runtimeSolibDir = runtimeSolibDir; this.nativeDeps = nativeDeps; this.useTestOnlyFlags = useTestOnlyFlags; @@ -590,7 +594,6 @@ public final class LinkCommandLine extends CommandLine { } // Stamp FDO builds with FDO subtype string - String fdoBuildStamp = CppHelper.getFdoBuildStamp(cppConfiguration); if (fdoBuildStamp != null) { optionList.add("-D" + CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\""); } @@ -1052,8 +1055,7 @@ public final class LinkCommandLine extends CommandLine { } CcToolchainFeatures.Variables.Builder buildVariables = new CcToolchainFeatures.Variables.Builder(); - CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class); - cppConfiguration.getFdoSupport().getLinkOptions(featureConfiguration, buildVariables); + CppHelper.getFdoSupport(ruleContext).getLinkOptions(featureConfiguration, buildVariables); variables = buildVariables.build(); } return new LinkCommandLine( @@ -1071,6 +1073,7 @@ public final class LinkCommandLine extends CommandLine { features, linkstamps, actualLinkstampCompileOptions, + CppHelper.getFdoBuildStamp(ruleContext), runtimeSolibDir, nativeDeps, useTestOnlyFlags, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java index 9aab8b11de..0746b62134 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java @@ -98,7 +98,7 @@ public final class PrecomputedValue implements SkyValue { public static final Precomputed> BUILD_INFO_FACTORIES = new Precomputed<>(SkyKey.create(SkyFunctions.PRECOMPUTED, "build_info_factories")); - static final Precomputed BLAZE_DIRECTORIES = + public static final Precomputed BLAZE_DIRECTORIES = new Precomputed<>(SkyKey.create(SkyFunctions.PRECOMPUTED, "blaze_directories")); static final Precomputed> BAD_ACTIONS = diff --git a/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java index 393110d5f8..4830d8fe19 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/ZipFileSystem.java @@ -15,11 +15,15 @@ package com.google.devtools.build.lib.vfs; import com.google.common.base.Predicate; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; +import com.google.devtools.build.lib.util.Preconditions; +import java.io.Closeable; import java.io.File; import java.io.FileNotFoundException; +import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; +import java.io.OutputStream; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -31,9 +35,11 @@ import java.util.zip.ZipFile; * Inherits the constraints imposed by ReadonlyFileSystem. */ @ThreadSafe -public class ZipFileSystem extends ReadonlyFileSystem { +public class ZipFileSystem extends ReadonlyFileSystem implements Closeable { + private final File tempFile; // In case this needs to be written to the file system private final ZipFile zipFile; + private boolean open; /** * The sole purpose of this field is to hold a strong reference to all leaf @@ -53,14 +59,25 @@ public class ZipFileSystem extends ReadonlyFileSystem { * Constructs a ZipFileSystem from a zip file identified with a given path. */ public ZipFileSystem(Path zipPath) throws IOException { - // Throw some more specific exceptions than ZipFile does. - // We do this using File instead of Path, in case zipPath points to an - // InMemoryFileSystem. This case is not really supported but - // can occur in tests. + if (!zipPath.exists()) { + throw new FileNotFoundException(String.format("File '%s' does not exist", zipPath)); + } + File file = zipPath.getPathFile(); if (!file.exists()) { - throw new FileNotFoundException(String.format("File '%s' does not exist", zipPath)); + // If the File says that it does not exist but the Path says that it does, we are probably + // dealing with a FileSystem that does not represent the actual file system we are running + // under. Then we copy the Path into a temporary File. + tempFile = File.createTempFile("bazel.test.", ".tmp"); + file = tempFile; + byte[] contents = FileSystemUtils.readContent(zipPath); + try (OutputStream os = new FileOutputStream(tempFile)) { + os.write(contents); + } + } else { + tempFile = null; } + if (!file.isFile()) { throw new IOException(String.format("'%s' is not a file", zipPath)); } @@ -70,6 +87,7 @@ public class ZipFileSystem extends ReadonlyFileSystem { this.zipFile = new ZipFile(file); this.paths = populatePathTree(); + this.open = true; } // ZipPath extends Path with a set-once ZipEntry field. @@ -160,12 +178,14 @@ public class ZipFileSystem extends ReadonlyFileSystem { @Override protected InputStream getInputStream(Path path) throws IOException { + Preconditions.checkState(open); return zipFile.getInputStream(zipEntryNonNull(path)); } @Override protected Collection getDirectoryEntries(Path path) throws IOException { + Preconditions.checkState(open); zipEntryNonNull(path); final Collection result = new ArrayList<>(); ((ZipPath) path).applyToChildren(new Predicate() { @@ -182,46 +202,54 @@ public class ZipFileSystem extends ReadonlyFileSystem { @Override protected boolean exists(Path path, boolean followSymlinks) { + Preconditions.checkState(open); return zipEntry(path) != null; } @Override protected boolean isDirectory(Path path, boolean followSymlinks) { + Preconditions.checkState(open); ZipEntry entry = zipEntry(path); return entry != null && entry.isDirectory(); } @Override protected boolean isFile(Path path, boolean followSymlinks) { + Preconditions.checkState(open); ZipEntry entry = zipEntry(path); return entry != null && !entry.isDirectory(); } @Override protected boolean isSpecialFile(Path path, boolean followSymlinks) { + Preconditions.checkState(open); return false; } @Override protected boolean isReadable(Path path) throws IOException { + Preconditions.checkState(open); zipEntryNonNull(path); return true; } @Override protected boolean isWritable(Path path) throws IOException { + Preconditions.checkState(open); zipEntryNonNull(path); return false; } @Override protected boolean isExecutable(Path path) throws IOException { + Preconditions.checkState(open); zipEntryNonNull(path); return false; } @Override protected PathFragment readSymbolicLink(Path path) throws IOException { + Preconditions.checkState(open); zipEntryNonNull(path); throw new NotASymlinkException(path); } @@ -229,22 +257,26 @@ public class ZipFileSystem extends ReadonlyFileSystem { @Override protected long getFileSize(Path path, boolean followSymlinks) throws IOException { + Preconditions.checkState(open); return zipEntryNonNull(path).getSize(); } @Override protected long getLastModifiedTime(Path path, boolean followSymlinks) throws FileNotFoundException { + Preconditions.checkState(open); return zipEntryNonNull(path).getTime(); } @Override protected boolean isSymbolicLink(Path path) { + Preconditions.checkState(open); return false; } @Override protected FileStatus statIfFound(Path path, boolean followSymlinks) { + Preconditions.checkState(open); try { return stat(path, followSymlinks); } catch (FileNotFoundException e) { @@ -255,4 +287,14 @@ public class ZipFileSystem extends ReadonlyFileSystem { } } + @Override + public void close() { + if (open) { + close(); + if (tempFile != null) { + tempFile.delete(); + } + open = false; + } + } } diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index 291ec1f600..8668cfac31 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/src/test/java/com/google/devtools/build/lib/BUILD @@ -99,6 +99,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:util", "//src/main/java/com/google/devtools/build/lib:vfs", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/skyframe", "//third_party:guava", "//third_party:guava-testlib", @@ -1054,6 +1055,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib:syntax", "//src/main/java/com/google/devtools/build/lib:vfs", "//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader", + "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/skyframe", "//third_party:guava", "//third_party:guava-testlib", diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 86c2f9727b..670780c611 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -16,7 +16,9 @@ package com.google.devtools.build.lib.analysis.mock; import com.google.common.base.Functions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList.Builder; +import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; +import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfigurationCollectionFactory; import com.google.devtools.build.lib.analysis.config.ConfigurationFactory; import com.google.devtools.build.lib.analysis.util.AnalysisMock; @@ -32,6 +34,8 @@ import com.google.devtools.build.lib.packages.util.MockToolsConfig; import com.google.devtools.build.lib.rules.android.AndroidConfiguration; import com.google.devtools.build.lib.rules.apple.AppleConfiguration; import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader; +import com.google.devtools.build.lib.rules.cpp.FdoSupportFunction; +import com.google.devtools.build.lib.rules.cpp.FdoSupportValue; import com.google.devtools.build.lib.rules.java.JavaConfigurationLoader; import com.google.devtools.build.lib.rules.java.JvmConfigurationLoader; import com.google.devtools.build.lib.rules.objc.J2ObjcConfiguration; @@ -42,6 +46,8 @@ import com.google.devtools.build.lib.testutil.BuildRuleWithDefaultsBuilder; import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.skyframe.SkyFunction; +import com.google.devtools.build.skyframe.SkyFunctionName; import java.io.IOException; import java.io.InputStream; @@ -221,4 +227,12 @@ public final class BazelAnalysisMock extends AnalysisMock { public MockCcSupport ccSupport() { return BazelMockCcSupport.INSTANCE; } + + @Override + public ImmutableMap getSkyFunctions(BlazeDirectories directories) { + ImmutableMap.Builder skyFunctions = ImmutableMap.builder(); + skyFunctions.putAll(super.getSkyFunctions(directories)); + skyFunctions.put(FdoSupportValue.SKYFUNCTION, new FdoSupportFunction()); + return skyFunctions.build(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java index ba70915558..e9e9844082 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java @@ -134,5 +134,11 @@ public abstract class AnalysisMock { public Collection getOptionOverrides() { return delegate.getOptionOverrides(); } + + @Override + public ImmutableMap getSkyFunctions( + BlazeDirectories directories) { + return delegate.getSkyFunctions(directories); + } } } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index abb7a79772..a690340f7c 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -1503,7 +1503,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { @Override public Artifact getEmbeddedToolArtifact(String embeddedPath) { - throw new UnsupportedOperationException(); + return null; } @Override diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java index 5e677f47a6..12683ace4e 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java @@ -29,6 +29,8 @@ import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.util.MockCcSupport; import com.google.devtools.build.lib.packages.util.MockToolsConfig; +import com.google.devtools.build.lib.rules.cpp.FdoSupportFunction; +import com.google.devtools.build.lib.rules.cpp.FdoSupportValue; import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction; import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; @@ -86,7 +88,8 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase { new RepositoryDelegatorFunction( directories, repositoryHandlers, skylarkRepositoryFunction, new AtomicBoolean(true)), SkyFunctions.REPOSITORY, - new RepositoryLoaderFunction()); + new RepositoryLoaderFunction(), + FdoSupportValue.SKYFUNCTION, new FdoSupportFunction()); } @Override diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index 679d75de8d..abb31f7d11 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.cpp; -import static com.google.devtools.build.lib.actions.util.ActionsTestUtil.NULL_ACTION_OWNER; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -25,11 +24,9 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ResourceSet; import com.google.devtools.build.lib.actions.Root; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; -import com.google.devtools.build.lib.analysis.AnalysisEnvironment; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.util.ActionTester; import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory; -import com.google.devtools.build.lib.analysis.util.AnalysisTestUtil; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -218,19 +215,14 @@ public class CppLinkActionTest extends BuildViewTestCase { || resources.getIoUsage() == scaledSet.getIoUsage()); } private Builder createLinkBuilder(Link.LinkTargetType type, String outputPath, - Iterable nonLibraryInputs, ImmutableList libraryInputs) { - return createLinkBuilder(type, outputPath, nonLibraryInputs, libraryInputs, - AnalysisTestUtil.STUB_ANALYSIS_ENVIRONMENT); - } - - private Builder createLinkBuilder(Link.LinkTargetType type, String outputPath, - Iterable nonLibraryInputs, ImmutableList libraryInputs, - AnalysisEnvironment analysisEnv) { - Builder builder = CppLinkAction.Builder.createTestBuilder( - NULL_ACTION_OWNER, - analysisEnv, + Iterable nonLibraryInputs, ImmutableList libraryInputs) + throws Exception { + RuleContext ruleContext = createDummyRuleContext(); + Builder builder = new CppLinkAction.Builder( + ruleContext, new Artifact(new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()), - getTargetConfiguration()) + ruleContext.getConfiguration(), + null) .addNonLibraryInputs(nonLibraryInputs) .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs)) .setLinkType(type) -- cgit v1.2.3