Fix a bug that leads to unnecessary compiles of .pic.o files (and pos…

…sibly)

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

src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java

@@ -46,7 +46,6 @@
 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 @@ protected CcBinary(CppSemantics semantics) {
     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 static ConfiguredTarget init(
             linkStaticness,
             linkopts);
     linkActionBuilder.setUseTestOnlyFlags(isTest);
-    linkActionBuilder.addNonCodeInputs(ccCompilationOutputs.getHeaderTokenFiles());
 
     CcToolchainProvider ccToolchain = CppHelper.getToolchain(ruleContext);
     if (linkStaticness == LinkStaticness.DYNAMIC) {
@@ -661,12 +654,13 @@ private static void addTransitiveInfoProviders(
     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,25 +683,17 @@ private static void addTransitiveInfoProviders(
         .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));
 
     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 @@ private static NestedSet<LinkerInput> collectTransitiveCcNativeLibraries(
     return builder.build();
   }
 
-
   private static NestedSet<Artifact> getTemps(CppConfiguration cppConfiguration,
       CcCompilationOutputs compilationOutputs) {
     return cppConfiguration.isLipoContextCollector()

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 @@ protected CcLibrary(CppSemantics semantics) {
       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 LibraryToLink apply(Artifact library) {
     // 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 LibraryToLink apply(Artifact library) {
         .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(

src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java

@@ -75,6 +75,13 @@
  * 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 Info build() throws RuleErrorException, InterruptedException {
     }
 
     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 Info build() throws RuleErrorException, InterruptedException {
     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 @@ private Iterable<CppModuleMap> collectModuleMaps() {
     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;

src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java

@@ -858,7 +858,6 @@ public CcLinkingOutputs createCcLinkActions(
     CppLinkAction maybePicAction =
         newLinkActionBuilder(linkedArtifact)
             .addObjectFiles(ccOutputs.getObjectFiles(usePicForBinaries))
-            .addNonCodeInputs(ccOutputs.getHeaderTokenFiles())
             .addNonCodeInputs(nonCodeLinkerInputs)
             .addLTOBitcodeFiles(ccOutputs.getLtoBitcodeFiles())
             .setLinkType(linkType)
@@ -888,7 +887,6 @@ public CcLinkingOutputs createCcLinkActions(
       CppLinkAction picAction =
           newLinkActionBuilder(picArtifact)
               .addObjectFiles(ccOutputs.getObjectFiles(true))
-              .addNonCodeInputs(ccOutputs.getHeaderTokenFiles())
               .addLTOBitcodeFiles(ccOutputs.getLtoBitcodeFiles())
               .setLinkType(picLinkType)
               .setLinkStaticness(LinkStaticness.FULLY_STATIC)

src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java

@@ -920,12 +920,13 @@ public void testProcessHeadersInDependenciesOfBinaries() throws Exception {
         scratchConfiguredTarget(
             "foo",
             "x",
-            "cc_binary(name = 'x', deps = [':y'])",
-            "cc_library(name = 'y', hdrs = ['y.h'])");
-    assertThat(
-            ActionsTestUtil.baseArtifactNames(
-                getOutputGroup(x, OutputGroupProvider.HIDDEN_TOP_LEVEL)))
-        .contains("y.h.processed");
+            "cc_binary(name = 'x', deps = [':y', ':z'])",
+            "cc_library(name = 'y', hdrs = ['y.h'])",
+            "cc_library(name = 'z', srcs = ['z.cc'])");
+    String hiddenTopLevel =
+        ActionsTestUtil.baseNamesOf(getOutputGroup(x, OutputGroupProvider.HIDDEN_TOP_LEVEL));
+    assertThat(hiddenTopLevel).contains("y.h.processed");
+    assertThat(hiddenTopLevel).doesNotContain("z.pic.o");
   }
 
   @Test