Turn the addition of -fPIC to the command line of PIC actions into a feature.

This is a resubmission of commit 45d48bf1fe7503acbbb0c095822b7f8f558881e8. It turns out that we also need -fPIC for *assembler* command line options, because some assembler sources are preprocessed and they can say "#ifdef __PIC__".

--
MOS_MIGRATED_REVID=122626234

8 files changed, 79 insertions, 8 deletions

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 06f2ef176f..e77bef91b4 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
@@ -86,7 +86,8 @@ public final class CcCommon {
       CppRuleClasses.MODULE_MAPS,
       CppRuleClasses.MODULE_MAP_HOME_CWD,
       CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
-      CppRuleClasses.INCLUDE_PATHS);
+      CppRuleClasses.INCLUDE_PATHS,
+      CppRuleClasses.PIC);
 
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 721ad0f889..f3f024d5e3 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -1415,10 +1415,6 @@ public class CppCompileAction extends AbstractAction
         options.add("-E");
       }
 
-      if (usePic) {
-        options.add("-fPIC");
-      }
-
       return options;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index f0d4906637..7e15b41323 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -692,6 +692,23 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
       return toolchain;
     }
     try {
+      if (!features.contains("pic")) {
+        TextFormat.merge(""
+            + "feature {"
+            + "  name: 'pic'"
+            + "  flag_set {"
+            + "    action: 'c-compile'"
+            + "    action: 'c++-compile'"
+            + "    action: 'c++-module-compile'"
+            + "    action: 'preprocess-assemble'"
+            + "    expand_if_all_available: 'pic'"
+            + "    flag_group {"
+            + "      flag: '-fPIC'"
+            + "    }"
+            + "  }"
+            + "}",
+            toolchainBuilder);
+      }
       if (!features.contains("include_paths")) {
         TextFormat.merge(""
             + "feature {"
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 65c158dce7..07497df4ee 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -388,6 +388,13 @@ public final class CppModel {
           getSafePathStrings(context.getSystemIncludeDirs()));
     }
 
+    if (usePic) {
+      if (!featureConfiguration.isEnabled(CppRuleClasses.PIC)) {
+        ruleContext.ruleError("PIC compilation is requested but the toolchain does not support it");
+      }
+      buildVariables.addVariable("pic", "");
+    }
+
     if (ccRelativeName != null) {
       CppHelper.getFdoSupport(ruleContext).configureCompilation(builder, buildVariables,
           ruleContext, ccRelativeName, autoFdoImportPath, usePic, featureConfiguration);
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 77a9719923..074900356c 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
@@ -165,6 +165,14 @@ public class CppRuleClasses {
   public static final String NO_LEGACY_FEATURES = "no_legacy_features";
 
   /**
+   * A string constant for the PIC feature.
+   *
+   * If this feature is active (currently it cannot be switched off) and PIC compilation is
+   * requested, the "pic" build variable will be defined with an empty string as its value.
+   */
+  public static final String PIC = "pic";
+
+  /**
    * A string constant for the include_paths feature.
    */
   public static final String INCLUDE_PATHS = "include_paths";
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
index ae897889da..66306be004 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/MockCcSupport.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages.util;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
@@ -223,9 +224,10 @@ public abstract class MockCcSupport {
    * @param partialToolchain A string representation of a CToolchain protocol buffer; note that
    *        this is allowed to be a partial buffer (required fields may be omitted).
    */
-  public void setupCrosstool(MockToolsConfig config, String partialToolchain) throws IOException {
+  public void setupCrosstool(MockToolsConfig config, String... partialToolchain)
+      throws IOException {
     CToolchain.Builder toolchainBuilder = CToolchain.newBuilder();
-    TextFormat.merge(partialToolchain, toolchainBuilder);
+    TextFormat.merge(Joiner.on("\n").join(partialToolchain), toolchainBuilder);
     setupCrosstool(config, toolchainBuilder.buildPartial());
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonConfiguredTargetTest.java
index f7e181a79d..f1e11a8d22 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonConfiguredTargetTest.java
@@ -397,6 +397,18 @@ public class CcCommonConfiguredTargetTest extends BuildViewTestCase {
     assertThat(getCppCompileAction("//a:nopiclib").getArgv()).doesNotContain("-fPIC");
   }
 
+  @Test
+  public void testPicModeAssembly() throws Exception {
+    CrosstoolConfigurationHelper.overwriteCrosstoolWithToolchain(
+        directories.getWorkspace(),
+        CrosstoolConfig.CToolchain.newBuilder().setNeedsPic(true).buildPartial());
+
+    scratch.file("a/BUILD",
+        "cc_library(name='preprocess', srcs=['preprocess.S'])");
+
+    assertThat(getCppCompileAction("//a:preprocess").getArgv()).contains("-fPIC");
+  }
+
   private CppCompileAction getCppCompileAction(String label) throws Exception {
     ConfiguredTarget target = getConfiguredTarget(label);
     List<CppCompileAction> compilationSteps =
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 57a9d62de8..974d19d8c4 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -673,10 +673,38 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase {
   }
 
   @Test
+  public void testPicNotAvailableError() throws Exception {
+    AnalysisMock.get()
+        .ccSupport()
+        .setupCrosstool(mockToolsConfig,
+            "feature { name: 'no_legacy_features' }");
+    useConfiguration();
+    writeSimpleCcLibrary();
+    reporter.removeHandler(failFastHandler);
+    getConfiguredTarget("//module:map");
+    assertContainsEvent("PIC compilation is requested but the toolchain does not support it");
+  }
+
+  @Test
+  public void testToolchainWithoutPicForNoPicCompilation() throws Exception {
+    AnalysisMock.get()
+        .ccSupport()
+        .setupCrosstool(mockToolsConfig,
+            "needsPic: false",
+            "feature { name: 'no_legacy_features' }");
+    useConfiguration();
+    scratchConfiguredTarget("a", "a",
+        "cc_binary(name='a', srcs=['a.cc'], deps=[':b'])",
+        "cc_library(name='b', srcs=['b.cc'])");
+  }
+
+  @Test
   public void testNoCppModuleMap() throws Exception {
     AnalysisMock.get()
         .ccSupport()
-        .setupCrosstool(mockToolsConfig, "feature { name: 'no_legacy_features' }");
+        .setupCrosstool(mockToolsConfig,
+            "feature { name: 'no_legacy_features' }",
+            "feature { name: 'pic' }");
     useConfiguration();
     writeSimpleCcLibrary();
     assertNoCppModuleMapAction("//module:map");