Skip to content

Commit

Permalink
Move FdoProvider under CcToolchainProvider
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hlopko authored and Copybara-Service committed Sep 5, 2018
1 parent 5939235 commit 13d70a9
Show file tree
Hide file tree
Showing 13 changed files with 56 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,7 @@ protected static CompilationInfo compile(
convertFromNoneable(skylarkFeatureConfiguration, null);
Pair<List<Artifact>, List<Artifact>> 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.
Expand Down Expand Up @@ -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<String> linkopts =
convertSkylarkListOrNestedSetToNestedSet(skylarkLinkopts, String.class);
CcLinkingHelper helper =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public final class CcToolchainProvider extends ToolchainInfo implements CcToolch
/* builtInIncludeDirectories= */ ImmutableList.<PathFragment>of(),
/* sysroot= */ null,
FdoMode.OFF,
/* fdoProvider= */ null,
/* useLLVMCoverageMapFormat= */ false,
/* codeCoverageEnabled= */ false,
/* isHostConfiguration= */ false);
Expand Down Expand Up @@ -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<String, Object> values,
Expand Down Expand Up @@ -153,6 +160,7 @@ public CcToolchainProvider(
ImmutableList<PathFragment> builtInIncludeDirectories,
@Nullable PathFragment sysroot,
FdoMode fdoMode,
FdoProvider fdoProvider,
boolean useLLVMCoverageMapFormat,
boolean codeCoverageEnabled,
boolean isHostConfiguration) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -693,6 +702,10 @@ public ImmutableMap<String, String> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,25 +247,6 @@ public static ImmutableList<String> getDynamicLinkOptions(
}
}

/**
* Return {@link FdoProvider} using default cc_toolchain attribute name.
*
* <p>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<Pair<String, String>> getCoverageEnvironmentIfNeeded(
RuleContext ruleContext, CcToolchainProvider toolchain) {
if (ruleContext.getConfiguration().isCodeCoverageEnabled()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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();
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -161,7 +160,7 @@ public final ConfiguredTarget create(RuleContext ruleContext)
objcProvider,
intermediateArtifacts.strippedSingleArchitectureLibrary(),
childConfigurationsAndToolchains.get(childConfig),
CppHelper.getFdoProviderUsingDefaultCcToolchainAttribute(ruleContext))
childToolchain.getFdoProvider())
.validateAttributes();
ruleContext.assertNoErrors();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -960,7 +960,7 @@ CompilationSupport registerCompileAndArchiveActions(
ExtraCompileArgs.NONE,
ImmutableList.<PathFragment>of(),
toolchain,
maybeGetFdoProvider());
toolchain.getFdoProvider());
}

/**
Expand Down Expand Up @@ -1083,7 +1083,7 @@ CompilationSupport registerCompileAndArchiveActions(
extraCompileArgs,
priorityHeaders,
toolchain,
maybeGetFdoProvider());
toolchain.getFdoProvider());
}
return this;
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -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<Artifact> getCustomModuleMap(RuleContext ruleContext) {
if (ruleContext.attributes().has("module_map", BuildType.LABEL)) {
return Optional.fromNullable(ruleContext.getPrerequisiteArtifact("module_map", Mode.TARGET));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public void equalityIsObjectIdentity() throws Exception {
/* builtInIncludeDirectories= */ ImmutableList.<PathFragment>of(),
/* sysroot= */ null,
FdoMode.OFF,
/* fdoProvider= */ null,
/* useLLVMCoverageMapFormat= */ false,
/* codeCoverageEnabled= */ false,
/* isHostConfiguration= */ false);
Expand Down Expand Up @@ -114,6 +115,7 @@ public void equalityIsObjectIdentity() throws Exception {
/* builtInIncludeDirectories= */ ImmutableList.<PathFragment>of(),
/* sysroot= */ null,
FdoMode.OFF,
/* fdoProvider= */ null,
/* useLLVMCoverageMapFormat= */ false,
/* codeCoverageEnabled= */ false,
/* isHostConfiguration= */ false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,14 +309,16 @@ public void testComputeKeyNonStatic() throws Exception {
@Override
public Action generate(ImmutableSet<NonStaticAttributes> 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)) {
Expand Down Expand Up @@ -365,14 +367,16 @@ public void testComputeKeyStatic() throws Exception {
@Override
public Action generate(ImmutableSet<StaticKeyAttributes> 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(
Expand All @@ -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);
Expand Down Expand Up @@ -487,6 +493,8 @@ private CppLinkActionBuilder createLinkBuilder(
FeatureConfiguration featureConfiguration)
throws Exception {
RuleContext ruleContext = createDummyRuleContext();
CcToolchainProvider toolchain =
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
CppLinkActionBuilder builder =
new CppLinkActionBuilder(
ruleContext,
Expand All @@ -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)
Expand Down

0 comments on commit 13d70a9

Please sign in to comment.