Store non-code inputs to link actions separately.

This is a re-submission of commit 99de0d07574f808fee36826289cb1f5c83e3b3e0 (rolled back in commit eff8b365c172b7e904ac6f7bba0c893fed7c91a8) and a few tweaks: - The fix for the bug that caused the rollback (see line 888 in CppModel.java -- we now use addNonCodeInputs() instead of addObjectFiles() to pass in the processed header tokens)
- A few extra assertions
- A test case
- The re-submission of the parts of commit 603b540bbcd7414cd3e2c0b92c1c8985b035e41b that were also rolled back as collateral damage.

The bug report didn't contain a precise reproduction, but the bug is trivial enough that I'm comfortable with things this way.

--
MOS_MIGRATED_REVID=129872117

1 files changed, 57 insertions, 37 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 75fd3fdeff..21425b2564 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
@@ -123,7 +123,8 @@ public class CppLinkActionBuilder {
 
   // Morally equivalent with {@link Context}, except these are mutable.
   // Keep these in sync with {@link Context}.
-  private final Set<LinkerInput> nonLibraries = new LinkedHashSet<>();
+  private final Set<LinkerInput> objectFiles = new LinkedHashSet<>();
+  private final Set<Artifact> nonCodeInputs = new LinkedHashSet<>();
   private final NestedSetBuilder<LibraryToLink> libraries = NestedSetBuilder.linkOrder();
   private NestedSet<Artifact> crosstoolInputs = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
   private Artifact runtimeMiddleman;
@@ -227,7 +228,8 @@ public class CppLinkActionBuilder {
     Preconditions.checkNotNull(linkContext);
 
     // All linkContext fields should be transferred to this Builder.
-    this.nonLibraries.addAll(linkContext.nonLibraries);
+    this.objectFiles.addAll(linkContext.objectFiles);
+    this.nonCodeInputs.addAll(linkContext.nonCodeInputs);
     this.libraries.addTransitive(linkContext.libraries);
     this.crosstoolInputs = linkContext.crosstoolInputs;
     this.runtimeMiddleman = linkContext.runtimeMiddleman;
@@ -248,10 +250,14 @@ public class CppLinkActionBuilder {
   }
   
   /** Returns linker inputs that are not libraries. */
-  public Set<LinkerInput> getNonLibraries() {
-    return nonLibraries;
+  public Set<LinkerInput> getObjectFiles() {
+    return objectFiles;
   }
-  
+
+  public Set<Artifact> getNonCodeInputs() {
+    return nonCodeInputs;
+  }
+
   /**
    * Returns linker inputs that are libraries.
    */
@@ -372,14 +378,9 @@ public class CppLinkActionBuilder {
         }
       }
     }
-    for (LinkerInput input : nonLibraries) {
-      // This relies on file naming conventions. It would be less fragile to have a dedicated
-      // field for non-library .o files.
-      if (CppFileTypes.OBJECT_FILE.matches(input.getArtifact().getExecPath())
-          || CppFileTypes.PIC_OBJECT_FILE.matches(input.getArtifact().getExecPath())) {
-        if (this.ltoBitcodeFiles.contains(input.getArtifact())) {
-          allBitcode.put(input.getArtifact().getExecPath(), input.getArtifact());
-        }
+    for (LinkerInput input : objectFiles) {
+      if (this.ltoBitcodeFiles.contains(input.getArtifact())) {
+        allBitcode.put(input.getArtifact().getExecPath(), input.getArtifact());
       }
     }
 
@@ -435,12 +436,11 @@ public class CppLinkActionBuilder {
             || needWholeArchive(linkStaticness, linkType, linkopts, isNativeDeps, cppConfiguration);
 
     NestedSet<LibraryToLink> uniqueLibraries = libraries.build();
-    final Iterable<Artifact> filteredNonLibraryArtifacts =
-        CppLinkAction.filterLinkerInputArtifacts(LinkerInputs.toLibraryArtifacts(nonLibraries));
+    final Iterable<Artifact> objectArtifacts = LinkerInputs.toLibraryArtifacts(objectFiles);
 
     final Iterable<LinkerInput> linkerInputs =
         IterablesChain.<LinkerInput>builder()
-            .add(ImmutableList.copyOf(CppLinkAction.filterLinkerInputs(nonLibraries)))
+            .add(ImmutableList.copyOf(objectFiles))
             .add(
                 ImmutableIterable.from(
                     Link.mergeInputsCmdLine(
@@ -463,12 +463,12 @@ public class CppLinkActionBuilder {
     }
 
     final LibraryToLink outputLibrary = LinkerInputs.newInputLibrary(
-        output, libraryIdentifier, filteredNonLibraryArtifacts, this.ltoBitcodeFiles);
+        output, libraryIdentifier, objectArtifacts, this.ltoBitcodeFiles);
     final LibraryToLink interfaceOutputLibrary =
         (interfaceOutput == null)
             ? null
             : LinkerInputs.newInputLibrary(interfaceOutput, libraryIdentifier,
-                filteredNonLibraryArtifacts, this.ltoBitcodeFiles);
+                objectArtifacts, this.ltoBitcodeFiles);
 
     final ImmutableMap<Artifact, Artifact> linkstampMap =
         mapLinkstampsToOutputs(linkstamps, ruleContext, configuration, output, linkArtifactFactory);
@@ -617,7 +617,8 @@ public class CppLinkActionBuilder {
         LinkerInputs.toLibraryArtifacts(
             Link.mergeInputsDependencies(
                 uniqueLibraries, needWholeArchive, cppConfiguration.archiveType()));
-    Iterable<Artifact> expandedNonLibraryInputs = LinkerInputs.toLibraryArtifacts(nonLibraries);
+    Iterable<Artifact> expandedNonLibraryInputs = LinkerInputs.toLibraryArtifacts(objectFiles);
+
     if (!isLTOIndexing && allLTOArtifacts != null) {
       // We are doing LTO, and this is the real link, so substitute
       // the LTO bitcode files with the real object files they were translated into.
@@ -656,6 +657,7 @@ public class CppLinkActionBuilder {
     IterablesChain.Builder<Artifact> inputsBuilder =
         IterablesChain.<Artifact>builder()
             .add(ImmutableList.copyOf(expandedNonLibraryInputs))
+            .add(ImmutableList.copyOf(nonCodeInputs))
             .add(dependencyInputsBuilder.build())
             .add(ImmutableIterable.from(expandedInputs));
 
@@ -839,14 +841,10 @@ public class CppLinkActionBuilder {
     return this;
   }
 
-  private void addNonLibraryInput(LinkerInput input) {
+  private void addObjectFile(LinkerInput input) {
     String name = input.getArtifact().getFilename();
-    Preconditions.checkArgument(
-        !Link.ARCHIVE_LIBRARY_FILETYPES.matches(name)
-            && !Link.SHARED_LIBRARY_FILETYPES.matches(name),
-        "'%s' is a library file",
-        input);
-    this.nonLibraries.add(input);
+    Preconditions.checkArgument(Link.OBJECT_FILETYPES.matches(name), name);
+    this.objectFiles.add(input);
   }
 
   public CppLinkActionBuilder addLTOBitcodeFiles(Iterable<Artifact> files) {
@@ -857,30 +855,52 @@ public class CppLinkActionBuilder {
   }
 
   /**
-   * Adds a single artifact to the set of inputs (C++ source files, header files, etc). Artifacts
-   * that are not of recognized types will be used for dependency checking but will not be passed to
-   * the linker. The artifact must not be an archive or a shared library.
+   * Adds a single object file to the set of inputs.
+   */
+  public CppLinkActionBuilder addObjectFile(Artifact input) {
+    addObjectFile(LinkerInputs.simpleLinkerInput(input));
+    return this;
+  }
+
+  /**
+   * Adds object files to the linker action.
    */
-  public CppLinkActionBuilder addNonLibraryInput(Artifact input) {
-    addNonLibraryInput(LinkerInputs.simpleLinkerInput(input));
+  public CppLinkActionBuilder addObjectFiles(Iterable<Artifact> inputs) {
+    for (Artifact input : inputs) {
+      addObjectFile(LinkerInputs.simpleLinkerInput(input));
+    }
     return this;
   }
 
   /**
-   * Adds multiple artifacts to the set of inputs (C++ source files, header files, etc). Artifacts
-   * that are not of recognized types will be used for dependency checking but will not be passed to
-   * the linker. The artifacts must not be archives or shared libraries.
+   * Adds non-code files to the set of inputs. They will not be passed to the linker command line
+   * unless that is explicitly modified, too.
    */
-  public CppLinkActionBuilder addNonLibraryInputs(Iterable<Artifact> inputs) {
+  public CppLinkActionBuilder addNonCodeInputs(Iterable<Artifact> inputs) {
     for (Artifact input : inputs) {
-      addNonLibraryInput(LinkerInputs.simpleLinkerInput(input));
+      addNonCodeInput(input);
     }
+
+    return this;
+  }
+
+  /**
+   * Adds a single non-code file to the set of inputs. It will not be passed to the linker command
+   * line unless that is explicitly modified, too.
+   */
+  public CppLinkActionBuilder addNonCodeInput(Artifact input) {
+    String basename = input.getFilename();
+    Preconditions.checkArgument(!Link.ARCHIVE_LIBRARY_FILETYPES.matches(basename), basename);
+    Preconditions.checkArgument(!Link.SHARED_LIBRARY_FILETYPES.matches(basename), basename);
+    Preconditions.checkArgument(!Link.OBJECT_FILETYPES.matches(basename), basename);
+
+    this.nonCodeInputs.add(input);
     return this;
   }
 
   public CppLinkActionBuilder addFakeNonLibraryInputs(Iterable<Artifact> inputs) {
     for (Artifact input : inputs) {
-      addNonLibraryInput(LinkerInputs.fakeLinkerInput(input));
+      addObjectFile(LinkerInputs.fakeLinkerInput(input));
     }
     return this;
   }