Add support for crosstool feature to prefer PIC compiles even for opt…

…imized binaries. This can have performance penalty, but in configurations where dynamic linking is used for tests can lead to a substantially better sharing of artifacts between tests and binaries. In contrast to the existing --force_pic, this can be enabled per crosstool and respects whether PIC is available for the used crosstool. PiperOrigin-RevId: 492940462 Change-Id: I178088730d50f057d45f41b46ca94bad5bd231da
bazelbuild · Dec 5, 2022 · 4084671 · 4084671
1 parent de7b26a
commit 4084671
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 1 deletion.
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
@@ -603,7 +603,8 @@ public static boolean usePicForBinaries(
       FeatureConfiguration featureConfiguration) {
     return cppConfiguration.forcePic()
         || (toolchain.usePicForDynamicLibraries(cppConfiguration, featureConfiguration)
-            && cppConfiguration.getCompilationMode() != CompilationMode.OPT);
+            && (cppConfiguration.getCompilationMode() != CompilationMode.OPT
+                || featureConfiguration.isEnabled(CppRuleClasses.PREFER_PIC_FOR_OPT_BINARIES)));
   }
 
   /**

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
@@ -254,6 +254,13 @@ public static ToolchainTypeRequirement ccToolchainTypeRequirement(RuleDefinition
   /** 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 a feature that indicates that PIC compiles are preferred for binaries
+   * even in optimized builds. For configurations that use dynamic linking for tests, this provides
+   * increases sharing of artifacts between tests and binaries at the cost of performance overhead.
+   */
+  public static final String PREFER_PIC_FOR_OPT_BINARIES = "prefer_pic_for_opt_binaries";
+
   /** A string constant for the feature the represents preprocessor defines. */
   public static final String PREPROCESSOR_DEFINES = "preprocessor_defines";
 

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl b/src/test/java/com/google/devtools/build/lib/analysis/mock/cc_toolchain_config.bzl
@@ -77,6 +77,7 @@ _FEATURE_NAMES = struct(
     implicit_fsafdo = "implicit_fsafdo",
     enable_fsafdo = "enable_fsafdo",
     supports_pic = "supports_pic",
+    prefer_pic_for_opt_binaries = "prefer_pic_for_opt_binaries",
     copy_dynamic_libraries_to_binary = "copy_dynamic_libraries_to_binary",
     per_object_debug_info = "per_object_debug_info",
     supports_start_end_lib = "supports_start_end_lib",
@@ -816,6 +817,11 @@ _supports_start_end_lib_feature = feature(
 
 _supports_pic_feature = feature(name = _FEATURE_NAMES.supports_pic, enabled = True)
 
+_prefer_pic_for_opt_binaries_feature = feature(
+    name = _FEATURE_NAMES.prefer_pic_for_opt_binaries,
+    enabled = True,
+)
+
 _targets_windows_feature = feature(
     name = _FEATURE_NAMES.targets_windows,
     enabled = True,
@@ -1342,6 +1348,7 @@ _feature_name_to_feature = {
     _FEATURE_NAMES.copy_dynamic_libraries_to_binary: _copy_dynamic_libraries_to_binary_feature,
     _FEATURE_NAMES.supports_start_end_lib: _supports_start_end_lib_feature,
     _FEATURE_NAMES.supports_pic: _supports_pic_feature,
+    _FEATURE_NAMES.prefer_pic_for_opt_binaries: _prefer_pic_for_opt_binaries_feature,
     _FEATURE_NAMES.targets_windows: _targets_windows_feature,
     _FEATURE_NAMES.archive_param_file: _archive_param_file_feature,
     _FEATURE_NAMES.compiler_param_file: _compiler_param_file_feature,

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
@@ -1013,6 +1013,66 @@ public void testWhenSupportsPicDisabledButForcePicSetPICAreGenerated() throws Ex
     assertThat(outputs).contains("a.pic.o");
   }
 
+  @Test
+  public void testPreferPicForOptBinaryFeature() throws Exception {
+    getAnalysisMock()
+        .ccSupport()
+        .setupCcToolchainConfig(
+            mockToolsConfig,
+            CcToolchainConfig.builder()
+                .withFeatures(
+                    CppRuleClasses.NO_LEGACY_FEATURES,
+                    CppRuleClasses.SUPPORTS_PIC,
+                    CppRuleClasses.PREFER_PIC_FOR_OPT_BINARIES)
+                .withActionConfigs(
+                    CppActionNames.CPP_LINK_STATIC_LIBRARY,
+                    CppActionNames.CPP_COMPILE,
+                    CppActionNames.CPP_LINK_NODEPS_DYNAMIC_LIBRARY));
+    useConfiguration("--cpu=k8", "--compilation_mode=opt");
+
+    scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])");
+    scratch.file("x/a.cc");
+
+    RuleConfiguredTarget ccLibrary = (RuleConfiguredTarget) getConfiguredTarget("//x:foo");
+    ImmutableList<ActionAnalysisMetadata> actions = ccLibrary.getActions();
+    ImmutableList<String> outputs =
+        actions.stream()
+            .map(ActionAnalysisMetadata::getPrimaryOutput)
+            .map(Artifact::getFilename)
+            .collect(ImmutableList.toImmutableList());
+    assertThat(outputs).doesNotContain("a.o");
+    assertThat(outputs).contains("a.pic.o");
+  }
+
+  @Test
+  public void testPreferPicForOptBinaryFeatureNeedsPicSupport() throws Exception {
+    getAnalysisMock()
+        .ccSupport()
+        .setupCcToolchainConfig(
+            mockToolsConfig,
+            CcToolchainConfig.builder()
+                .withFeatures(
+                    CppRuleClasses.NO_LEGACY_FEATURES, CppRuleClasses.PREFER_PIC_FOR_OPT_BINARIES)
+                .withActionConfigs(
+                    CppActionNames.CPP_LINK_STATIC_LIBRARY,
+                    CppActionNames.CPP_COMPILE,
+                    CppActionNames.CPP_LINK_NODEPS_DYNAMIC_LIBRARY));
+    useConfiguration("--cpu=k8", "--compilation_mode=opt");
+
+    scratch.file("x/BUILD", "cc_library(name = 'foo', srcs = ['a.cc'])");
+    scratch.file("x/a.cc");
+
+    RuleConfiguredTarget ccLibrary = (RuleConfiguredTarget) getConfiguredTarget("//x:foo");
+    ImmutableList<ActionAnalysisMetadata> actions = ccLibrary.getActions();
+    ImmutableList<String> outputs =
+        actions.stream()
+            .map(ActionAnalysisMetadata::getPrimaryOutput)
+            .map(Artifact::getFilename)
+            .collect(ImmutableList.toImmutableList());
+    assertThat(outputs).doesNotContain("a.pic.o");
+    assertThat(outputs).contains("a.o");
+  }
+
   @Test
   public void testWhenSupportsPicNotPresentAndForcePicPassedIsError() throws Exception {
     reporter.removeHandler(failFastHandler);