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 #5883. Also
see the rollout doc here:
https://docs.google.com/document/d/1uv4c1zag6KvdI31qdx8C6jiTognXPQrxgsUpVefm9fM/edit#.

Flag removing legacy behavior is #6861

RELNOTES: None.
PiperOrigin-RevId: 227134726

site/docs/crosstool-reference.md

@@ -901,7 +901,7 @@ The following is a reference of `CROSSTOOL` build variables.
    <td><strong><code>legacy_link_flags</code></strong>
    </td>
    <td>link</td>
-   <td>Linker flags coming from the legacy <code>CROSSTOOL</code>.
+   <td>Linker flags coming from the legacy <code>CROSSTOOL</code> fields.
    </td>
   </tr>
   <tr>
@@ -930,7 +930,8 @@ The following is a reference of `CROSSTOOL` build variables.
    <td><strong><code>force_pic</code></strong>
    </td>
    <td>link</td>
-   <td>Presence of this variable indicates that PIC code should be generated.
+   <td>Presence of this variable indicates that PIC/PIE code should
+     be generated (Bazel option `--force_pic` was passed).
    </td>
   </tr>
   <tr>
@@ -996,14 +997,6 @@ conditions.
         <code>--fission</code> flag.
    </td>
   </tr>
-  <tr>
-   <td><strong><code>pic</code></strong>
-   </td>
-   <td>Required if the target needs PIC objects for dynamic libraries. Enabled
-       by default - the `pic` variable is present whenever PIC compilation is
-       requested.
-   </td>
-  </tr>
   <tr>
    <td><strong><code>supports_start_end_lib</code></strong>
    </td>
@@ -1040,4 +1033,14 @@ conditions.
      linking mode) will be added to the linking actions.
    </td>
   </tr>
+  <tr>
+   <td><strong><code>supports_pic</code></strong>
+   </td>
+   <td>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.
+   </td>
+  </tr>
 </table>

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)

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<Artifact> getDwpInputs(
       RuleContext context,
       CcToolchainProvider toolchain,
+      FeatureConfiguration featureConfiguration,
       NestedSet<Artifact> picDwoArtifacts,
       NestedSet<Artifact> dwoArtifacts) {
-    return usePic(context, toolchain) ? picDwoArtifacts : dwoArtifacts;
+    return usePic(context, toolchain, featureConfiguration) ? picDwoArtifacts : dwoArtifacts;
   }
 
   /**
@@ -782,12 +786,14 @@ public static Iterable<Artifact> getDwpInputs(
   private static void createDebugPackagerActions(
       RuleContext context,
       CcToolchainProvider toolchain,
+      FeatureConfiguration featureConfiguration,
       Artifact dwpOutput,
       DwoArtifactsCollector dwoArtifactsCollector) {
     Iterable<Artifact> 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<Artifact> filesToBuild,
@@ -975,7 +982,8 @@ private static void addTransitiveInfoProviders(
         CcCompilationHelper.collectHeaderTokens(ruleContext, ccCompilationOutputs);
     NestedSet<Artifact> filesToCompile =
         ccCompilationOutputs.getFilesToCompile(
-            cppConfiguration.processHeadersInDependencies(), toolchain.usePicForDynamicLibraries());
+            cppConfiguration.processHeadersInDependencies(),
+            toolchain.usePicForDynamicLibraries(featureConfiguration));
 
     builder
         .setFilesToBuild(filesToBuild)
@@ -1016,11 +1024,14 @@ private static NestedSet<LinkerInput> 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);
     }
   }
 }

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<String> 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;

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));

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<Artifact> collectHiddenTopLevelArtifacts(
       RuleContext ruleContext,
       CcToolchainProvider toolchain,
-      CcCompilationOutputs ccCompilationOutputs) {
+      CcCompilationOutputs ccCompilationOutputs,
+      FeatureConfiguration featureConfiguration) {
     // Ensure that we build all the dependencies, otherwise users may get confused.
     NestedSetBuilder<Artifact> 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 :

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 =

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

@@ -1034,12 +1034,6 @@ private static <T> NestedSet<T> 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<Object> toolPaths,
       SkylarkList<Object> 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(),

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<FeatureConfiguration>, 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<String, String> 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.
+   *
+   * <p>See {link {@link #usePicForDynamicLibraries(FeatureConfiguration)} for docs}
+   *
+   * @return
+   */
+  @Deprecated
+  @Override
+  public boolean usePicForDynamicLibrariesUsingLegacyFields() {
+    return forcePic
+        || toolchainNeedsPic()
+        || FeatureConfiguration.EMPTY.isEnabled(CppRuleClasses.SUPPORTS_PIC);
   }
 
   /**

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(), "");

src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java

@@ -137,7 +137,6 @@ public static ImmutableList<CToolchain.Feature> getLegacyFeatures(
                 Joiner.on("\n")
                     .join(
                         "  name: 'pic'",
-                        "  enabled: true",
                         "  flag_set {",
                         "    action: 'assemble'",
                         "    action: 'preprocess-assemble'",

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);
   }
 
   /**

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()