Fix a bug that leads to unnecessary compiles of .pic.o files (and possibly)

others in opt compiles.

A while back, we added a HIDDEN_TOP_LEVEL output group to CcBinary targets to
ensure that --process_headers_in_dependencies works as expected for them.
However, adding all HIDDEN_TOP_LEVEL files is actually too much and e.g. also
contains .pic.o files which are expensive to build but not actually needed for
the compile. The fundamental difference between CcLibrary and CcBinary targets
here is that a CcBinary already declares most of its inputs as it needs all of
them to link the binary. In contrast, CcLibraries wouldn't need any of they
dependent .o files and thus wouldn't even try to build them.

This changes splits out the header token files which are required to make
--process_headers_in_dependencies work correctly and only adds those to
HIDDEN_TOP_LEVEL outputs of binaries files.

Also removing some unused code that was producing warnings.

--
MOS_MIGRATED_REVID=132883722

4 files changed, 41 insertions, 47 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 a8eaed6b9b..db3839eadb 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
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.rules.test.ExecutionInfoProvider;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.syntax.Type;
-import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -67,11 +66,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     this.semantics = semantics;
   }
 
-  // TODO(bazel-team): should this use Link.SHARED_LIBRARY_FILETYPES?
-  private static final FileTypeSet SHARED_LIBRARY_FILETYPES = FileTypeSet.of(
-      CppFileTypes.SHARED_LIBRARY,
-      CppFileTypes.VERSIONED_SHARED_LIBRARY);
-
   /**
    * The maximum number of inputs for any single .dwp generating action. For cases where
    * this value is exceeded, the action is split up into "batches" that fall under the limit.
@@ -215,7 +209,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             linkStaticness,
             linkopts);
     linkActionBuilder.setUseTestOnlyFlags(isTest);
-    linkActionBuilder.addNonCodeInputs(ccCompilationOutputs.getHeaderTokenFiles());
 
     CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext);
     if (linkStaticness == LinkStaticness.DYNAMIC) {
@@ -661,12 +654,13 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     InstrumentedFilesProvider instrumentedFilesProvider = common.getInstrumentedFilesProvider(
         instrumentedObjectFiles, !TargetUtils.isTestRule(ruleContext.getRule()) && !fake);
 
+    NestedSet<Artifact> headerTokens =
+        CcLibraryHelper.collectHeaderTokens(ruleContext, ccCompilationOutputs);
     NestedSet<Artifact> filesToCompile =
         ccCompilationOutputs.getFilesToCompile(
             cppConfiguration.isLipoContextCollector(),
             cppConfiguration.processHeadersInDependencies(),
             CppHelper.usePic(ruleContext, false));
-    NestedSet<Artifact> artifactsToForce = collectHiddenTopLevelArtifacts(ruleContext);
     builder
         .setFilesToBuild(filesToBuild)
         .add(CppCompilationContext.class, cppCompilationContext)
@@ -689,7 +683,10 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
         .addOutputGroup(
             OutputGroupProvider.TEMP_FILES, getTemps(cppConfiguration, ccCompilationOutputs))
         .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, filesToCompile)
-        .addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, artifactsToForce)
+        // For CcBinary targets, we only want to ensure that we process headers in dependencies and
+        // thus only add header tokens to HIDDEN_TOP_LEVEL. If we add all HIDDEN_TOP_LEVEL artifacts
+        // from dependent CcLibrary targets, we'd be building .pic.o files in nopic builds.
+        .addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, headerTokens)
         .addOutputGroup(
             OutputGroupProvider.COMPILATION_PREREQUISITES,
             CcCommon.collectCompilationPrerequisites(ruleContext, cppCompilationContext));
@@ -697,17 +694,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     CppHelper.maybeAddStaticLinkMarkerProvider(builder, ruleContext);
   }
 
-  private static NestedSet<Artifact> collectHiddenTopLevelArtifacts(RuleContext ruleContext) {
-    // Ensure that we build all the dependencies, otherwise users may get confused.
-    NestedSetBuilder<Artifact> artifactsToForceBuilder = NestedSetBuilder.stableOrder();
-    for (OutputGroupProvider dep :
-        ruleContext.getPrerequisites("deps", Mode.TARGET, OutputGroupProvider.class)) {
-      artifactsToForceBuilder.addTransitive(
-          dep.getOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL));
-    }
-    return artifactsToForceBuilder.build();
-  }
-
   private static NestedSet<Artifact> collectExecutionDynamicLibraryArtifacts(
       RuleContext ruleContext,
       List<LibraryToLink> executionDynamicLibraries) {
@@ -738,7 +724,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     return builder.build();
   }
 
-
   private static NestedSet<Artifact> getTemps(CppConfiguration cppConfiguration,
       CcCompilationOutputs compilationOutputs) {
     return cppConfiguration.isLipoContextCollector()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index cd0f9f8a5a..885ca0a0bc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.common.base.Function;
-import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -62,14 +61,6 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
       CppFileTypes.ALWAYS_LINK_LIBRARY, CppFileTypes.ALWAYS_LINK_PIC_LIBRARY,
       CppFileTypes.SHARED_LIBRARY);
 
-  private static final Predicate<LibraryToLink> PIC_STATIC_FILTER = new Predicate<LibraryToLink>() {
-    @Override
-    public boolean apply(LibraryToLink input) {
-      String name = input.getArtifact().getExecPath().getBaseName();
-      return !name.endsWith(".nopic.a") && !name.endsWith(".nopic.lo");
-    }
-  };
-
   private static Runfiles collectRunfiles(RuleContext context,
       CcLinkingOutputs ccLinkingOutputs,
       boolean neverLink, boolean addDynamicRuntimeInputArtifactsToRunfiles,
@@ -259,9 +250,6 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
     // For now, we don't add the precompiled libraries to the files to build.
     CcLinkingOutputs linkedLibraries = info.getCcLinkingOutputsExcludingPrecompiledLibraries();
 
-    NestedSet<Artifact> artifactsToForce =
-        collectHiddenTopLevelArtifacts(ruleContext, info.getCcCompilationOutputs());
-
     NestedSetBuilder<Artifact> filesBuilder = NestedSetBuilder.stableOrder();
     filesBuilder.addAll(LinkerInputs.toLibraryArtifacts(linkedLibraries.getStaticLibraries()));
     filesBuilder.addAll(LinkerInputs.toLibraryArtifacts(linkedLibraries.getPicStaticLibraries()));
@@ -294,18 +282,22 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
         .add(RunfilesProvider.class, RunfilesProvider.withData(staticRunfiles, sharedRunfiles))
         // Remove this?
         .add(CppRunfilesProvider.class, new CppRunfilesProvider(staticRunfiles, sharedRunfiles))
-        .addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, artifactsToForce);
-
+        .addOutputGroup(
+            OutputGroupProvider.HIDDEN_TOP_LEVEL,
+            collectHiddenTopLevelArtifacts(ruleContext, info.getCcCompilationOutputs()))
+        .addOutputGroup(
+            CcLibraryHelper.HIDDEN_HEADER_TOKENS,
+            CcLibraryHelper.collectHeaderTokens(ruleContext, info.getCcCompilationOutputs()));
   }
 
   private static NestedSet<Artifact> collectHiddenTopLevelArtifacts(
-      RuleContext ruleContext, CcCompilationOutputs ccCompilationOutputs) {
+      RuleContext ruleContext,
+      CcCompilationOutputs ccCompilationOutputs) {
     // Ensure that we build all the dependencies, otherwise users may get confused.
     NestedSetBuilder<Artifact> artifactsToForceBuilder = NestedSetBuilder.stableOrder();
-    boolean isLipoCollector =
-        ruleContext.getFragment(CppConfiguration.class).isLipoContextCollector();
-    boolean processHeadersInDependencies =
-        ruleContext.getFragment(CppConfiguration.class).processHeadersInDependencies();
+    CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
+    boolean isLipoCollector = cppConfiguration.isLipoContextCollector();
+    boolean processHeadersInDependencies = cppConfiguration.processHeadersInDependencies();
     boolean usePic = CppHelper.usePic(ruleContext, false);
     artifactsToForceBuilder.addTransitive(
         ccCompilationOutputs.getFilesToCompile(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index 70d74afcb3..eaee2ead6e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -75,6 +75,13 @@ import javax.annotation.Nullable;
  * methods.
  */
 public final class CcLibraryHelper {
+  /**
+   * Similar to {@code OutputGroupProvider.HIDDEN_TOP_LEVEL}, but specific to header token files.
+   */
+  public static final String HIDDEN_HEADER_TOKENS =
+      OutputGroupProvider.HIDDEN_OUTPUT_GROUP_PREFIX
+          + "hidden_header_tokens"
+          + OutputGroupProvider.INTERNAL_SUFFIX;
 
   /**
    * A group of source file types and action names for builds controlled by CcLibraryHelper.
@@ -986,11 +993,10 @@ public final class CcLibraryHelper {
     }
 
     outputGroups.put(OutputGroupProvider.TEMP_FILES, getTemps(ccOutputs));
+    CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
     if (emitCompileProviders) {
-      boolean isLipoCollector =
-          ruleContext.getFragment(CppConfiguration.class).isLipoContextCollector();
-      boolean processHeadersInDependencies =
-          ruleContext.getFragment(CppConfiguration.class).processHeadersInDependencies();
+      boolean isLipoCollector = cppConfiguration.isLipoContextCollector();
+      boolean processHeadersInDependencies = cppConfiguration.processHeadersInDependencies();
       boolean usePic = CppHelper.usePic(ruleContext, false);
       outputGroups.put(
           OutputGroupProvider.FILES_TO_COMPILE,
@@ -1008,7 +1014,7 @@ public final class CcLibraryHelper {
     providers.put(CcExecutionDynamicLibrariesProvider.class,
         collectExecutionDynamicLibraryArtifacts(ccLinkingOutputs.getExecutionDynamicLibraries()));
 
-    boolean forcePic = ruleContext.getFragment(CppConfiguration.class).forcePic();
+    boolean forcePic = cppConfiguration.forcePic();
     if (emitCcSpecificLinkParamsProvider) {
       providers.put(CcSpecificLinkParamsProvider.class, new CcSpecificLinkParamsProvider(
           createCcLinkParamsStore(ccLinkingOutputs, cppCompilationContext, forcePic)));
@@ -1207,6 +1213,19 @@ public final class CcLibraryHelper {
     return Iterables.filter(result, Predicates.<CppModuleMap>notNull());
   }
 
+  static NestedSet<Artifact> collectHeaderTokens(
+      RuleContext ruleContext, CcCompilationOutputs ccCompilationOutputs) {
+    NestedSetBuilder<Artifact> headerTokens = NestedSetBuilder.stableOrder();
+    for (OutputGroupProvider dep :
+        ruleContext.getPrerequisites("deps", Mode.TARGET, OutputGroupProvider.class)) {
+      headerTokens.addTransitive(dep.getOutputGroup(CcLibraryHelper.HIDDEN_HEADER_TOKENS));
+    }
+    if (ruleContext.getFragment(CppConfiguration.class).processHeadersInDependencies()) {
+      headerTokens.addAll(ccCompilationOutputs.getHeaderTokenFiles());
+    }
+    return headerTokens.build();
+  }
+
   private TransitiveLipoInfoProvider collectTransitiveLipoInfo(CcCompilationOutputs outputs) {
     if (CppHelper.getFdoSupport(ruleContext).getFdoRoot() == null) {
       return TransitiveLipoInfoProvider.EMPTY;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 9eae435422..fef753103f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -858,7 +858,6 @@ public final class CppModel {
     CppLinkAction maybePicAction =
         newLinkActionBuilder(linkedArtifact)
             .addObjectFiles(ccOutputs.getObjectFiles(usePicForBinaries))
-            .addNonCodeInputs(ccOutputs.getHeaderTokenFiles())
             .addNonCodeInputs(nonCodeLinkerInputs)
             .addLTOBitcodeFiles(ccOutputs.getLtoBitcodeFiles())
             .setLinkType(linkType)
@@ -888,7 +887,6 @@ public final class CppModel {
       CppLinkAction picAction =
           newLinkActionBuilder(picArtifact)
               .addObjectFiles(ccOutputs.getObjectFiles(true))
-              .addNonCodeInputs(ccOutputs.getHeaderTokenFiles())
               .addLTOBitcodeFiles(ccOutputs.getLtoBitcodeFiles())
               .setLinkType(picLinkType)
               .setLinkStaticness(LinkStaticness.FULLY_STATIC)