Consolidating start/end lib archive expansion

Different places in the link logic were in charge of expanding start/end lib archives, causing discrepancies. This CL gets rid of the special iteration in Link.java and makes LibrariesToLinkCollecter solely responsible for mapping static libraries to their object files on the link command line.

RELNOTES:
Fixing start/end lib expansion for linking. There were many cases where archive files were still being used with toolchains that support start/end lib. This change consolidates the places that make that decision so they can be more consistent.
PiperOrigin-RevId: 194400112

3 files changed, 105 insertions, 171 deletions

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 19bd3a7ddd..4739b7be53 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
@@ -838,7 +838,7 @@ public class CppLinkActionBuilder {
     }
 
     Map<Artifact, Artifact> ltoMapping = new HashMap<>();
-    ;
+
     if (isFinalLinkOfLtoBuild()) {
       for (LtoBackendArtifacts a : allLtoArtifacts) {
         ltoMapping.put(a.getBitcodeFile(), a.getObjectFile());
@@ -846,15 +846,8 @@ public class CppLinkActionBuilder {
     }
     Iterable<Artifact> objectArtifacts =
         getArtifactsPossiblyLtoMapped(objectFileInputs, ltoMapping);
-    Iterable<Artifact> linkstampObjectArtifacts =
+    ImmutableSet<Artifact> linkstampObjectArtifacts =
         getArtifactsPossiblyLtoMapped(linkstampObjectFileInputs, ltoMapping);
-    Iterable<Artifact> expandedInputs =
-        getArtifactsPossiblyLtoMapped(
-            Link.mergeInputsDependencies(
-                uniqueLibraries,
-                needWholeArchive,
-                CppHelper.getArchiveType(cppConfiguration, toolchain)),
-            ltoMapping);
 
     ImmutableSet<Artifact> combinedObjectArtifacts =
         ImmutableSet.<Artifact>builder()
@@ -928,16 +921,12 @@ public class CppLinkActionBuilder {
               symbolCounts);
     }
 
-    final Iterable<LinkerInput> linkerInputs =
+    // Linker inputs without any start/end lib expansions.
+    final Iterable<LinkerInput> nonExpandedLinkerInputs =
         IterablesChain.<LinkerInput>builder()
             .add(objectFileInputs)
             .add(linkstampObjectFileInputs)
-            .add(
-                ImmutableIterable.from(
-                    Link.mergeInputsCmdLine(
-                        uniqueLibraries,
-                        needWholeArchive,
-                        CppHelper.getArchiveType(cppConfiguration, toolchain))))
+            .add(ImmutableIterable.from(uniqueLibraries))
             .add(
                 // Adding toolchain libraries without whole archive no-matter-what. People don't
                 // want to include whole libstdc++ in their binary ever.
@@ -982,10 +971,21 @@ public class CppLinkActionBuilder {
             featureConfiguration,
             thinltoParamFile,
             allowLtoIndexing,
-            linkerInputs,
+            nonExpandedLinkerInputs,
             needWholeArchive);
     CollectedLibrariesToLink collectedLibrariesToLink =
         librariesToLinkCollector.collectLibrariesToLink();
+
+    ImmutableSet<Artifact> expandedLinkerArtifacts =
+        getArtifactsPossiblyLtoMapped(
+            collectedLibrariesToLink.getExpandedLinkerInputs(), ltoMapping);
+
+    // Remove the linkstamp objects from inputs so that createLinkstampCompileAction doesn't cause a
+    // circular dependency.
+    Set<Artifact> expandedLinkerArtifactsNoLinkstamps =
+        new LinkedHashSet<Artifact>(expandedLinkerArtifacts);
+    expandedLinkerArtifactsNoLinkstamps.removeAll(linkstampObjectArtifacts);
+
     Variables variables =
         LinkBuildVariables.setupVariables(
             this,
@@ -1041,7 +1041,7 @@ public class CppLinkActionBuilder {
 
     LinkCommandLine.Builder linkCommandLineBuilder =
         new LinkCommandLine.Builder(ruleContext)
-            .setLinkerInputs(linkerInputs)
+            .setLinkerInputs(collectedLibrariesToLink.getExpandedLinkerInputs())
             .setLinkTargetType(linkType)
             .setLinkStaticness(linkStaticness)
             .setToolchainLibrariesSolibDir(
@@ -1107,7 +1107,7 @@ public class CppLinkActionBuilder {
             .add(ImmutableList.copyOf(objectArtifacts))
             .add(ImmutableList.copyOf(nonCodeInputs))
             .add(dependencyInputsBuilder.build())
-            .add(ImmutableIterable.from(expandedInputs));
+            .add(ImmutableSet.copyOf(expandedLinkerArtifactsNoLinkstamps));
 
     if (thinltoParamFile != null && !isLtoIndexing) {
       inputsBuilder.add(ImmutableList.of(thinltoParamFile));
@@ -1116,9 +1116,8 @@ public class CppLinkActionBuilder {
       inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile()));
       // Pass along tree artifacts, so they can be properly expanded.
       ImmutableSet<Artifact> paramFileActionInputs =
-          ImmutableSet.<LinkerInput>copyOf(linkerInputs)
+          expandedLinkerArtifacts
               .stream()
-              .map(LinkerInput::getArtifact)
               .filter(a -> a.isTreeArtifact())
               .collect(ImmutableSet.toImmutableSet());
 
@@ -1207,7 +1206,7 @@ public class CppLinkActionBuilder {
     return !isLtoIndexing && allLtoArtifacts != null;
   }
 
-  private Iterable<Artifact> getArtifactsPossiblyLtoMapped(
+  private ImmutableSet<Artifact> getArtifactsPossiblyLtoMapped(
       Iterable<LinkerInput> inputs, Map<Artifact, Artifact> ltoMapping) {
     Preconditions.checkNotNull(ltoMapping);
     ImmutableSet.Builder<Artifact> result = ImmutableSet.builder();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
index dc7fb3bc6b..ec86c09f6c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java
@@ -117,14 +117,17 @@ public class LibrariesToLinkCollector {
    */
   public static class CollectedLibrariesToLink {
     private final SequenceBuilder librariesToLink;
+    private final ImmutableSet<LinkerInput> expandedLinkerInputs;
     private final ImmutableSet<String> librarySearchDirectories;
     private final ImmutableSet<String> runtimeLibrarySearchDirectories;
 
     public CollectedLibrariesToLink(
         SequenceBuilder librariesToLink,
+        ImmutableSet<LinkerInput> expandedLinkerInputs,
         ImmutableSet<String> librarySearchDirectories,
         ImmutableSet<String> runtimeLibrarySearchDirectories) {
       this.librariesToLink = librariesToLink;
+      this.expandedLinkerInputs = expandedLinkerInputs;
       this.librarySearchDirectories = librarySearchDirectories;
       this.runtimeLibrarySearchDirectories = runtimeLibrarySearchDirectories;
     }
@@ -133,6 +136,11 @@ public class LibrariesToLinkCollector {
       return librariesToLink;
     }
 
+    // TODO(b/78347840): Figure out how to make these Artifacts.
+    public ImmutableSet<LinkerInput> getExpandedLinkerInputs() {
+      return expandedLinkerInputs;
+    }
+
     public ImmutableSet<String> getLibrarySearchDirectories() {
       return librarySearchDirectories;
     }
@@ -155,6 +163,7 @@ public class LibrariesToLinkCollector {
     ImmutableSet.Builder<String> librarySearchDirectories = ImmutableSet.builder();
     ImmutableSet.Builder<String> runtimeLibrarySearchDirectories = ImmutableSet.builder();
     ImmutableSet.Builder<String> rpathRootsForExplicitSoDeps = ImmutableSet.builder();
+    ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder = ImmutableSet.builder();
     // List of command line parameters that need to be placed *outside* of
     // --whole-archive ... --no-whole-archive.
     SequenceBuilder librariesToLink = new SequenceBuilder();
@@ -193,7 +202,11 @@ public class LibrariesToLinkCollector {
     }
 
     Pair<Boolean, Boolean> includeSolibsPair =
-        addLinkerInputs(librarySearchDirectories, rpathRootsForExplicitSoDeps, librariesToLink);
+        addLinkerInputs(
+            librarySearchDirectories,
+            rpathRootsForExplicitSoDeps,
+            librariesToLink,
+            expandedLinkerInputsBuilder);
     boolean includeSolibDir = includeSolibsPair.first;
     boolean includeToolchainLibrariesSolibDir = includeSolibsPair.second;
     Preconditions.checkState(
@@ -211,6 +224,7 @@ public class LibrariesToLinkCollector {
 
     return new CollectedLibrariesToLink(
         librariesToLink,
+        expandedLinkerInputsBuilder.build(),
         librarySearchDirectories.build(),
         allRuntimeLibrarySearchDirectories.build());
   }
@@ -218,7 +232,8 @@ public class LibrariesToLinkCollector {
   private Pair<Boolean, Boolean> addLinkerInputs(
       Builder<String> librarySearchDirectories,
       Builder<String> rpathEntries,
-      SequenceBuilder librariesToLink) {
+      SequenceBuilder librariesToLink,
+      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder) {
     boolean includeSolibDir = false;
     boolean includeToolchainLibrariesSolibDir = false;
     for (LinkerInput input : linkerInputs) {
@@ -242,9 +257,14 @@ public class LibrariesToLinkCollector {
             includeToolchainLibrariesSolibDir = true;
           }
         }
-        addDynamicInputLinkOptions(input, librariesToLink, librarySearchDirectories, rpathEntries);
+        addDynamicInputLinkOptions(
+            input,
+            librariesToLink,
+            expandedLinkerInputsBuilder,
+            librarySearchDirectories,
+            rpathEntries);
       } else {
-        addStaticInputLinkOptions(input, librariesToLink);
+        addStaticInputLinkOptions(input, librariesToLink, expandedLinkerInputsBuilder);
       }
     }
     return Pair.of(includeSolibDir, includeToolchainLibrariesSolibDir);
@@ -258,6 +278,7 @@ public class LibrariesToLinkCollector {
   private void addDynamicInputLinkOptions(
       LinkerInput input,
       SequenceBuilder librariesToLink,
+      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder,
       ImmutableSet.Builder<String> librarySearchDirectories,
       ImmutableSet.Builder<String> rpathRootsForExplicitSoDeps) {
     Preconditions.checkState(
@@ -267,6 +288,7 @@ public class LibrariesToLinkCollector {
         !Link.useStartEndLib(
             input, CppHelper.getArchiveType(cppConfiguration, ccToolchainProvider)));
 
+    expandedLinkerInputsBuilder.add(input);
     Artifact inputArtifact = input.getArtifact();
     PathFragment libDir = inputArtifact.getExecPath().getParentDirectory();
     if (!libDir.equals(solibDir)
@@ -309,9 +331,24 @@ public class LibrariesToLinkCollector {
    *
    * @param librariesToLink - a collection that will be exposed as a build variable.
    */
-  private void addStaticInputLinkOptions(LinkerInput input, SequenceBuilder librariesToLink) {
+  private void addStaticInputLinkOptions(
+      LinkerInput input,
+      SequenceBuilder librariesToLink,
+      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder) {
     ArtifactCategory artifactCategory = input.getArtifactCategory();
-    Preconditions.checkState(artifactCategory != ArtifactCategory.DYNAMIC_LIBRARY);
+    Preconditions.checkArgument(
+        artifactCategory.equals(ArtifactCategory.OBJECT_FILE)
+            || artifactCategory.equals(ArtifactCategory.STATIC_LIBRARY)
+            || artifactCategory.equals(ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY));
+    boolean isAlwaysLinkStaticLibrary =
+        artifactCategory == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY;
+
+    // input.disableWholeArchive() should only be true for libstdc++/libc++ etc.
+    boolean inputIsWholeArchive =
+        !input.disableWholeArchive() && (isAlwaysLinkStaticLibrary || needWholeArchive);
+
+    String pathPrefix = input.isFake() ? Link.FAKE_OBJECT_PREFIX : "";
+
     // If we had any LTO artifacts, ltoMap whould be non-null. In that case,
     // we should have created a thinltoParamFile which the LTO indexing
     // step will populate with the exec paths that correspond to the LTO
@@ -335,6 +372,12 @@ public class LibrariesToLinkCollector {
             if (handledByLtoIndexing(a, allowLtoIndexing)) {
               // The LTO artifacts that should be included in the final link
               // are listed in the thinltoParamFile, generated by the LTO indexing.
+
+              // Even if this object file is being skipped for exposure as a Build variable, it's
+              // still an input to this action.
+              expandedLinkerInputsBuilder.add(
+                  LinkerInputs.simpleLinkerInput(
+                      a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive  = */ false));
               continue;
             }
             // No LTO indexing step, so use the LTO backend's generated artifact directly
@@ -342,29 +385,50 @@ public class LibrariesToLinkCollector {
             member = a;
           }
           nonLtoArchiveMembersBuilder.add(member);
+          expandedLinkerInputsBuilder.add(
+              LinkerInputs.simpleLinkerInput(
+                  member, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive  = */ false));
         }
         ImmutableList<Artifact> nonLtoArchiveMembers = nonLtoArchiveMembersBuilder.build();
         if (!nonLtoArchiveMembers.isEmpty()) {
-          librariesToLink.addValue(
-              LibraryToLinkValue.forObjectFileGroup(nonLtoArchiveMembers, needWholeArchive));
+          if (inputIsWholeArchive) {
+            for (Artifact member : nonLtoArchiveMembers) {
+              if (member.isTreeArtifact()) {
+                // TODO(b/78189629): This object filegroup is expanded at action time but wrapped
+                // with --start/--end-lib. There's currently no way to force these objects to be
+                // linked in.
+                librariesToLink.addValue(
+                    LibraryToLinkValue.forObjectFileGroup(
+                        ImmutableList.<Artifact>of(member), /* isWholeArchive= */ true));
+              } else {
+                // TODO(b/78189629): These each need to be their own LibraryToLinkValue so they're
+                // not wrapped in --start/--end-lib (which lets the linker leave out objects with
+                // unreferenced code).
+                librariesToLink.addValue(
+                    LibraryToLinkValue.forObjectFile(
+                        pathPrefix + member.getExecPathString(), /* isWholeArchive= */ true));
+              }
+            }
+          } else {
+            librariesToLink.addValue(
+                LibraryToLinkValue.forObjectFileGroup(
+                    nonLtoArchiveMembers, /* isWholeArchive= */ false));
+          }
         }
       }
     } else {
-      Preconditions.checkArgument(
-          artifactCategory.equals(ArtifactCategory.OBJECT_FILE)
-              || artifactCategory.equals(ArtifactCategory.STATIC_LIBRARY)
-              || artifactCategory.equals(ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY));
-      boolean isAlwaysLinkStaticLibrary =
-          artifactCategory == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY;
-      boolean inputIsWholeArchive =
-          !input.disableWholeArchive() && (isAlwaysLinkStaticLibrary || needWholeArchive);
-
       Artifact inputArtifact = input.getArtifact();
       Artifact a;
       if (ltoMap != null && (a = ltoMap.remove(inputArtifact)) != null) {
         if (handledByLtoIndexing(a, allowLtoIndexing)) {
           // The LTO artifacts that should be included in the final link
           // are listed in the thinltoParamFile, generated by the LTO indexing.
+
+          // Even if this object file is being skipped for exposure as a Build variable, it's
+          // still an input to this action.
+          expandedLinkerInputsBuilder.add(
+              LinkerInputs.simpleLinkerInput(
+                  a, ArtifactCategory.OBJECT_FILE, /* disableWholeArchive  = */ false));
           return;
         }
         // No LTO indexing step, so use the LTO backend's generated artifact directly
@@ -372,12 +436,7 @@ public class LibrariesToLinkCollector {
         inputArtifact = a;
       }
 
-      String name;
-      if (input.isFake()) {
-        name = Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString();
-      } else {
-        name = inputArtifact.getExecPathString();
-      }
+      String name = pathPrefix + inputArtifact.getExecPathString();
 
       if (artifactCategory.equals(ArtifactCategory.OBJECT_FILE)) {
         if (inputArtifact.isTreeArtifact()) {
@@ -387,8 +446,10 @@ public class LibrariesToLinkCollector {
         } else {
           librariesToLink.addValue(LibraryToLinkValue.forObjectFile(name, inputIsWholeArchive));
         }
+        expandedLinkerInputsBuilder.add(input);
       } else {
         librariesToLink.addValue(LibraryToLinkValue.forStaticLibrary(name, inputIsWholeArchive));
+        expandedLinkerInputsBuilder.add(input);
       }
     }
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java
index 74142be962..30cdbbe7dd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/Link.java
@@ -14,13 +14,7 @@
 
 package com.google.devtools.build.lib.rules.cpp;
 
-import com.google.common.collect.AbstractIterator;
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.collect.CollectionUtils;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.util.FileTypeSet;
-import java.util.Iterator;
 
 /**
  * Utility types and methods for generating command lines for the linker, given
@@ -295,124 +289,4 @@ public abstract class Link {
             || linkerInput.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY)
         && linkerInput.containsObjectFiles();
   }
-
-  /**
-   * Replace always used archives with its members. This is used to build the linker cmd line.
-   */
-  public static Iterable<LinkerInput> mergeInputsCmdLine(NestedSet<LibraryToLink> inputs,
-      boolean globalNeedWholeArchive, ArchiveType archiveType) {
-    return new FilterMembersForLinkIterable(inputs, globalNeedWholeArchive, archiveType, false);
-  }
-
-  /**
-   * Add in any object files which are implicitly named as inputs by the linker.
-   */
-  public static Iterable<LinkerInput> mergeInputsDependencies(NestedSet<LibraryToLink> inputs,
-      boolean globalNeedWholeArchive, ArchiveType archiveType) {
-    return new FilterMembersForLinkIterable(inputs, globalNeedWholeArchive, archiveType, true);
-  }
-
-  /**
-   * On the fly implementation to filter the members.
-   */
-  private static final class FilterMembersForLinkIterable implements Iterable<LinkerInput> {
-    private final boolean globalNeedWholeArchive;
-    private final ArchiveType archiveType;
-    private final boolean deps;
-
-    private final Iterable<LibraryToLink> inputs;
-
-    private FilterMembersForLinkIterable(Iterable<LibraryToLink> inputs,
-        boolean globalNeedWholeArchive, ArchiveType archiveType, boolean deps) {
-      this.globalNeedWholeArchive = globalNeedWholeArchive;
-      this.archiveType = archiveType;
-      this.deps = deps;
-      this.inputs = CollectionUtils.makeImmutable(inputs);
-    }
-
-    @Override
-    public Iterator<LinkerInput> iterator() {
-      return new FilterMembersForLinkIterator(inputs.iterator(), globalNeedWholeArchive,
-          archiveType, deps);
-    }
-  }
-
-  /**
-   * On the fly implementation to filter the members.
-   */
-  private static final class FilterMembersForLinkIterator extends AbstractIterator<LinkerInput> {
-    private final boolean globalNeedWholeArchive;
-    private final ArchiveType archiveType;
-    private final boolean deps;
-
-    private final Iterator<LibraryToLink> inputs;
-    private Iterator<LinkerInput> delayList = ImmutableList.<LinkerInput>of().iterator();
-
-    private FilterMembersForLinkIterator(Iterator<LibraryToLink> inputs,
-        boolean globalNeedWholeArchive, ArchiveType archiveType, boolean deps) {
-      this.globalNeedWholeArchive = globalNeedWholeArchive;
-      this.archiveType = archiveType;
-      this.deps = deps;
-      this.inputs = inputs;
-    }
-
-    @Override
-    protected LinkerInput computeNext() {
-      if (delayList.hasNext()) {
-        return delayList.next();
-      }
-
-      while (inputs.hasNext()) {
-        LibraryToLink inputLibrary = inputs.next();
-
-        // True if the linker might use the members of this file, i.e., if the file is a thin or
-        // start_end_lib archive (aka static library). Also check if the library contains object
-        // files - otherwise getObjectFiles returns null, which would lead to an NPE in
-        // simpleLinkerInputs.
-        boolean needMembersForLink = archiveType != ArchiveType.REGULAR
-            && (inputLibrary.getArtifactCategory() == ArtifactCategory.STATIC_LIBRARY
-                || inputLibrary.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY)
-            && inputLibrary.containsObjectFiles();
-
-        // True if we will pass the members instead of the original archive.
-        boolean passMembersToLinkCmd = needMembersForLink && (globalNeedWholeArchive
-            || inputLibrary.getArtifactCategory() == ArtifactCategory.ALWAYSLINK_STATIC_LIBRARY);
-
-        // If deps is false (when computing the inputs to be passed on the command line), then it's
-        // an if-then-else, i.e., the passMembersToLinkCmd flag decides whether to pass the object
-        // files or the archive itself. This flag in turn is based on whether the archives are fat
-        // or not (thin archives or start_end_lib) - we never expand fat archives, but we do expand
-        // non-fat archives if we need whole-archives for the entire link, or for the specific
-        // library (i.e., if alwayslink=1).
-        //
-        // If deps is true (when computing the inputs to be passed to the action as inputs), then it
-        // becomes more complicated. We always need to pass the members for thin and start_end_lib
-        // archives (needMembersForLink). And we _also_ need to pass the archive file itself unless
-        // it's a start_end_lib archive (unless it's an alwayslink library).
-
-        // A note about ordering: the order in which the object files and the library are returned
-        // does not currently matter - this code results in the library returned first, and the
-        // object files returned after, but only if both are returned, which can only happen if
-        // deps is true, in which case this code only computes the list of inputs for the link
-        // action (so the order isn't critical).
-        if (passMembersToLinkCmd || (deps && needMembersForLink)) {
-          delayList =
-              LinkerInputs.simpleLinkerInputs(
-                      inputLibrary.getObjectFiles(),
-                      ArtifactCategory.OBJECT_FILE,
-                      /* disableWholeArchive= */ false)
-                  .iterator();
-        }
-
-        if (!(passMembersToLinkCmd || (deps && useStartEndLib(inputLibrary, archiveType)))) {
-          return inputLibrary;
-        }
-
-        if (delayList.hasNext()) {
-          return delayList.next();
-        }
-      }
-      return endOfData();
-    }
-  }
 }