From 0a502ff58810d1093dffeafa7baf120f02ab29c3 Mon Sep 17 00:00:00 2001 From: hlopko Date: Tue, 8 May 2018 01:19:31 -0700 Subject: [PATCH] Remove cppConfiguration from LinkBuildVariables This is to simplify the API that will eventually be exposed to Skylark. Working towards #4571. RELNOTES: None. PiperOrigin-RevId: 195785588 --- .../build/lib/rules/cpp/CcBinary.java | 6 +- .../build/lib/rules/cpp/CcCommon.java | 2 +- .../lib/rules/cpp/CcCompilationHelper.java | 8 +-- .../lib/rules/cpp/CcToolchainProvider.java | 34 +++++++++++ .../build/lib/rules/cpp/CppHelper.java | 19 ------ .../lib/rules/cpp/CppLinkActionBuilder.java | 7 +-- .../lib/rules/cpp/LinkBuildVariables.java | 8 +-- .../build/lib/rules/java/JavaBinary.java | 3 +- .../lib/rules/objc/CompilationSupport.java | 3 +- .../build/lib/rules/cpp/CcToolchainTest.java | 60 ++++--------------- 10 files changed, 59 insertions(+), 91 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java index c82e3297339d0b..62cbb5232c2453 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java @@ -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 = @@ -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(); } 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 56199a7e752d27..e85c538ca9f4f7 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 @@ -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); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index c45002281ed232..1d8c9528b3d329 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -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; } @@ -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 = 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 3e229e15fc808a..91ec045a892e6b 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 @@ -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 values, @@ -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. */ @@ -249,6 +258,23 @@ public static Map 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 globalMakeEnvBuilder) { globalMakeEnvBuilder.putAll( @@ -1021,5 +1047,13 @@ public boolean isCodeCoverageEnabled() { public boolean isHostConfiguration() { return isHostConfiguration; } + + public boolean getForcePic() { + return forcePic; + } + + public boolean getShouldStripBinaries() { + return shouldStripBinaries; + } } 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 9025818edcf499..61e7b379c51627 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 @@ -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); - } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 83df6d0f239d73..40af3b8e8721dc 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -489,8 +489,7 @@ private LtoBackendArtifacts createLtoArtifact( toolchain, fdoSupport, usePicForLtoBackendActions, - CppHelper.shouldCreatePerObjectDebugInfo( - cppConfiguration, toolchain, featureConfiguration), + toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration), argv) : new LtoBackendArtifacts( ltoOutputRootPrefix, @@ -503,8 +502,7 @@ private LtoBackendArtifacts createLtoArtifact( toolchain, fdoSupport, usePicForLtoBackendActions, - CppHelper.shouldCreatePerObjectDebugInfo( - cppConfiguration, toolchain, featureConfiguration), + toolchain.shouldCreatePerObjectDebugInfo(featureConfiguration), argv); return ltoArtifact; } @@ -1004,7 +1002,6 @@ public CppLinkAction build() throws InterruptedException { thinltoMergedObjectFile, mustKeepDebug, symbolCounts, - cppConfiguration, toolchain, featureConfiguration, useTestOnlyFlags, diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java index 7dd22fc24745d6..826bf6ad109280 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkBuildVariables.java @@ -101,7 +101,6 @@ public static Variables setupVariables( Artifact thinltoMergedObjectFile, boolean mustKeepDebug, Artifact symbolCounts, - CppConfiguration cppConfiguration, CcToolchainProvider ccToolchainProvider, FeatureConfiguration featureConfiguration, boolean useTestOnlyFlags, @@ -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(), ""); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java index aa6de48b3a60d4..69ff3d9834584f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaBinary.java @@ -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); 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 71ba13cf018243..7b3622640520cc 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 @@ -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; @@ -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); } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java index 53f572b0b3fb10..afa366beefe384 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java @@ -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