From 701913139adc0eba49a7a9963fea4f555fcd844f Mon Sep 17 00:00:00 2001 From: hlopko Date: Fri, 28 Dec 2018 07:44:21 -0800 Subject: [PATCH] Allow setting needsPic crosstool capability using feature `needsPic` can now be expressed using 'pic' feature (should be enabled for it to take effect). This cl is a step towards https://github.com/bazelbuild/bazel/issues/5883. Also see the rollout doc here: https://docs.google.com/document/d/1uv4c1zag6KvdI31qdx8C6jiTognXPQrxgsUpVefm9fM/edit#. Flag removing legacy behavior is https://github.com/bazelbuild/bazel/issues/6861 RELNOTES: None. PiperOrigin-RevId: 227134726 --- site/docs/crosstool-reference.md | 23 ++-- .../lib/packages/SkylarkSemanticsOptions.java | 15 +++ .../build/lib/rules/cpp/CcBinary.java | 27 +++-- .../build/lib/rules/cpp/CcCommon.java | 10 +- .../lib/rules/cpp/CcCompilationHelper.java | 10 +- .../build/lib/rules/cpp/CcLibrary.java | 8 +- .../build/lib/rules/cpp/CcLinkingHelper.java | 5 +- .../build/lib/rules/cpp/CcModule.java | 12 +- .../lib/rules/cpp/CcToolchainProvider.java | 23 +++- .../lib/rules/cpp/CompileBuildVariables.java | 3 +- .../build/lib/rules/cpp/CppActionConfigs.java | 1 - .../build/lib/rules/cpp/CppHelper.java | 8 +- .../lib/rules/cpp/CppLinkActionBuilder.java | 3 +- .../build/lib/rules/cpp/CppRuleClasses.java | 4 + .../rules/nativedeps/NativeDepsHelper.java | 3 +- .../build/lib/skylarkbuildapi/cpp/BUILD | 1 + .../cpp/CcToolchainProviderApi.java | 35 ++++-- .../build/lib/syntax/SkylarkSemantics.java | 7 ++ src/main/protobuf/crosstool_config.proto | 1 + .../SkylarkSemanticsConsistencyTest.java | 2 + .../build/lib/rules/cpp/CcCommonTest.java | 106 ++++++++++++++++++ .../build/lib/rules/cpp/CcToolchainTest.java | 9 +- .../cpp/CppLinkstampCompileHelperTest.java | 10 +- .../lib/rules/cpp/SkylarkCcCommonTest.java | 3 - 24 files changed, 265 insertions(+), 64 deletions(-) diff --git a/site/docs/crosstool-reference.md b/site/docs/crosstool-reference.md index 22cc3ec7de34a9..59e60ed96a8d3c 100644 --- a/site/docs/crosstool-reference.md +++ b/site/docs/crosstool-reference.md @@ -901,7 +901,7 @@ The following is a reference of `CROSSTOOL` build variables. legacy_link_flags link - Linker flags coming from the legacy CROSSTOOL. + Linker flags coming from the legacy CROSSTOOL fields. @@ -930,7 +930,8 @@ The following is a reference of `CROSSTOOL` build variables. force_pic link - Presence of this variable indicates that PIC code should be generated. + Presence of this variable indicates that PIC/PIE code should + be generated (Bazel option `--force_pic` was passed). @@ -996,14 +997,6 @@ conditions. --fission flag. - - pic - - Required if the target needs PIC objects for dynamic libraries. Enabled - by default - the `pic` variable is present whenever PIC compilation is - requested. - - supports_start_end_lib @@ -1040,4 +1033,14 @@ conditions. linking mode) will be added to the linking actions. + + supports_pic + + If enabled, toolchain will know to use PIC objects for dynamic libraries. + The `pic` variable is present whenever PIC compilation is needed. If not enabled + by default, and `--force_pic` is passed, Bazel will request `supports_pic` and + validate that the feature is enabled. If the feature is missing, or couldn't + be enabled, `--force_pic` cannot be used. + + diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java index 6087eedc0438d5..919064f20bfb0e 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java @@ -191,6 +191,20 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable + "attribute definition methods, such as attr.label().") public boolean incompatibleDisableDeprecatedAttrParams; + @Option( + name = "incompatible_require_feature_configuration_for_pic", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.TOOLCHAIN, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If true, cc_toolchain_info.use_pic_for_dynamic_libraries will require " + + "feature_configuration argument (see #7007).") + public boolean requireFeatureConfigurationForPic; + @Option( name = "incompatible_disable_objc_provider_resources", defaultValue = "false", @@ -550,6 +564,7 @@ public SkylarkSemantics toSkylarkSemantics() { .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository) .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive) .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar) + .incompatibleRequireFeatureConfigurationForPic(requireFeatureConfigurationForPic) .incompatibleStricArgumentOrdering(incompatibleStricArgumentOrdering) .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable) .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) 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 51ff6b5c88a04b..0c580106653172 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 @@ -231,7 +231,8 @@ private static Runfiles collectRunfiles( // or header modules. builder.addSymlinksToArtifacts(ccCompilationContext.getAdditionalInputs()); builder.addSymlinksToArtifacts( - ccCompilationContext.getTransitiveModules(usePic(ruleContext, toolchain))); + ccCompilationContext.getTransitiveModules( + usePic(ruleContext, toolchain, featureConfiguration))); } return builder.build(); } @@ -387,7 +388,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont } } - boolean usePic = usePic(ruleContext, ccToolchain); + boolean usePic = usePic(ruleContext, ccToolchain, featureConfiguration); // On Windows, if GENERATE_PDB_FILE feature is enabled // then a pdb file will be built along with the executable. @@ -482,7 +483,8 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont ccLinkingOutputsBinary.getAllLtoArtifacts()); Artifact dwpFile = ruleContext.getImplicitOutputArtifact(CppRuleClasses.CC_BINARY_DEBUG_PACKAGE); - createDebugPackagerActions(ruleContext, ccToolchain, dwpFile, dwoArtifacts); + createDebugPackagerActions( + ruleContext, ccToolchain, featureConfiguration, dwpFile, dwoArtifacts); // The debug package should include the dwp file only if it was explicitly requested. Artifact explicitDwpFile = dwpFile; @@ -541,6 +543,7 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont ruleContext, ccToolchain, cppConfiguration, + featureConfiguration, common, ruleBuilder, filesToBuild, @@ -771,9 +774,10 @@ private static DwoArtifactsCollector collectTransitiveDwoArtifacts( public static Iterable getDwpInputs( RuleContext context, CcToolchainProvider toolchain, + FeatureConfiguration featureConfiguration, NestedSet picDwoArtifacts, NestedSet dwoArtifacts) { - return usePic(context, toolchain) ? picDwoArtifacts : dwoArtifacts; + return usePic(context, toolchain, featureConfiguration) ? picDwoArtifacts : dwoArtifacts; } /** @@ -782,12 +786,14 @@ public static Iterable getDwpInputs( private static void createDebugPackagerActions( RuleContext context, CcToolchainProvider toolchain, + FeatureConfiguration featureConfiguration, Artifact dwpOutput, DwoArtifactsCollector dwoArtifactsCollector) { Iterable allInputs = getDwpInputs( context, toolchain, + featureConfiguration, dwoArtifactsCollector.getPicDwoArtifacts(), dwoArtifactsCollector.getDwoArtifacts()); @@ -954,6 +960,7 @@ private static void addTransitiveInfoProviders( RuleContext ruleContext, CcToolchainProvider toolchain, CppConfiguration cppConfiguration, + FeatureConfiguration featureConfiguration, CcCommon common, RuleConfiguredTargetBuilder builder, NestedSet filesToBuild, @@ -975,7 +982,8 @@ private static void addTransitiveInfoProviders( CcCompilationHelper.collectHeaderTokens(ruleContext, ccCompilationOutputs); NestedSet filesToCompile = ccCompilationOutputs.getFilesToCompile( - cppConfiguration.processHeadersInDependencies(), toolchain.usePicForDynamicLibraries()); + cppConfiguration.processHeadersInDependencies(), + toolchain.usePicForDynamicLibraries(featureConfiguration)); builder .setFilesToBuild(filesToBuild) @@ -1016,11 +1024,14 @@ private static NestedSet collectTransitiveCcNativeLibraries( return builder.build(); } - private static boolean usePic(RuleContext ruleContext, CcToolchainProvider ccToolchainProvider) { + private static boolean usePic( + RuleContext ruleContext, + CcToolchainProvider ccToolchainProvider, + FeatureConfiguration featureConfiguration) { if (isLinkShared(ruleContext)) { - return ccToolchainProvider.usePicForDynamicLibraries(); + return ccToolchainProvider.usePicForDynamicLibraries(featureConfiguration); } else { - return CppHelper.usePicForBinaries(ruleContext, ccToolchainProvider); + return CppHelper.usePicForBinaries(ruleContext, ccToolchainProvider, featureConfiguration); } } } 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 f3e5f215b35231..ce9c4c360715f7 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 @@ -85,7 +85,8 @@ public final class CcCommon { public static final String MINIMUM_OS_VERSION_VARIABLE_NAME = "minimum_os_version"; public static final String PIC_CONFIGURATION_ERROR = - "PIC compilation is requested but the toolchain does not support it"; + "PIC compilation is requested but the toolchain does not support it " + + "(feature named 'supports_pic' is not enabled)"; private static final String NO_COPTS_ATTRIBUTE = "nocopts"; @@ -838,6 +839,10 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException( } } + if (cppConfiguration.forcePic()) { + allRequestedFeaturesBuilder.add(CppRuleClasses.SUPPORTS_PIC); + } + ImmutableSet allUnsupportedFeatures = unsupportedFeaturesBuilder.build(); // If STATIC_LINK_MSVCRT feature isn't specified by user, we add DYNAMIC_LINK_MSVCRT_* feature @@ -934,7 +939,8 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException( } } if ((cppConfiguration.forcePic() || toolchain.toolchainNeedsPic()) - && !featureConfiguration.isEnabled(CppRuleClasses.PIC)) { + && (!featureConfiguration.isEnabled(CppRuleClasses.PIC) + && !featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_PIC))) { throw new EvalException(/* location= */ null, PIC_CONFIGURATION_ERROR); } return featureConfiguration; 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 d0b7e09b0a8027..13bfd9db74776e 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 @@ -352,11 +352,11 @@ public CcCompilationHelper( this.cppConfiguration = Preconditions.checkNotNull(ruleContext.getFragment(CppConfiguration.class)); setGenerateNoPicAction( - !ccToolchain.usePicForDynamicLibraries() - || !CppHelper.usePicForBinaries(ruleContext, ccToolchain)); + !ccToolchain.usePicForDynamicLibraries(featureConfiguration) + || !CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration)); setGeneratePicAction( - ccToolchain.usePicForDynamicLibraries() - || CppHelper.usePicForBinaries(ruleContext, ccToolchain)); + ccToolchain.usePicForDynamicLibraries(featureConfiguration) + || CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration)); } /** @@ -830,7 +830,7 @@ public CompilationInfo compile() throws RuleErrorException { outputGroups.put(OutputGroupInfo.TEMP_FILES, ccOutputs.getTemps()); if (emitCompileProviders) { boolean processHeadersInDependencies = cppConfiguration.processHeadersInDependencies(); - boolean usePic = ccToolchain.usePicForDynamicLibraries(); + boolean usePic = ccToolchain.usePicForDynamicLibraries(featureConfiguration); outputGroups.put( OutputGroupInfo.FILES_TO_COMPILE, ccOutputs.getFilesToCompile(processHeadersInDependencies, usePic)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java index 076d0e08d0cd84..dd9ffcef19347d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java @@ -455,7 +455,8 @@ public static void init( .addProvider(RunfilesProvider.withData(defaultRunfiles.build(), dataRunfiles.build())) .addOutputGroup( OutputGroupInfo.HIDDEN_TOP_LEVEL, - collectHiddenTopLevelArtifacts(ruleContext, ccToolchain, ccCompilationOutputs)) + collectHiddenTopLevelArtifacts( + ruleContext, ccToolchain, ccCompilationOutputs, featureConfiguration)) .addOutputGroup( CcCompilationHelper.HIDDEN_HEADER_TOKENS, CcCompilationHelper.collectHeaderTokens(ruleContext, ccCompilationOutputs)); @@ -464,12 +465,13 @@ public static void init( private static NestedSet collectHiddenTopLevelArtifacts( RuleContext ruleContext, CcToolchainProvider toolchain, - CcCompilationOutputs ccCompilationOutputs) { + CcCompilationOutputs ccCompilationOutputs, + FeatureConfiguration featureConfiguration) { // Ensure that we build all the dependencies, otherwise users may get confused. NestedSetBuilder artifactsToForceBuilder = NestedSetBuilder.stableOrder(); CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class); boolean processHeadersInDependencies = cppConfiguration.processHeadersInDependencies(); - boolean usePic = toolchain.usePicForDynamicLibraries(); + boolean usePic = toolchain.usePicForDynamicLibraries(featureConfiguration); artifactsToForceBuilder.addTransitive( ccCompilationOutputs.getFilesToCompile(processHeadersInDependencies, usePic)); for (OutputGroupInfo dep : diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java index c1b3f2e4faea0f..744fd8d23c3b9b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java @@ -411,8 +411,9 @@ private CcLinkingOutputs createCcLinkActions(CcCompilationOutputs ccOutputs) CcLinkingOutputs.Builder result = new CcLinkingOutputs.Builder(); AnalysisEnvironment env = ruleContext.getAnalysisEnvironment(); - boolean usePicForBinaries = CppHelper.usePicForBinaries(ruleContext, ccToolchain); - boolean usePicForDynamicLibs = ccToolchain.usePicForDynamicLibraries(); + boolean usePicForBinaries = + CppHelper.usePicForBinaries(ruleContext, ccToolchain, featureConfiguration); + boolean usePicForDynamicLibs = ccToolchain.usePicForDynamicLibraries(featureConfiguration); PathFragment labelName = PathFragment.create(ruleContext.getLabel().getName()); String libraryIdentifier = 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 80f40ff53e7230..50047a8b773d0a 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 @@ -1034,12 +1034,6 @@ private static NestedSet convertSkylarkListOrNestedSetToNestedSet( @Param(name = "compiler", positional = false, type = String.class, named = true), @Param(name = "abi_version", positional = false, type = String.class, named = true), @Param(name = "abi_libc_version", positional = false, type = String.class, named = true), - @Param( - name = "needs_pic", - positional = false, - type = Boolean.class, - defaultValue = "False", - named = true), @Param( name = "tool_paths", positional = false, @@ -1081,7 +1075,6 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark( String compiler, String abiVersion, String abiLibcVersion, - Boolean needsPic, SkylarkList toolPaths, SkylarkList makeVariables, Object builtinSysroot, @@ -1252,8 +1245,7 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark( .setTargetLibc(targetLibc) .setCompiler(compiler) .setAbiVersion(abiVersion) - .setAbiLibcVersion(abiLibcVersion) - .setNeedsPic(needsPic); + .setAbiLibcVersion(abiLibcVersion); if (convertFromNoneable(ccTargetOs, /* defaultValue= */ null) != null) { cToolchain.setCcTargetOs((String) ccTargetOs); @@ -1282,7 +1274,7 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromSkylark( /* staticRuntimesFilegroup= */ "", /* dynamicRuntimesFilegroup= */ "", supportsFission, - needsPic, + /* needsPic= */ false, toolPathList, /* compilerFlags= */ ImmutableList.of(), /* cxxFlags= */ ImmutableList.of(), 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 319dfe48b8eaa1..109bccd911a48b 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 @@ -48,7 +48,7 @@ @Immutable @AutoCodec public final class CcToolchainProvider extends ToolchainInfo - implements CcToolchainProviderApi, HasCcToolchainLabel { + implements CcToolchainProviderApi, HasCcToolchainLabel { public static final String SKYLARK_NAME = "CcToolchainInfo"; /** An empty toolchain to be returned in the error case (instead of null). */ @@ -289,8 +289,25 @@ public static Map getCppBuildVariables( * @return true if this rule's compilations should apply -fPIC, false otherwise */ @Override - public boolean usePicForDynamicLibraries() { - return forcePic || toolchainNeedsPic(); + public boolean usePicForDynamicLibraries(FeatureConfiguration featureConfiguration) { + return forcePic + || toolchainNeedsPic() + || featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_PIC); + } + + /** + * Deprecated since it uses legacy crosstool fields. + * + *

See {link {@link #usePicForDynamicLibraries(FeatureConfiguration)} for docs} + * + * @return + */ + @Deprecated + @Override + public boolean usePicForDynamicLibrariesUsingLegacyFields() { + return forcePic + || toolchainNeedsPic() + || FeatureConfiguration.EMPTY.isEnabled(CppRuleClasses.SUPPORTS_PIC); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java index f4d750d1885889..0d103c9a71f5ec 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java @@ -281,7 +281,8 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException( buildVariables.addStringSequenceVariable(PREPROCESSOR_DEFINES.getVariableName(), allDefines); if (usePic) { - if (!featureConfiguration.isEnabled(CppRuleClasses.PIC)) { + if (!featureConfiguration.isEnabled(CppRuleClasses.PIC) + && !featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_PIC)) { throw new EvalException(Location.BUILTIN, CcCommon.PIC_CONFIGURATION_ERROR); } buildVariables.addStringVariable(PIC.getVariableName(), ""); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java index 3fc3da59caa73c..b7d1d73841632a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java @@ -137,7 +137,6 @@ public static ImmutableList getLegacyFeatures( Joiner.on("\n") .join( " name: 'pic'", - " enabled: true", " flag_set {", " action: 'assemble'", " action: 'preprocess-assemble'", 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 13d13c22e5c01d..f5afd7431a7590 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 @@ -519,13 +519,17 @@ public static void checkLinkstampsUnique( // CcCommonConfiguredTarget.noCoptsMatches(). /** Returns whether binaries must be compiled with position independent code. */ - public static boolean usePicForBinaries(RuleContext ruleContext, CcToolchainProvider toolchain) { + public static boolean usePicForBinaries( + RuleContext ruleContext, + CcToolchainProvider toolchain, + FeatureConfiguration featureConfiguration) { CppConfiguration config = ruleContext.getFragment(CppConfiguration.class); if (CcCommon.noCoptsMatches("-fPIC", ruleContext)) { return false; } return config.forcePic() - || (toolchain.toolchainNeedsPic() && config.getCompilationMode() != CompilationMode.OPT); + || (toolchain.usePicForDynamicLibraries(featureConfiguration) + && config.getCompilationMode() != CompilationMode.OPT); } /** 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 6f725f75820039..7471d2195ad6ac 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 @@ -1011,7 +1011,8 @@ public CppLinkAction build() throws InterruptedException { CppHelper.getFdoBuildStamp(ruleContext, fdoProvider, featureConfiguration), featureConfiguration, cppConfiguration.forcePic() - || (linkType.isDynamicLibrary() && toolchain.toolchainNeedsPic()), + || (linkType.isDynamicLibrary() + && toolchain.usePicForDynamicLibraries(featureConfiguration)), Matcher.quoteReplacement( isNativeDeps && cppConfiguration.shareNativeDeps() ? output.getExecPathString() diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java index 6fd1c2eefcb79c..d806d6bd41bdb2 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java @@ -236,6 +236,9 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) { */ public static final String PIC = "pic"; + /** A string constant for a feature that indicates that the toolchain can produce PIC objects. */ + public static final String SUPPORTS_PIC = "supports_pic"; + /** * A string constant for the feature the represents preprocessor defines. */ @@ -396,6 +399,7 @@ public static Label ccToolchainTypeAttribute(RuleDefinitionEnvironment env) { * runtime. */ public static final String SUPPORTS_DYNAMIC_LINKER = "supports_dynamic_linker"; + /** Ancestor for all rules that do include scanning. */ public static final class CcIncludeScanningRule implements RuleDefinition { @Override 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 c643d3bcb613a6..2d604af2894472 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 @@ -274,7 +274,8 @@ public static NativeDepsRunfiles createNativeDepsAction( if (builder.hasLtoBitcodeInputs() && featureConfiguration.isEnabled(CppRuleClasses.THIN_LTO)) { builder.setLtoIndexing(true); - builder.setUsePicForLtoBackendActions(toolchain.usePicForDynamicLibraries()); + builder.setUsePicForLtoBackendActions( + toolchain.usePicForDynamicLibraries(featureConfiguration)); CppLinkAction indexAction = builder.build(); if (indexAction != null) { ruleContext.registerAction(indexAction); diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BUILD b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BUILD index 6aff40a8fce5d3..17d6904f1cf72a 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/BUILD @@ -21,6 +21,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib:build-base", "//src/main/java/com/google/devtools/build/lib:events", + "//src/main/java/com/google/devtools/build/lib:skylark_semantics", "//src/main/java/com/google/devtools/build/lib:skylarkinterface", "//src/main/java/com/google/devtools/build/lib:syntax", "//src/main/java/com/google/devtools/build/lib/actions", diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcToolchainProviderApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcToolchainProviderApi.java index 1161eb7e8ecff3..997a9bb76aa2b4 100644 --- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcToolchainProviderApi.java +++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcToolchainProviderApi.java @@ -16,26 +16,43 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.skylarkbuildapi.platform.ToolchainInfoApi; +import com.google.devtools.build.lib.skylarkinterface.Param; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; +import com.google.devtools.build.lib.syntax.SkylarkSemantics.FlagIdentifier; import javax.annotation.Nullable; /** Information about the C++ toolchain. */ @SkylarkModule(name = "CcToolchainInfo", doc = "Information about the C++ compiler being used.") -public interface CcToolchainProviderApi extends ToolchainInfoApi { +public interface CcToolchainProviderApi + extends ToolchainInfoApi { @SkylarkCallable( name = "use_pic_for_dynamic_libraries", + disableWithFlag = FlagIdentifier.INCOMPATIBLE_REQUIRE_FEATURE_CONFIGURATION_FOR_PIC, doc = - "Returns true if this rule's compilations should apply -fPIC, false otherwise. " - + "Determines if we should apply -fPIC for this rule's C++ compilations. This " - + "determination is generally made by the global C++ configuration settings " - + "needsPic and usePicForBinaries. However, an individual " - + "rule may override these settings by applying -fPIC to its " - + "nocopts attribute. This allows incompatible rules to opt out of " - + "global PIC settings.", + "Deprecated (see https://github.com/bazelbuild/bazel/issues/7007)." + + "" + + "

Returns true if this rule's compilations should apply -fPIC, false otherwise. " + + "Determines if we should apply -fPIC for this rule's C++ compilations.", structField = true) - boolean usePicForDynamicLibraries(); + boolean usePicForDynamicLibrariesUsingLegacyFields(); + + @SkylarkCallable( + name = "needs_pic_for_dynamic_libraries", + doc = + "Returns true if this rule's compilations should apply -fPIC, false otherwise. " + + "Determines if we should apply -fPIC for this rule's C++ compilations depending " + + "on the C++ toolchain and presence of `--force_pic` Bazel option.", + parameters = { + @Param( + name = "feature_configuration", + doc = "Feature configuration to be queried.", + positional = false, + named = true, + type = FeatureConfigurationApi.class) + }) + boolean usePicForDynamicLibraries(FeatureConfigurationT featureConfigurationApi); @SkylarkCallable( name = "built_in_include_directories", diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java index d58fd2c4609046..492ceb829ad383 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java @@ -49,6 +49,8 @@ public enum FlagIdentifier { INCOMPATIBLE_NO_TARGET_OUTPUT_GROUP( SkylarkSemantics::incompatibleNoTargetOutputGroup), INCOMPATIBLE_NO_ATTR_LICENSE(SkylarkSemantics::incompatibleNoAttrLicense), + INCOMPATIBLE_REQUIRE_FEATURE_CONFIGURATION_FOR_PIC( + SkylarkSemantics::incompatibleRequireFeatureConfigurationForPic), NONE(null); // Using a Function here makes the enum definitions far cleaner, and, since this is @@ -176,6 +178,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleRemoveNativeMavenJar(); + public abstract boolean incompatibleRequireFeatureConfigurationForPic(); + public abstract boolean incompatibleStricArgumentOrdering(); public abstract boolean incompatibleStringIsNotIterable(); @@ -229,6 +233,7 @@ public static Builder builderWithDefaults() { .incompatibleRemoveNativeGitRepository(true) .incompatibleRemoveNativeHttpArchive(true) .incompatibleRemoveNativeMavenJar(false) + .incompatibleRequireFeatureConfigurationForPic(false) .incompatibleStricArgumentOrdering(false) .incompatibleStringIsNotIterable(true) .internalSkylarkFlagTestCanary(false) @@ -261,6 +266,8 @@ public abstract static class Builder { public abstract Builder incompatibleDisableDeprecatedAttrParams(boolean value); + public abstract Builder incompatibleRequireFeatureConfigurationForPic(boolean value); + public abstract Builder incompatibleDisableObjcProviderResources(boolean value); public abstract Builder incompatibleDisallowConflictingProviders(boolean value); diff --git a/src/main/protobuf/crosstool_config.proto b/src/main/protobuf/crosstool_config.proto index f9be862bb44bb2..a1e46953f7fbeb 100644 --- a/src/main/protobuf/crosstool_config.proto +++ b/src/main/protobuf/crosstool_config.proto @@ -373,6 +373,7 @@ message CToolchain { optional bool supports_fission = 43 [default = false]; // Legacy field, ignored by Bazel. optional bool supports_dsym = 51 [default = false]; + // Legacy field, use 'supports_pic' feature instead optional bool needsPic = 12 [default = false]; // Compiler flags for C/C++/Asm compilation. diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index ac6fc3958a9907..c5ace037cfa13c 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -153,6 +153,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex "--incompatible_remove_native_git_repository=" + rand.nextBoolean(), "--incompatible_remove_native_http_archive=" + rand.nextBoolean(), "--incompatible_remove_native_maven_jar=" + rand.nextBoolean(), + "--incompatible_require_feature_configuration_for_pic=" + rand.nextBoolean(), "--incompatible_strict_argument_ordering=" + rand.nextBoolean(), "--incompatible_string_is_not_iterable=" + rand.nextBoolean(), "--internal_skylark_flag_test_canary=" + rand.nextBoolean()); @@ -198,6 +199,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) { .incompatibleRemoveNativeGitRepository(rand.nextBoolean()) .incompatibleRemoveNativeHttpArchive(rand.nextBoolean()) .incompatibleRemoveNativeMavenJar(rand.nextBoolean()) + .incompatibleRequireFeatureConfigurationForPic(rand.nextBoolean()) .incompatibleStricArgumentOrdering(rand.nextBoolean()) .incompatibleStringIsNotIterable(rand.nextBoolean()) .internalSkylarkFlagTestCanary(rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java index 793e8fe6c9ed3d..ad21d0d437c272 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java @@ -23,12 +23,14 @@ import com.google.common.collect.ImmutableSet; import com.google.common.truth.IterableSubject; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.AnalysisUtils; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.OutputGroupInfo; +import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; import com.google.devtools.build.lib.analysis.mock.BazelAnalysisMock; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; @@ -981,6 +983,110 @@ public void testSymlinkActionIsNotRegisteredWhenIncludePrefixDoesntChangePath() assertContainsEvent("Symbol a is provided by all of the following features: a1 a2"); } + @Test + public void testSupportsPicFeatureResultsInPICObjectGenerated() throws Exception { + getAnalysisMock() + .ccSupport() + .setupCrosstool( + mockToolsConfig, + MockCcSupport.NO_LEGACY_FEATURES_FEATURE, + MockCcSupport.EMPTY_STATIC_LIBRARY_ACTION_CONFIG, + MockCcSupport.EMPTY_COMPILE_ACTION_CONFIG, + MockCcSupport.EMPTY_DYNAMIC_LIBRARY_ACTION_CONFIG, + "needsPic: false", + "feature { name: 'supports_pic' enabled: true }"); + useConfiguration(); + + scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])"); + scratch.file("x/a.cc"); + + RuleConfiguredTarget ccLibrary = (RuleConfiguredTarget) getConfiguredTarget("//x:foo"); + ImmutableList actions = ccLibrary.getActions(); + ImmutableList outputs = + actions.stream() + .map(ActionAnalysisMetadata::getPrimaryOutput) + .map(Artifact::getFilename) + .collect(ImmutableList.toImmutableList()); + assertThat(outputs).contains("a.pic.o"); + } + + @Test + public void testWhenSupportsPicDisabledPICObjectAreNotGenerated() throws Exception { + getAnalysisMock() + .ccSupport() + .setupCrosstool( + mockToolsConfig, + MockCcSupport.NO_LEGACY_FEATURES_FEATURE, + MockCcSupport.EMPTY_STATIC_LIBRARY_ACTION_CONFIG, + MockCcSupport.EMPTY_COMPILE_ACTION_CONFIG, + MockCcSupport.EMPTY_DYNAMIC_LIBRARY_ACTION_CONFIG, + "needsPic: false", + "feature { name: 'supports_pic' }"); + useConfiguration(); + + scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])"); + scratch.file("x/a.cc"); + + RuleConfiguredTarget ccLibrary = (RuleConfiguredTarget) getConfiguredTarget("//x:foo"); + ImmutableList actions = ccLibrary.getActions(); + ImmutableList outputs = + actions.stream() + .map(ActionAnalysisMetadata::getPrimaryOutput) + .map(Artifact::getFilename) + .collect(ImmutableList.toImmutableList()); + assertThat(outputs).doesNotContain("a.pic.o"); + } + + @Test + public void testWhenSupportsPicDisabledButForcePicSetPICAreGenerated() throws Exception { + getAnalysisMock() + .ccSupport() + .setupCrosstool( + mockToolsConfig, + MockCcSupport.NO_LEGACY_FEATURES_FEATURE, + MockCcSupport.EMPTY_STATIC_LIBRARY_ACTION_CONFIG, + MockCcSupport.EMPTY_COMPILE_ACTION_CONFIG, + MockCcSupport.EMPTY_DYNAMIC_LIBRARY_ACTION_CONFIG, + "needsPic: false", + "feature { name: 'supports_pic' }"); + useConfiguration("--force_pic"); + + scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])"); + scratch.file("x/a.cc"); + + RuleConfiguredTarget ccLibrary = (RuleConfiguredTarget) getConfiguredTarget("//x:foo"); + ImmutableList actions = ccLibrary.getActions(); + ImmutableList outputs = + actions.stream() + .map(ActionAnalysisMetadata::getPrimaryOutput) + .map(Artifact::getFilename) + .collect(ImmutableList.toImmutableList()); + assertThat(outputs).contains("a.pic.o"); + } + + @Test + public void testWhenSupportsPicNotPresentAndForcePicPassedIsError() throws Exception { + reporter.removeHandler(failFastHandler); + getAnalysisMock() + .ccSupport() + .setupCrosstool( + mockToolsConfig, + MockCcSupport.NO_LEGACY_FEATURES_FEATURE, + MockCcSupport.EMPTY_STATIC_LIBRARY_ACTION_CONFIG, + MockCcSupport.EMPTY_DYNAMIC_LIBRARY_ACTION_CONFIG, + MockCcSupport.EMPTY_COMPILE_ACTION_CONFIG, + "needsPic: false"); + useConfiguration("--force_pic"); + + scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])"); + scratch.file("x/a.cc"); + + getConfiguredTarget("//x:foo"); + assertContainsEvent( + "PIC compilation is requested but the toolchain does not support it" + + " (feature named 'supports_pic' is not enabled"); + } + /** * Tests for the case where there are only C++ rules defined. */ 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 586fefa9dbee8d..44d9a032ea08eb 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 @@ -19,6 +19,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; import com.google.devtools.build.lib.analysis.ConfiguredTarget; import com.google.devtools.build.lib.analysis.RuleContext; @@ -213,8 +214,13 @@ private boolean usePicForBinariesWithConfiguration(String... configuration) thro ConfiguredTarget target = getConfiguredTarget("//a:b"); CcToolchainProvider toolchainProvider = (CcToolchainProvider) target.get(ToolchainInfo.PROVIDER); + FeatureConfiguration featureConfiguration = + CcCommon.configureFeaturesOrThrowEvalException( + /* requestedFeatures= */ ImmutableSet.of(), + /* unsupportedFeatures= */ ImmutableSet.of(), + toolchainProvider); RuleContext ruleContext = getRuleContext(target); - return CppHelper.usePicForBinaries(ruleContext, toolchainProvider); + return CppHelper.usePicForBinaries(ruleContext, toolchainProvider, featureConfiguration); } @Test @@ -763,7 +769,6 @@ private void writeStarlarkRule() throws IOException { " tool_path(name = 'llvm_profdata', path = '/some/path'),", " ],", " cc_target_os = 'os',", - " needs_pic = True,", " builtin_sysroot = 'sysroot')", "cc_toolchain_config_rule = rule(", " implementation = _impl,", diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java index 9f8bea926867fa..c5d2d1dfd003cc 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.util.ActionsTestUtil; @@ -24,6 +25,7 @@ import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; import com.google.devtools.build.lib.analysis.util.AnalysisMock; import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; +import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -215,7 +217,13 @@ public void testLinkstampCompileDependsOnAllCcBinaryLinkingInputs() throws Excep Artifact executable = getExecutable(target); CcToolchainProvider toolchain = CppHelper.getToolchainUsingDefaultCcToolchainAttribute(getRuleContext(target)); - boolean usePic = CppHelper.usePicForBinaries(getRuleContext(target), toolchain); + FeatureConfiguration featureConfiguration = + CcCommon.configureFeaturesOrThrowEvalException( + /* requestedFeatures= */ ImmutableSet.of(), + /* unsupportedFeatures= */ ImmutableSet.of(), + toolchain); + boolean usePic = + CppHelper.usePicForBinaries(getRuleContext(target), toolchain, featureConfiguration); CppLinkAction generatingAction = (CppLinkAction) getGeneratingAction(executable); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java index 54c2e64abf9f76..7d841e1af800d2 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java @@ -4261,7 +4261,6 @@ public void testCcToolchainInfoFromSkylark() throws Exception { " abi_version = 'abi',", " tool_paths = [tool_path(name = 'name1', path = 'path1')],", " cc_target_os = 'os',", - " needs_pic = True,", " builtin_sysroot = 'sysroot',", " make_variables = [make_variable(name = 'acs', value = 'asd')])", "cc_toolchain_config_rule = rule(", @@ -4617,7 +4616,6 @@ public void testCcToolchainInfoToProto() throws Exception { " compiler = 'compiler',", " abi_libc_version = 'abi_libc',", " abi_version = 'abi',", - " needs_pic = True,", " tool_paths = [tool_path(name = 'name1', path = 'path1')],", " make_variables = [make_variable(name = 'variable', value = '--a -b -c')],", " builtin_sysroot = 'sysroot',", @@ -4655,7 +4653,6 @@ public void testCcToolchainInfoToProto() throws Exception { assertThat(toolchain.getCompiler()).isEqualTo("compiler"); assertThat(toolchain.getAbiLibcVersion()).isEqualTo("abi_libc"); assertThat(toolchain.getAbiVersion()).isEqualTo("abi"); - assertThat(toolchain.getNeedsPic()).isTrue(); ToolPath toolPath = Iterables.getOnlyElement(toolchain.getToolPathList()); assertThat(toolPath.getName()).isEqualTo("name1"); assertThat(toolPath.getPath()).isEqualTo("path1");