Fix bug in --experimental_link_dynamic_binaries_separately.

If linkstatic was explicitly set to 0 for a non-test target, we didn't set
CcLibraryHelper to create CcLinkOutputs, but would still try to link those in
instead of the compile output. Instead, pull out a variable that puts this
logic into a single spot.

Also rename the flag to --experimental_link_compile_output_separately, which
IMO makes slightly more sense. Not too important as I don't think we should
keep this flag long-term anyway.

--
PiperOrigin-RevId: 144194903
MOS_MIGRATED_REVID=144194903

3 files changed, 50 insertions, 27 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 b68e2e3c5f..b21536e797 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
@@ -89,7 +89,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       NestedSet<Artifact> filesToBuild,
       Iterable<Artifact> fakeLinkerInputs,
       boolean fake,
-      ImmutableSet<CppSource> cAndCppSources) {
+      ImmutableSet<CppSource> cAndCppSources,
+      boolean linkCompileOutputSeparately) {
     Runfiles.Builder builder = new Runfiles.Builder(
         context.getWorkspaceName(), context.getConfiguration().legacyExternalRunfiles());
     Function<TransitiveInfoCollection, Runfiles> runfilesMapping =
@@ -104,12 +105,11 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     // Add the C++ runtime libraries if linking them dynamically.
     if (linkStaticness == LinkStaticness.DYNAMIC) {
       builder.addTransitiveArtifacts(toolchain.getDynamicRuntimeLinkInputs());
-      CppConfiguration cppConfiguration = context.getFragment(CppConfiguration.class);
-      if (cppConfiguration.getLinkDynamicBinariesSeparately()) {
-        builder.addArtifacts(
-            LinkerInputs.toLibraryArtifacts(
-                info.getCcLinkingOutputs().getExecutionDynamicLibraries()));
-      }
+    }
+    if (linkCompileOutputSeparately) {
+      builder.addArtifacts(
+          LinkerInputs.toLibraryArtifacts(
+              info.getCcLinkingOutputs().getExecutionDynamicLibraries()));
     }
     // For cc_binary and cc_test rules, there is an implicit dependency on
     // the malloc library package, which is specified by the "malloc" attribute.
@@ -172,6 +172,15 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     if (ruleContext.hasErrors()) {
       return null;
     }
+    
+    List<String> linkopts = common.getLinkopts();
+    LinkStaticness linkStaticness = getLinkStaticness(ruleContext, linkopts, cppConfiguration);
+
+    // We currently only want link the dynamic library generated for test code separately.
+    boolean linkCompileOutputSeparately =
+        ruleContext.isTestTarget()
+            && cppConfiguration.getLinkCompileOutputSeparately()
+            && linkStaticness == LinkStaticness.DYNAMIC;
 
     CcLibraryHelper helper =
         new CcLibraryHelper(ruleContext, semantics, featureConfiguration)
@@ -181,9 +190,19 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             .setFake(fake)
             .addPrecompiledFiles(precompiledFiles)
             .enableInterfaceSharedObjects();
-    // Always treat test targets as static libraries so that their code can be compiled into a
-    // dynamic library and profit from optimizations like interface so.
-    helper.setLinkType(ruleContext.isTestTarget() ? LinkTargetType.STATIC_LIBRARY : linkType);
+    // When linking the object files directly into the resulting binary, we do not need
+    // library-level link outputs; thus, we do not let CcLibraryHelper produce link outputs
+    // (either shared object files or archives) for a non-library link type [*], and add
+    // the object files explicitly in determineLinkerArguments.
+    //
+    // When linking the object files into their own library, we want CcLibraryHelper to
+    // take care of creating the library link outputs for us, so we need to set the link
+    // type to STATIC_LIBRARY.
+    //
+    // [*] The only library link type is STATIC_LIBRARY. EXECUTABLE specifies a normal
+    // cc_binary output, while DYNAMIC_LIBRARY is a cc_binary rules that produces an
+    // output matching a shared object, for example cc_binary(name="foo.so", ...) on linux.
+    helper.setLinkType(linkCompileOutputSeparately ? LinkTargetType.STATIC_LIBRARY : linkType);
 
     CcLibraryHelper.Info info = helper.build();
     CppCompilationContext cppCompilationContext = info.getCppCompilationContext();
@@ -206,9 +225,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       return null;
     }
 
-    List<String> linkopts = common.getLinkopts();
-    LinkStaticness linkStaticness = getLinkStaticness(ruleContext, linkopts, cppConfiguration);
-
     CppLinkActionBuilder linkActionBuilder =
         determineLinkerArguments(
             ruleContext,
@@ -219,7 +235,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             fake,
             binary,
             linkStaticness,
-            linkopts);
+            linkopts,
+            linkCompileOutputSeparately);
     linkActionBuilder.setUseTestOnlyFlags(ruleContext.isTestTarget());
 
     CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext);
@@ -333,7 +350,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             filesToBuild,
             fakeLinkerInputs,
             fake,
-            helper.getCompilationUnitSources());
+            helper.getCompilationUnitSources(),
+            linkCompileOutputSeparately);
     RunfilesSupport runfilesSupport = RunfilesSupport.withExecutable(
         ruleContext, runfiles, executable, ruleContext.getConfiguration().buildRunfiles());
 
@@ -409,7 +427,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       boolean fake,
       Artifact binary,
       LinkStaticness linkStaticness,
-      List<String> linkopts)
+      List<String> linkopts,
+      boolean linkCompileOutputSeparately)
       throws InterruptedException {
     CppLinkActionBuilder builder =
         new CppLinkActionBuilder(context, binary)
@@ -418,9 +437,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
 
     // Either link in the .o files generated for the sources of this target or link in the
     // generated dynamic library they are compiled into.
-    CppConfiguration cppConfiguration = context.getFragment(CppConfiguration.class);
-    if (cppConfiguration.getLinkDynamicBinariesSeparately()
-        && linkStaticness == LinkStaticness.DYNAMIC) {
+    if (linkCompileOutputSeparately) {
       for (LibraryToLink library : info.getCcLinkingOutputs().getDynamicLibraries()) {
         builder.addLibrary(library);
       }
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 fd205d8951..b1228148c0 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
@@ -1598,8 +1598,8 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return dynamicMode;
   }
 
-  public boolean getLinkDynamicBinariesSeparately() {
-    return cppOptions.linkDynamicBinariesSeparately;
+  public boolean getLinkCompileOutputSeparately() {
+    return cppOptions.linkCompileOutputSeparately;
   }
 
   /*
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index ecbe6bb70b..7382b00ce2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -229,7 +229,7 @@ public class CppOptions extends FragmentOptions {
   public DynamicModeFlag dynamicMode;
 
   @Option(
-    name = "experimental_link_dynamic_binaries_separately",
+    name = "experimental_link_compile_output_separately",
     defaultValue = "false",
     category = "semantics",
     help =
@@ -237,7 +237,7 @@ public class CppOptions extends FragmentOptions {
             + "If true, dynamically linked binary targets will build and link their own srcs as "
             + "a dynamic library instead of directly linking it in."
   )
-  public boolean linkDynamicBinariesSeparately;
+  public boolean linkCompileOutputSeparately;
 
   @Option(
     name = "force_pic",
@@ -550,9 +550,7 @@ public class CppOptions extends FragmentOptions {
     name = "experimental_skip_unused_modules",
     defaultValue = "false",
     category = "experimental",
-    help =
-        "If enabled, not all transitive modules automatically become an action's inputs. Instead,"
-            + " input discovery adds just the required ones."
+    help = "Deprecated. No effect."
   )
   public boolean skipUnusedModules;
 
@@ -560,11 +558,19 @@ public class CppOptions extends FragmentOptions {
     name = "experimental_prune_more_modules",
     defaultValue = "false",
     category = "experimental",
-    help = "If enabled, modules pruning is used when building modules themselves."
+    help = "Deprecated. No effect."
   )
   public boolean pruneMoreModules;
 
   @Option(
+    name = "prune_cpp_modules",
+    defaultValue = "true",
+    category = "strategy",
+    help = "If enabled, use the results of input discovery to reduce the number of used modules."
+  )
+  public boolean pruneCppModules;
+
+  @Option(
     name = "experimental_omitfp",
     defaultValue = "false",
     category = "semantics",