Skip to content

Commit

Permalink
Remove cppConfiguration from LinkBuildVariables
Browse files Browse the repository at this point in the history
This is to simplify the API that will eventually be exposed to Skylark.

Working towards #4571.

RELNOTES: None.
PiperOrigin-RevId: 195785588
  • Loading branch information
hlopko authored and Copybara-Service committed May 8, 2018
1 parent 0c5cc1b commit 0a502ff
Show file tree
Hide file tree
Showing 10 changed files with 59 additions and 91 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
ruleContext,
ccCompilationOutputs,
linkingMode,
CppHelper.useFission(cppConfiguration, ccToolchain),
ccToolchain.useFission(),
usePic,
ltoBackendArtifacts);
Artifact dwpFile =
Expand All @@ -438,14 +438,14 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont

// The debug package should include the dwp file only if it was explicitly requested.
Artifact explicitDwpFile = dwpFile;
if (!CppHelper.useFission(cppConfiguration, ccToolchain)) {
if (!ccToolchain.useFission()) {
explicitDwpFile = null;
} else {
// For cc_test rules, include the dwp in the runfiles if Fission is enabled and the test was
// built statically.
if (TargetUtils.isTestRule(ruleContext.getRule())
&& linkingMode != Link.LinkingMode.DYNAMIC
&& CppHelper.useFission(cppConfiguration, ccToolchain)
&& ccToolchain.useFission()
&& cppConfiguration.buildTestDwpIsActivated()) {
filesToBuild = NestedSetBuilder.fromNestedSet(filesToBuild).add(dwpFile).build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
allFeatures.add("nonhost");
}

if (CppHelper.useFission(cppConfiguration, toolchain)) {
if (toolchain.useFission()) {
allFeatures.add(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1362,9 +1362,7 @@ private CcCompilationOutputs createCcCompileActions() throws RuleErrorException
// The source action does not generate dwo when it has bitcode
// output (since it isn't generating a native object with debug
// info). In that case the LtoBackendAction will generate the dwo.
CppHelper.shouldCreatePerObjectDebugInfo(
cppConfiguration, ccToolchain, featureConfiguration)
&& !bitcodeOutput,
ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration) && !bitcodeOutput,
isGenerateDotdFile(sourceArtifact));
break;
}
Expand Down Expand Up @@ -1600,9 +1598,7 @@ private void createModuleCodegenAction(
? CppHelper.getCompileOutputArtifact(ruleContext, gcnoFileName, configuration)
: null;

boolean generateDwo =
CppHelper.shouldCreatePerObjectDebugInfo(
cppConfiguration, ccToolchain, featureConfiguration);
boolean generateDwo = ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration);
Artifact dwoFile = generateDwo ? getDwoFile(builder.getOutputFile()) : null;
// TODO(tejohnson): Add support for ThinLTO if needed.
boolean bitcodeOutput =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ public final class CcToolchainProvider extends ToolchainInfo {
private final boolean useLLVMCoverageMapFormat;
private final boolean codeCoverageEnabled;
private final boolean isHostConfiguration;
private final boolean forcePic;
private final boolean shouldStripBinaries;

public CcToolchainProvider(
ImmutableMap<String, Object> values,
Expand Down Expand Up @@ -191,6 +193,13 @@ public CcToolchainProvider(
this.useLLVMCoverageMapFormat = useLLVMCoverageMapFormat;
this.codeCoverageEnabled = codeCoverageEnabled;
this.isHostConfiguration = isHostConfiguration;
if (cppConfiguration != null) {
this.forcePic = cppConfiguration.forcePic();
this.shouldStripBinaries = cppConfiguration.shouldStripBinaries();
} else {
this.forcePic = false;
this.shouldStripBinaries = false;
}
}

/** Returns c++ Make variables. */
Expand Down Expand Up @@ -249,6 +258,23 @@ public static Map<String, String> getCppBuildVariables(
return result.build();
}

/**
* Returns true if Fission is specified and supported by the CROSSTOOL for the build implied by
* the given configuration and toolchain.
*/
public boolean useFission() {
return Preconditions.checkNotNull(cppConfiguration).fissionIsActiveForCurrentCompilationMode()
&& supportsFission();
}

/**
* Returns true if Fission and PER_OBJECT_DEBUG_INFO are specified and supported by the CROSSTOOL
* for the build implied by the given configuration, toolchain and feature configuration.
*/
public boolean shouldCreatePerObjectDebugInfo(FeatureConfiguration featureConfiguration) {
return useFission() && featureConfiguration.isEnabled(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
}

@Override
public void addGlobalMakeVariables(ImmutableMap.Builder<String, String> globalMakeEnvBuilder) {
globalMakeEnvBuilder.putAll(
Expand Down Expand Up @@ -1021,5 +1047,13 @@ public boolean isCodeCoverageEnabled() {
public boolean isHostConfiguration() {
return isHostConfiguration;
}

public boolean getForcePic() {
return forcePic;
}

public boolean getShouldStripBinaries() {
return shouldStripBinaries;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -1081,23 +1081,4 @@ public static boolean useInterfaceSharedObjects(
return toolchain.supportsInterfaceSharedObjects() && config.getUseInterfaceSharedObjects();
}

/**
* Returns true if Fission is specified and supported by the CROSSTOOL for the build implied by
* the given configuration and toolchain.
*/
public static boolean useFission(CppConfiguration config, CcToolchainProvider toolchain) {
return config.fissionIsActiveForCurrentCompilationMode() && toolchain.supportsFission();
}

/**
* Returns true if Fission and PER_OBJECT_DEBUG_INFO are specified and supported by the CROSSTOOL
* for the build implied by the given configuration, toolchain and feature configuration.
*/
public static boolean shouldCreatePerObjectDebugInfo(
CppConfiguration config,
CcToolchainProvider toolchain,
FeatureConfiguration featureConfiguration) {
return useFission(config, toolchain)
&& featureConfiguration.isEnabled(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,7 @@ private LtoBackendArtifacts createLtoArtifact(
toolchain,
fdoSupport,
usePicForLtoBackendActions,
CppHelper.shouldCreatePerObjectDebugInfo(
cppConfiguration, toolchain, featureConfiguration),
toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
argv)
: new LtoBackendArtifacts(
ltoOutputRootPrefix,
Expand All @@ -503,8 +502,7 @@ private LtoBackendArtifacts createLtoArtifact(
toolchain,
fdoSupport,
usePicForLtoBackendActions,
CppHelper.shouldCreatePerObjectDebugInfo(
cppConfiguration, toolchain, featureConfiguration),
toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration),
argv);
return ltoArtifact;
}
Expand Down Expand Up @@ -1004,7 +1002,6 @@ public CppLinkAction build() throws InterruptedException {
thinltoMergedObjectFile,
mustKeepDebug,
symbolCounts,
cppConfiguration,
toolchain,
featureConfiguration,
useTestOnlyFlags,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ public static Variables setupVariables(
Artifact thinltoMergedObjectFile,
boolean mustKeepDebug,
Artifact symbolCounts,
CppConfiguration cppConfiguration,
CcToolchainProvider ccToolchainProvider,
FeatureConfiguration featureConfiguration,
boolean useTestOnlyFlags,
Expand All @@ -123,17 +122,16 @@ public static Variables setupVariables(
}

// pic
if (cppConfiguration.forcePic()) {
if (ccToolchainProvider.getForcePic()) {
buildVariables.addStringVariable(FORCE_PIC.getVariableName(), "");
}

if (!mustKeepDebug && cppConfiguration.shouldStripBinaries()) {
if (!mustKeepDebug && ccToolchainProvider.getShouldStripBinaries()) {
buildVariables.addStringVariable(STRIP_DEBUG_SYMBOLS.getVariableName(), "");
}

if (isUsingLinkerNotArchiver
&& CppHelper.shouldCreatePerObjectDebugInfo(
cppConfiguration, ccToolchainProvider, featureConfiguration)) {
&& ccToolchainProvider.shouldCreatePerObjectDebugInfo(featureConfiguration)) {
buildVariables.addStringVariable(IS_USING_FISSION.getVariableName(), "");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
// TODO(b/64384912): Remove in favor of CcToolchainProvider
boolean stripAsDefault =
CppHelper.useFission(cppConfiguration, ccToolchain)
&& cppConfiguration.getCompilationMode() == CompilationMode.OPT;
ccToolchain.useFission() && cppConfiguration.getCompilationMode() == CompilationMode.OPT;
DeployArchiveBuilder unstrippedDeployArchiveBuilder = null;
if (stripAsDefault) {
unstrippedDeployArchiveBuilder = new DeployArchiveBuilder(semantics, ruleContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables.VariablesExtension;
import com.google.devtools.build.lib.rules.cpp.CcToolchainProvider;
import com.google.devtools.build.lib.rules.cpp.CppCompileAction;
import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
import com.google.devtools.build.lib.rules.cpp.CppHelper;
import com.google.devtools.build.lib.rules.cpp.CppLinkAction;
Expand Down Expand Up @@ -542,7 +541,7 @@ private FeatureConfiguration getFeatureConfiguration(
if (objcProvider.is(Flag.USES_OBJC)) {
activatedCrosstoolSelectables.add(CONTAINS_OBJC);
}
if (CppHelper.useFission(ruleContext.getFragment(CppConfiguration.class), toolchain)) {
if (toolchain.useFission()) {
activatedCrosstoolSelectables.add(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,101 +180,65 @@ public void testFission() throws Exception {
CcToolchainProvider toolchainProvider =
(CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);

assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

// Mode-specific settings.
useConfiguration("-c", "dbg", "--fission=dbg");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isTrue();
assertThat(toolchainProvider.useFission()).isTrue();

useConfiguration("-c", "dbg", "--fission=opt");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

useConfiguration("-c", "dbg", "--fission=opt,dbg");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isTrue();
assertThat(toolchainProvider.useFission()).isTrue();

useConfiguration("-c", "fastbuild", "--fission=opt,dbg");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

useConfiguration("-c", "fastbuild", "--fission=opt,dbg");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

// Universally enabled
useConfiguration("-c", "dbg", "--fission=yes");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isTrue();
assertThat(toolchainProvider.useFission()).isTrue();

useConfiguration("-c", "opt", "--fission=yes");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isTrue();
assertThat(toolchainProvider.useFission()).isTrue();

useConfiguration("-c", "fastbuild", "--fission=yes");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isTrue();
assertThat(toolchainProvider.useFission()).isTrue();

// Universally disabled
useConfiguration("-c", "dbg", "--fission=no");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

useConfiguration("-c", "opt", "--fission=no");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();

useConfiguration("-c", "fastbuild", "--fission=no");
target = getConfiguredTarget("//a:b");
toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER);
assertThat(
CppHelper.useFission(
getConfiguration(target).getFragment(CppConfiguration.class), toolchainProvider))
.isFalse();
assertThat(toolchainProvider.useFission()).isFalse();
}

@Test
Expand Down

0 comments on commit 0a502ff

Please sign in to comment.