Automated rollback of commit 8ca66458a42266f11aba77732b05ab06b96e95fb.

PiperOrigin-RevId: 194506134

3 files changed, 171 insertions, 105 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 4739b7be53..19bd3a7ddd 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,8 +846,15 @@ public class CppLinkActionBuilder {
     }
     Iterable<Artifact> objectArtifacts =
         getArtifactsPossiblyLtoMapped(objectFileInputs, ltoMapping);
-    ImmutableSet<Artifact> linkstampObjectArtifacts =
+    Iterable<Artifact> linkstampObjectArtifacts =
         getArtifactsPossiblyLtoMapped(linkstampObjectFileInputs, ltoMapping);
+    Iterable<Artifact> expandedInputs =
+        getArtifactsPossiblyLtoMapped(
+            Link.mergeInputsDependencies(
+                uniqueLibraries,
+                needWholeArchive,
+                CppHelper.getArchiveType(cppConfiguration, toolchain)),
+            ltoMapping);
 
     ImmutableSet<Artifact> combinedObjectArtifacts =
         ImmutableSet.<Artifact>builder()
@@ -921,12 +928,16 @@ public class CppLinkActionBuilder {
               symbolCounts);
     }
 
-    // Linker inputs without any start/end lib expansions.
-    final Iterable<LinkerInput> nonExpandedLinkerInputs =
+    final Iterable<LinkerInput> linkerInputs =
         IterablesChain.<LinkerInput>builder()
             .add(objectFileInputs)
             .add(linkstampObjectFileInputs)
-            .add(ImmutableIterable.from(uniqueLibraries))
+            .add(
+                ImmutableIterable.from(
+                    Link.mergeInputsCmdLine(
+                        uniqueLibraries,
+                        needWholeArchive,
+                        CppHelper.getArchiveType(cppConfiguration, toolchain))))
             .add(
                 // Adding toolchain libraries without whole archive no-matter-what. People don't
                 // want to include whole libstdc++ in their binary ever.
@@ -971,21 +982,10 @@ public class CppLinkActionBuilder {
             featureConfiguration,
             thinltoParamFile,
             allowLtoIndexing,
-            nonExpandedLinkerInputs,
+            linkerInputs,
             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(collectedLibrariesToLink.getExpandedLinkerInputs())
+            .setLinkerInputs(linkerInputs)
             .setLinkTargetType(linkType)
             .setLinkStaticness(linkStaticness)
             .setToolchainLibrariesSolibDir(
@@ -1107,7 +1107,7 @@ public class CppLinkActionBuilder {
             .add(ImmutableList.copyOf(objectArtifacts))
             .add(ImmutableList.copyOf(nonCodeInputs))
             .add(dependencyInputsBuilder.build())
-            .add(ImmutableSet.copyOf(expandedLinkerArtifactsNoLinkstamps));
+            .add(ImmutableIterable.from(expandedInputs));
 
     if (thinltoParamFile != null && !isLtoIndexing) {
       inputsBuilder.add(ImmutableList.of(thinltoParamFile));
@@ -1116,8 +1116,9 @@ public class CppLinkActionBuilder {
       inputsBuilder.add(ImmutableList.of(linkCommandLine.getParamFile()));
       // Pass along tree artifacts, so they can be properly expanded.
       ImmutableSet<Artifact> paramFileActionInputs =
-          expandedLinkerArtifacts
+          ImmutableSet.<LinkerInput>copyOf(linkerInputs)
               .stream()
+              .map(LinkerInput::getArtifact)
               .filter(a -> a.isTreeArtifact())
               .collect(ImmutableSet.toImmutableSet());
 
@@ -1206,7 +1207,7 @@ public class CppLinkActionBuilder {
     return !isLtoIndexing && allLtoArtifacts != null;
   }
 
-  private ImmutableSet<Artifact> getArtifactsPossiblyLtoMapped(
+  private Iterable<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 ec86c09f6c..dc7fb3bc6b 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,17 +117,14 @@ 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;
     }
@@ -136,11 +133,6 @@ 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;
     }
@@ -163,7 +155,6 @@ 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();
@@ -202,11 +193,7 @@ public class LibrariesToLinkCollector {
     }
 
     Pair<Boolean, Boolean> includeSolibsPair =
-        addLinkerInputs(
-            librarySearchDirectories,
-            rpathRootsForExplicitSoDeps,
-            librariesToLink,
-            expandedLinkerInputsBuilder);
+        addLinkerInputs(librarySearchDirectories, rpathRootsForExplicitSoDeps, librariesToLink);
     boolean includeSolibDir = includeSolibsPair.first;
     boolean includeToolchainLibrariesSolibDir = includeSolibsPair.second;
     Preconditions.checkState(
@@ -224,7 +211,6 @@ public class LibrariesToLinkCollector {
 
     return new CollectedLibrariesToLink(
         librariesToLink,
-        expandedLinkerInputsBuilder.build(),
         librarySearchDirectories.build(),
         allRuntimeLibrarySearchDirectories.build());
   }
@@ -232,8 +218,7 @@ public class LibrariesToLinkCollector {
   private Pair<Boolean, Boolean> addLinkerInputs(
       Builder<String> librarySearchDirectories,
       Builder<String> rpathEntries,
-      SequenceBuilder librariesToLink,
-      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder) {
+      SequenceBuilder librariesToLink) {
     boolean includeSolibDir = false;
     boolean includeToolchainLibrariesSolibDir = false;
     for (LinkerInput input : linkerInputs) {
@@ -257,14 +242,9 @@ public class LibrariesToLinkCollector {
             includeToolchainLibrariesSolibDir = true;
           }
         }
-        addDynamicInputLinkOptions(
-            input,
-            librariesToLink,
-            expandedLinkerInputsBuilder,
-            librarySearchDirectories,
-            rpathEntries);
+        addDynamicInputLinkOptions(input, librariesToLink, librarySearchDirectories, rpathEntries);
       } else {
-        addStaticInputLinkOptions(input, librariesToLink, expandedLinkerInputsBuilder);
+        addStaticInputLinkOptions(input, librariesToLink);
       }
     }
     return Pair.of(includeSolibDir, includeToolchainLibrariesSolibDir);
@@ -278,7 +258,6 @@ public class LibrariesToLinkCollector {
   private void addDynamicInputLinkOptions(
       LinkerInput input,
       SequenceBuilder librariesToLink,
-      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder,
       ImmutableSet.Builder<String> librarySearchDirectories,
       ImmutableSet.Builder<String> rpathRootsForExplicitSoDeps) {
     Preconditions.checkState(
@@ -288,7 +267,6 @@ 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)
@@ -331,24 +309,9 @@ public class LibrariesToLinkCollector {
    *
    * @param librariesToLink - a collection that will be exposed as a build variable.
    */
-  private void addStaticInputLinkOptions(
-      LinkerInput input,
-      SequenceBuilder librariesToLink,
-      ImmutableSet.Builder<LinkerInput> expandedLinkerInputsBuilder) {
+  private void addStaticInputLinkOptions(LinkerInput input, SequenceBuilder librariesToLink) {
     ArtifactCategory artifactCategory = input.getArtifactCategory();
-    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 : "";
-
+    Preconditions.checkState(artifactCategory != ArtifactCategory.DYNAMIC_LIBRARY);
     // 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
@@ -372,12 +335,6 @@ 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
@@ -385,50 +342,29 @@ 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()) {
-          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));
-          }
+          librariesToLink.addValue(
+              LibraryToLinkValue.forObjectFileGroup(nonLtoArchiveMembers, needWholeArchive));
         }
       }
     } 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
@@ -436,7 +372,12 @@ public class LibrariesToLinkCollector {
         inputArtifact = a;
       }
 
-      String name = pathPrefix + inputArtifact.getExecPathString();
+      String name;
+      if (input.isFake()) {
+        name = Link.FAKE_OBJECT_PREFIX + inputArtifact.getExecPathString();
+      } else {
+        name = inputArtifact.getExecPathString();
+      }
 
       if (artifactCategory.equals(ArtifactCategory.OBJECT_FILE)) {
         if (inputArtifact.isTreeArtifact()) {
@@ -446,10 +387,8 @@ 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 30cdbbe7dd..74142be962 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,7 +14,13 @@
 
 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
@@ -289,4 +295,124 @@ 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();
+    }
+  }
 }