From 13d70a93dd795f65149a0000ee6076b412d67aa9 Mon Sep 17 00:00:00 2001 From: hlopko Date: Wed, 5 Sep 2018 08:13:10 -0700 Subject: [PATCH] Move FdoProvider under CcToolchainProvider Currently FdoProfiler is provided by the cc_toolchain separately. This causes problems to custom skylark rules that store the implicit attribute as _cc_toolchain, and native rules store it as ':cc_toolchain'. And FdoProvider was expected to be provided by the ':cc_toolchain'. Since FdoProvider is used together with the CcToolchainProvider, I'm merging them together. #6072. RELNOTES: None. PiperOrigin-RevId: 211635916 --- .../build/lib/rules/cpp/CcCommon.java | 4 +--- .../build/lib/rules/cpp/CcImport.java | 3 +-- .../build/lib/rules/cpp/CcModule.java | 6 ++--- .../build/lib/rules/cpp/CcToolchain.java | 13 +++++----- .../lib/rules/cpp/CcToolchainProvider.java | 13 ++++++++++ .../build/lib/rules/cpp/CppHelper.java | 19 --------------- .../lib/rules/cpp/proto/CcProtoAspect.java | 12 ++++++---- .../rules/nativedeps/NativeDepsHelper.java | 3 +-- .../lib/rules/objc/AppleStaticLibrary.java | 3 +-- .../lib/rules/objc/CompilationSupport.java | 22 ++++------------- .../build/lib/rules/objc/J2ObjcAspect.java | 3 +-- .../rules/cpp/CcToolchainProviderTest.java | 2 ++ .../lib/rules/cpp/CppLinkActionTest.java | 24 ++++++++++++------- 13 files changed, 56 insertions(+), 71 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 41b0dfd6ff23b5..7ab21025f52b10 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 @@ -166,9 +166,7 @@ public CcCommon(RuleContext ruleContext) { this.ccToolchain = Preconditions.checkNotNull( CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext)); - this.fdoProvider = - Preconditions.checkNotNull( - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext)); + this.fdoProvider = ccToolchain.getFdoProvider(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java index 14a4092f694412..2ca3dea601b199 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcImport.java @@ -69,8 +69,7 @@ public ConfiguredTarget create(RuleContext ruleContext) CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext); FeatureConfiguration featureConfiguration = CcCommon.configureFeaturesOrReportRuleError(ruleContext, ccToolchain); - FdoProvider fdoProvider = - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); + FdoProvider fdoProvider = ccToolchain.getFdoProvider(); // Add headers to compilation step. final CcCommon common = new CcCommon(ruleContext); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java index 1fc0fe2f598eb9..aa7ea76cac5ce6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java @@ -418,8 +418,7 @@ protected static CompilationInfo compile( convertFromNoneable(skylarkFeatureConfiguration, null); Pair, List> separatedHeadersAndSources = separateSourcesFromHeaders(sources); - FdoProvider fdoProvider = - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); + FdoProvider fdoProvider = ccToolchainProvider.getFdoProvider(); // TODO(plf): Need to flatten the nested set to convert the Strings to PathFragment. This could // be avoided if path fragments are ever added to Skylark or in the C++ code we take Strings // instead of PathFragments. @@ -497,8 +496,7 @@ protected static LinkingInfo link( CcToolchainProvider ccToolchainProvider = convertFromNoneable(skylarkCcToolchainProvider, null); FeatureConfiguration featureConfiguration = convertFromNoneable(skylarkFeatureConfiguration, null); - FdoProvider fdoProvider = - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); + FdoProvider fdoProvider = ccToolchainProvider.getFdoProvider(); NestedSet linkopts = convertSkylarkListOrNestedSetToNestedSet(skylarkLinkopts, String.class); CcLinkingHelper helper = 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 7b2b9e6b863af7..b6eae3c87ea363 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 @@ -585,6 +585,12 @@ public ConfiguredTarget create(RuleContext ruleContext) builtInIncludeDirectories, sysroot, fdoMode, + new FdoProvider( + fdoSupport.getFilePath(), + fdoMode, + cppConfiguration.getFdoInstrument(), + profileArtifact, + prefetchHintsArtifact), cppConfiguration.useLLVMCoverageMapFormat(), configuration.isCodeCoverageEnabled(), configuration.isHostConfiguration()); @@ -597,13 +603,6 @@ public ConfiguredTarget create(RuleContext ruleContext) new RuleConfiguredTargetBuilder(ruleContext) .addNativeDeclaredProvider(ccProvider) .addNativeDeclaredProvider(templateVariableInfo) - .addProvider( - new FdoProvider( - fdoSupport.getFilePath(), - fdoMode, - cppConfiguration.getFdoInstrument(), - profileArtifact, - prefetchHintsArtifact)) .setFilesToBuild(crosstool) .addProvider(RunfilesProvider.simple(Runfiles.EMPTY)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java index 5ac50d33b7c93c..bdeacf09c3cf97 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java @@ -81,6 +81,7 @@ public final class CcToolchainProvider extends ToolchainInfo implements CcToolch /* builtInIncludeDirectories= */ ImmutableList.of(), /* sysroot= */ null, FdoMode.OFF, + /* fdoProvider= */ null, /* useLLVMCoverageMapFormat= */ false, /* codeCoverageEnabled= */ false, /* isHostConfiguration= */ false); @@ -120,6 +121,12 @@ public final class CcToolchainProvider extends ToolchainInfo implements CcToolch private final boolean isHostConfiguration; private final boolean forcePic; private final boolean shouldStripBinaries; + /** + * WARNING: We don't like {@link FdoProvider}. Its {@link FdoProvider#fdoProfilePath} is pure + * path and that is horrible as it breaks many Bazel assumptions! Don't do bad stuff with it, + * don't take inspiration from it. + */ + private final FdoProvider fdoProvider; public CcToolchainProvider( ImmutableMap values, @@ -153,6 +160,7 @@ public CcToolchainProvider( ImmutableList builtInIncludeDirectories, @Nullable PathFragment sysroot, FdoMode fdoMode, + FdoProvider fdoProvider, boolean useLLVMCoverageMapFormat, boolean codeCoverageEnabled, boolean isHostConfiguration) { @@ -188,6 +196,7 @@ public CcToolchainProvider( this.builtInIncludeDirectories = builtInIncludeDirectories; this.sysroot = sysroot; this.fdoMode = fdoMode; + this.fdoProvider = fdoProvider; this.useLLVMCoverageMapFormat = useLLVMCoverageMapFormat; this.codeCoverageEnabled = codeCoverageEnabled; this.isHostConfiguration = isHostConfiguration; @@ -693,6 +702,10 @@ public ImmutableMap getAdditionalMakeVariables() { return toolchainInfo.getAdditionalMakeVariables(); } + public FdoProvider getFdoProvider() { + return fdoProvider; + } + /** * Returns whether the toolchain supports "Fission" C++ builds, i.e. builds where compilation * partitions object code and debug symbols into separate output files. 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 e8ccf338115488..477a9ef19fb309 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 @@ -247,25 +247,6 @@ public static ImmutableList getDynamicLinkOptions( } } - /** - * Return {@link FdoProvider} using default cc_toolchain attribute name. - * - *

Be careful to provide explicit attribute name if the rule doesn't store cc_toolchain under - * the default name. - */ - @Nullable - public static FdoProvider getFdoProviderUsingDefaultCcToolchainAttribute( - RuleContext ruleContext) { - return getFdoProvider(ruleContext, CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME); - } - - @Nullable public static FdoProvider getFdoProvider(RuleContext ruleContext, - String ccToolchainAttribute) { - return ruleContext - .getPrerequisite(ccToolchainAttribute, Mode.TARGET) - .getProvider(FdoProvider.class); - } - public static NestedSet> getCoverageEnvironmentIfNeeded( RuleContext ruleContext, CcToolchainProvider toolchain) { if (ruleContext.getConfiguration().isCodeCoverageEnabled()) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java index 95438a41894c22..a4871d5a62d81f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/proto/CcProtoAspect.java @@ -237,13 +237,14 @@ private FeatureConfiguration getFeatureConfiguration(SupportData supportData) { private CcCompilationHelper initializeCompilationHelper( FeatureConfiguration featureConfiguration) { + CcToolchainProvider toolchain = ccToolchain(ruleContext); CcCompilationHelper helper = new CcCompilationHelper( ruleContext, cppSemantics, featureConfiguration, - ccToolchain(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext)); + toolchain, + toolchain.getFdoProvider()); TransitiveInfoCollection runtime = getProtoToolchainProvider().runtime(); if (runtime != null) { helper.addDeps(ImmutableList.of(runtime)); @@ -257,13 +258,14 @@ private CcCompilationHelper initializeCompilationHelper( } private CcLinkingHelper initializeLinkingHelper(FeatureConfiguration featureConfiguration) { + CcToolchainProvider toolchain = ccToolchain(ruleContext); CcLinkingHelper helper = new CcLinkingHelper( ruleContext, cppSemantics, featureConfiguration, - ccToolchain(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext), + toolchain, + toolchain.getFdoProvider(), ruleContext.getConfiguration()) .enableCcNativeLibrariesProvider(); TransitiveInfoCollection runtime = getProtoToolchainProvider().runtime(); @@ -273,7 +275,7 @@ private CcLinkingHelper initializeLinkingHelper(FeatureConfiguration featureConf helper.addDeps(ruleContext.getPrerequisites("deps", TARGET)); // TODO(dougk): Configure output artifact with action_config // once proto compile action is configurable from the crosstool. - if (!ccToolchain(ruleContext).supportsDynamicLinker()) { + if (!toolchain.supportsDynamicLinker()) { helper.setShouldCreateDynamicLibrary(false); } return helper; diff --git a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java index 89e8c481b26e80..d480356c2d7bab 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/nativedeps/NativeDepsHelper.java @@ -216,8 +216,7 @@ public static NativeDepsRunfiles createNativeDepsAction( } else { sharedLibrary = nativeDeps; } - FdoProvider fdoProvider = - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); + FdoProvider fdoProvider = toolchain.getFdoProvider(); FeatureConfiguration featureConfiguration = CcCommon.configureFeaturesOrReportRuleError( ruleContext, diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java index c321398e5222c7..8f0a4d5a5a0d2f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStaticLibrary.java @@ -35,7 +35,6 @@ import com.google.devtools.build.lib.rules.apple.ApplePlatform.PlatformType; import com.google.devtools.build.lib.rules.cpp.CcLinkingInfo; import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider; -import com.google.devtools.build.lib.rules.cpp.CppHelper; import com.google.devtools.build.lib.rules.objc.ObjcProvider.Key; import com.google.devtools.build.lib.rules.proto.ProtoSourcesProvider; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; @@ -161,7 +160,7 @@ public final ConfiguredTarget create(RuleContext ruleContext) objcProvider, intermediateArtifacts.strippedSingleArchitectureLibrary(), childConfigurationsAndToolchains.get(childConfig), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext)) + childToolchain.getFdoProvider()) .validateAttributes(); ruleContext.assertNoErrors(); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 005b5dc694808d..1fcd24261fab9f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -960,7 +960,7 @@ CompilationSupport registerCompileAndArchiveActions( ExtraCompileArgs.NONE, ImmutableList.of(), toolchain, - maybeGetFdoProvider()); + toolchain.getFdoProvider()); } /** @@ -1083,7 +1083,7 @@ CompilationSupport registerCompileAndArchiveActions( extraCompileArgs, priorityHeaders, toolchain, - maybeGetFdoProvider()); + toolchain.getFdoProvider()); } return this; } @@ -1159,8 +1159,7 @@ CompilationSupport registerLinkActions( .addVariableCategory(VariableCategory.EXECUTABLE_LINKING_VARIABLES); Artifact binaryToLink = getBinaryToLink(); - FdoProvider fdoProvider = - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); + FdoProvider fdoProvider = toolchain.getFdoProvider(); CppLinkActionBuilder executableLinkAction = new CppLinkActionBuilder( ruleContext, @@ -1294,7 +1293,8 @@ private CompilationSupport registerObjFilelistAction( */ public CompilationSupport registerFullyLinkAction( ObjcProvider objcProvider, Artifact outputArchive) throws InterruptedException { - return registerFullyLinkAction(objcProvider, outputArchive, toolchain, maybeGetFdoProvider()); + return registerFullyLinkAction( + objcProvider, outputArchive, toolchain, toolchain.getFdoProvider()); } /** @@ -1862,18 +1862,6 @@ private void registerHeaderScanningAction( return objcHeaderThinningInfoByCommandLine; } - @Nullable - private FdoProvider maybeGetFdoProvider() { - // TODO(rduan): Remove this check once all rules are using the crosstool support. - if (ruleContext - .attributes() - .has(CcToolchain.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME, BuildType.LABEL)) { - return CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext); - } else { - return null; - } - } - public static Optional getCustomModuleMap(RuleContext ruleContext) { if (ruleContext.attributes().has("module_map", BuildType.LABEL)) { return Optional.fromNullable(ruleContext.getPrerequisiteArtifact("module_map", Mode.TARGET)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java index 919024ab80f342..073a40798b5a86 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/J2ObjcAspect.java @@ -257,8 +257,7 @@ private ConfiguredAspect buildAspect( try { CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext, ":j2objc_cc_toolchain"); - FdoProvider fdoProvider = - CppHelper.getFdoProvider(ruleContext, ":j2objc_cc_toolchain"); + FdoProvider fdoProvider = ccToolchain.getFdoProvider(); CompilationSupport compilationSupport = new CompilationSupport.Builder() .setRuleContext(ruleContext) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java index 897d4b27d9406f..c7804b2002539c 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderTest.java @@ -77,6 +77,7 @@ public void equalityIsObjectIdentity() throws Exception { /* builtInIncludeDirectories= */ ImmutableList.of(), /* sysroot= */ null, FdoMode.OFF, + /* fdoProvider= */ null, /* useLLVMCoverageMapFormat= */ false, /* codeCoverageEnabled= */ false, /* isHostConfiguration= */ false); @@ -114,6 +115,7 @@ public void equalityIsObjectIdentity() throws Exception { /* builtInIncludeDirectories= */ ImmutableList.of(), /* sysroot= */ null, FdoMode.OFF, + /* fdoProvider= */ null, /* useLLVMCoverageMapFormat= */ false, /* codeCoverageEnabled= */ false, /* isHostConfiguration= */ false); 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 af79589f813b2d..e2d013ccbb8fd8 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 @@ -309,14 +309,16 @@ public void testComputeKeyNonStatic() throws Exception { @Override public Action generate(ImmutableSet attributesToFlip) throws InterruptedException { + CcToolchainProvider toolchain = + CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext); CppLinkActionBuilder builder = new CppLinkActionBuilder( ruleContext, attributesToFlip.contains(NonStaticAttributes.OUTPUT_FILE) ? dynamicOutputFile : staticOutputFile, - CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext), + toolchain, + toolchain.getFdoProvider(), featureConfiguration, MockCppSemantics.INSTANCE) {}; if (attributesToFlip.contains(NonStaticAttributes.OUTPUT_FILE)) { @@ -365,14 +367,16 @@ public void testComputeKeyStatic() throws Exception { @Override public Action generate(ImmutableSet attributes) throws InterruptedException { + CcToolchainProvider toolchain = + CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext); CppLinkActionBuilder builder = new CppLinkActionBuilder( ruleContext, attributes.contains(StaticKeyAttributes.OUTPUT_FILE) ? staticOutputFile : dynamicOutputFile, - CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext), + toolchain, + toolchain.getFdoProvider(), featureConfiguration, MockCppSemantics.INSTANCE) {}; builder.setLinkType( @@ -397,12 +401,14 @@ public void testCommandLineSplitting() throws Exception { PathFragment.create("output/path.ifso"), getTargetConfiguration().getBinDirectory( RepositoryName.MAIN), ActionsTestUtil.NULL_ARTIFACT_OWNER); + CcToolchainProvider toolchain = + CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext); CppLinkActionBuilder builder = new CppLinkActionBuilder( ruleContext, output, - CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext), + toolchain, + toolchain.getFdoProvider(), FeatureConfiguration.EMPTY, MockCppSemantics.INSTANCE); builder.setLinkType(LinkTargetType.STATIC_LIBRARY); @@ -487,6 +493,8 @@ private CppLinkActionBuilder createLinkBuilder( FeatureConfiguration featureConfiguration) throws Exception { RuleContext ruleContext = createDummyRuleContext(); + CcToolchainProvider toolchain = + CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext); CppLinkActionBuilder builder = new CppLinkActionBuilder( ruleContext, @@ -495,8 +503,8 @@ private CppLinkActionBuilder createLinkBuilder( getTargetConfiguration() .getBinDirectory(ruleContext.getRule().getRepository())), ruleContext.getConfiguration(), - CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext), - CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext), + toolchain, + toolchain.getFdoProvider(), featureConfiguration, MockCppSemantics.INSTANCE) .addObjectFiles(nonLibraryInputs)