Update to header thinning feature to create header_scanner actions based on compiler flags.

--
PiperOrigin-RevId: 148273368
MOS_MIGRATED_REVID=148273368

2 files changed, 174 insertions, 168 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
index ed19ace0c9..df7dc609f4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -38,9 +38,11 @@ import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
@@ -53,6 +55,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
 import com.google.devtools.build.lib.analysis.actions.ParameterFileWriteAction;
+import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -1054,6 +1057,23 @@ public abstract class CompilationSupport {
     return parents.build();
   }
 
+  /** Holds information about Objective-C compile actions that require header thinning. */
+  protected static final class ObjcHeaderThinningInfo {
+    /** Source file for compile action. */
+    public final Artifact sourceFile;
+    /** headers_list file for compile action. */
+    public final Artifact headersListFile;
+    /** Command line arguments for compile action execution. */
+    public final ImmutableList<String> arguments;
+
+    public ObjcHeaderThinningInfo(
+        Artifact sourceFile, Artifact headersListFile, ImmutableList<String> arguments) {
+      this.sourceFile = Preconditions.checkNotNull(sourceFile);
+      this.headersListFile = Preconditions.checkNotNull(headersListFile);
+      this.arguments = Preconditions.checkNotNull(arguments);
+    }
+  }
+
   /**
    * Returns true when ObjC header thinning is enabled via configuration and an a valid
    * header_scanner executable target is provided.
@@ -1075,6 +1095,78 @@ public abstract class CompilationSupport {
         .getProvider(FilesToRunProvider.class);
   }
 
+  protected void registerHeaderScanningActions(
+      ImmutableList<ObjcHeaderThinningInfo> headerThinningInfo,
+      ObjcProvider objcProvider,
+      CompilationArtifacts compilationArtifacts) {
+    if (headerThinningInfo.isEmpty()) {
+      return;
+    }
+
+    FilesToRunProvider headerScannerTool = getHeaderThinningToolExecutable();
+    ListMultimap<ImmutableList<String>, ObjcHeaderThinningInfo>
+        objcHeaderThinningInfoByCommandLine = groupActionsByCommandLine(headerThinningInfo);
+    // Register a header scanning spawn action for each unique set of command line arguments
+    for (ImmutableList<String> args : objcHeaderThinningInfoByCommandLine.keySet()) {
+      SpawnAction.Builder builder =
+          new SpawnAction.Builder()
+              .setMnemonic("ObjcHeaderScanning")
+              .setExecutable(headerScannerTool);
+      CustomCommandLine.Builder cmdLine = CustomCommandLine.builder();
+      for (ObjcHeaderThinningInfo info : objcHeaderThinningInfoByCommandLine.get(args)) {
+        cmdLine.addJoinPaths(
+            ":",
+            Lists.newArrayList(info.sourceFile.getExecPath(), info.headersListFile.getExecPath()));
+        builder.addInput(info.sourceFile).addOutput(info.headersListFile);
+      }
+      ruleContext.registerAction(
+          builder
+              .setCommandLine(cmdLine.add("--").add(args).build())
+              .addInputs(compilationArtifacts.getPrivateHdrs())
+              .addTransitiveInputs(attributes.hdrs())
+              .addTransitiveInputs(objcProvider.get(ObjcProvider.HEADER))
+              .addInputs(compilationArtifacts.getPchFile().asSet())
+              .addTransitiveInputs(objcProvider.get(ObjcProvider.STATIC_FRAMEWORK_FILE))
+              .addTransitiveInputs(objcProvider.get(ObjcProvider.DYNAMIC_FRAMEWORK_FILE))
+              .build(ruleContext));
+    }
+  }
+
+  /**
+   * Groups {@link ObjcHeaderThinningInfo} objects based on the command line arguments of the
+   * ObjcCompile action.
+   *
+   * <p>Grouping by command line arguments allows {@link
+   * #registerHeaderScanningActions(ImmutableList, ObjcProvider, CompilationArtifacts)} to create a
+   * {@link SpawnAction} based on the compiler command line flags that may cause a difference in
+   * behaviour by the preprocessor. Some of the command line arguments must be filtered out as they
+   * change with every source {@link Artifact}; for example the object file (-o) and dotd filenames
+   * (-MF). These arguments are known not to change the preprocessor behaviour.
+   *
+   * @param headerThinningInfos information for compile actions that require header thinning
+   * @return values in {@code headerThinningInfos} grouped by compile action command line arguments
+   */
+  private static ListMultimap<ImmutableList<String>, ObjcHeaderThinningInfo>
+      groupActionsByCommandLine(ImmutableList<ObjcHeaderThinningInfo> headerThinningInfos) {
+    // Maintain insertion order so that iteration in #registerHeaderScanningActions is deterministic
+    ListMultimap<ImmutableList<String>, ObjcHeaderThinningInfo>
+        objcHeaderThinningInfoByCommandLine = ArrayListMultimap.create();
+    for (ObjcHeaderThinningInfo info : headerThinningInfos) {
+      ImmutableList.Builder<String> filteredArgumentsBuilder = ImmutableList.builder();
+      List<String> arguments = info.arguments;
+      for (int i = 0; i < arguments.size(); ++i) {
+        String arg = arguments.get(i);
+        if (arg.equals("-MF") || arg.equals("-o") || arg.equals("-c")) {
+          ++i;
+        } else if (!arg.equals("-MD")) {
+          filteredArgumentsBuilder.add(arg);
+        }
+      }
+      objcHeaderThinningInfoByCommandLine.put(filteredArgumentsBuilder.build(), info);
+    }
+    return objcHeaderThinningInfoByCommandLine;
+  }
+
   @Nullable
   private CcToolchainProvider maybeGetCcToolchain() {
     // TODO(rduan): Remove this check once all rules are using the crosstool support.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
index d3c4805f8e..8a1c5778dd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/LegacyCompilationSupport.java
@@ -40,13 +40,10 @@ import com.google.common.base.Joiner;
 import com.google.common.base.Optional;
 import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
-import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -76,7 +73,6 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.List;
-import java.util.Map.Entry;
 import javax.annotation.Nullable;
 
 /**
@@ -188,23 +184,8 @@ public class LegacyCompilationSupport extends CompilationSupport {
       ExtraCompileArgs extraCompileArgs,
       Iterable<PathFragment> priorityHeaders,
       Optional<CppModuleMap> moduleMap) {
-    ImmutableList.Builder<Artifact> objFiles = new ImmutableList.Builder<>();
-    ImmutableMap<Artifact, Artifact> sourceFilesToHeadersListFiles = ImmutableMap.of();
-    if (isHeaderThinningEnabled()) {
-      sourceFilesToHeadersListFiles =
-          generateHeadersListArtifactsForSources(
-              Iterables.concat(
-                  compilationArtifacts.getSrcs(), compilationArtifacts.getNonArcSrcs()));
-      if (!sourceFilesToHeadersListFiles.isEmpty()) {
-        registerHeaderScanningAction(
-            sourceFilesToHeadersListFiles,
-            objcProvider,
-            compilationArtifacts,
-            priorityHeaders,
-            moduleMap,
-            extraCompileArgs);
-      }
-    }
+    ImmutableList.Builder<Artifact> objFiles = ImmutableList.builder();
+    ImmutableList.Builder<ObjcHeaderThinningInfo> objcHeaderThinningInfos = ImmutableList.builder();
 
     for (Artifact sourceFile : compilationArtifacts.getSrcs()) {
       Artifact objFile = intermediateArtifacts.objFile(sourceFile);
@@ -220,15 +201,18 @@ public class LegacyCompilationSupport extends CompilationSupport {
             compilationArtifacts,
             Iterables.concat(extraCompileArgs, ImmutableList.of("-fobjc-arc")));
       } else {
-        registerCompileAction(
-            sourceFile,
-            objFile,
-            objcProvider,
-            priorityHeaders,
-            moduleMap,
-            compilationArtifacts,
-            Iterables.concat(extraCompileArgs, ImmutableList.of("-fobjc-arc")),
-            sourceFilesToHeadersListFiles);
+        ObjcHeaderThinningInfo objcHeaderThinningInfo =
+            registerCompileAction(
+                sourceFile,
+                objFile,
+                objcProvider,
+                priorityHeaders,
+                moduleMap,
+                compilationArtifacts,
+                Iterables.concat(extraCompileArgs, ImmutableList.of("-fobjc-arc")));
+        if (objcHeaderThinningInfo != null) {
+          objcHeaderThinningInfos.add(objcHeaderThinningInfo);
+        }
       }
     }
     for (Artifact nonArcSourceFile : compilationArtifacts.getNonArcSrcs()) {
@@ -244,15 +228,18 @@ public class LegacyCompilationSupport extends CompilationSupport {
             compilationArtifacts,
             Iterables.concat(extraCompileArgs, ImmutableList.of("-fno-objc-arc")));
       } else {
-        registerCompileAction(
-            nonArcSourceFile,
-            objFile,
-            objcProvider,
-            priorityHeaders,
-            moduleMap,
-            compilationArtifacts,
-            Iterables.concat(extraCompileArgs, ImmutableList.of("-fno-objc-arc")),
-            sourceFilesToHeadersListFiles);
+        ObjcHeaderThinningInfo objcHeaderThinningInfo =
+            registerCompileAction(
+                nonArcSourceFile,
+                objFile,
+                objcProvider,
+                priorityHeaders,
+                moduleMap,
+                compilationArtifacts,
+                Iterables.concat(extraCompileArgs, ImmutableList.of("-fno-objc-arc")));
+        if (objcHeaderThinningInfo != null) {
+          objcHeaderThinningInfos.add(objcHeaderThinningInfo);
+        }
       }
     }
 
@@ -261,32 +248,43 @@ public class LegacyCompilationSupport extends CompilationSupport {
     for (Artifact archive : compilationArtifacts.getArchive().asSet()) {
       registerArchiveActions(objFiles.build(), archive);
     }
+
+    registerHeaderScanningActions(
+        objcHeaderThinningInfos.build(), objcProvider, compilationArtifacts);
   }
 
-  /**
-   * Configures a {@link CustomCommandLine.Builder} with common compilation arguments for building
-   * Objective-C sources.
-   *
-   * <p>The command line arguments generated by this method are common to both ObjcCompile and
-   * ObjcHeaderScanning actions. Arguments that are only necessary or useful when doing more than
-   * preprocessing (i.e. building an object file via ObjcCompile) are not specified here.
-   *
-   * @param commandLine existing {@link CustomCommandLine.Builder} to add common arguments to
-   * @return passed in {@code commandLine} with common arguments added
-   */
-  private CustomCommandLine.Builder commonCompileActionCommandLine(
-      CustomCommandLine.Builder commandLine,
+  private CustomCommandLine compileActionCommandLine(
+      Artifact sourceFile,
+      Artifact objFile,
       ObjcProvider objcProvider,
       Iterable<PathFragment> priorityHeaders,
       Optional<CppModuleMap> moduleMap,
       Optional<Artifact> pchFile,
+      Optional<Artifact> dotdFile,
       Iterable<String> otherFlags,
+      boolean collectCodeCoverage,
       boolean isCPlusPlusSource) {
+    CustomCommandLine.Builder commandLine = new CustomCommandLine.Builder().add(CLANG);
+
     if (isCPlusPlusSource) {
       commandLine.add("-stdlib=libc++");
       commandLine.add("-std=gnu++11");
     }
 
+    // The linker needs full debug symbol information to perform binary dead-code stripping.
+    if (objcConfiguration.shouldStripBinary()) {
+      commandLine.add("-g");
+    }
+
+    List<String> coverageFlags = ImmutableList.of();
+    if (collectCodeCoverage) {
+      if (buildConfiguration.isLLVMCoverageMapFormatEnabled()) {
+        coverageFlags = CLANG_LLVM_COVERAGE_FLAGS;
+      } else {
+        coverageFlags = CLANG_GCOV_COVERAGE_FLAGS;
+      }
+    }
+
     commandLine
         .add(compileFlagsForClang(appleConfiguration))
         .add(commonLinkAndCompileFlagsForClang(objcProvider, objcConfiguration, appleConfiguration))
@@ -299,61 +297,9 @@ public class LegacyCompilationSupport extends CompilationSupport {
         .addBeforeEachPath("-isystem", objcProvider.get(INCLUDE_SYSTEM))
         .add(otherFlags)
         .addFormatEach("-D%s", objcProvider.get(DEFINE))
+        .add(coverageFlags)
         .add(getCompileRuleCopts());
 
-    // Add module map arguments.
-    if (moduleMap.isPresent()) {
-      // If modules are enabled for the rule, -fmodules is added to the copts already. (This implies
-      // module map usage). Otherwise, we need to pass -fmodule-maps.
-      if (!attributes.enableModules()) {
-        commandLine.add("-fmodule-maps");
-      }
-      // -fmodule-map-file only loads the module in Xcode 7, so we add the module maps's directory
-      // to the include path instead.
-      // TODO(bazel-team): Use -fmodule-map-file when Xcode 6 support is dropped.
-      commandLine
-          .add("-iquote")
-          .add(moduleMap.get().getArtifact().getExecPath().getParentDirectory().toString())
-          .add("-fmodule-name=" + moduleMap.get().getName());
-    }
-
-    return commandLine;
-  }
-
-  private CustomCommandLine compileActionCommandLine(
-      Artifact sourceFile,
-      Artifact objFile,
-      ObjcProvider objcProvider,
-      Iterable<PathFragment> priorityHeaders,
-      Optional<CppModuleMap> moduleMap,
-      Optional<Artifact> pchFile,
-      Optional<Artifact> dotdFile,
-      Iterable<String> otherFlags,
-      boolean collectCodeCoverage,
-      boolean isCPlusPlusSource) {
-    CustomCommandLine.Builder commandLine =
-        commonCompileActionCommandLine(
-            new CustomCommandLine.Builder().add(CLANG),
-            objcProvider,
-            priorityHeaders,
-            moduleMap,
-            pchFile,
-            otherFlags,
-            isCPlusPlusSource);
-
-    // The linker needs full debug symbol information to perform binary dead-code stripping.
-    if (objcConfiguration.shouldStripBinary()) {
-      commandLine.add("-g");
-    }
-
-    if (collectCodeCoverage) {
-      if (buildConfiguration.isLLVMCoverageMapFormatEnabled()) {
-        commandLine.add(CLANG_LLVM_COVERAGE_FLAGS);
-      } else {
-        commandLine.add(CLANG_GCOV_COVERAGE_FLAGS);
-      }
-    }
-
     // Add input source file arguments
     commandLine.add("-c");
     if (!sourceFile.getExecPath().isAbsolute()
@@ -393,18 +339,34 @@ public class LegacyCompilationSupport extends CompilationSupport {
       commandLine.add("-MD").addExecPath("-MF", dotdFile.get());
     }
 
+    // Add module map arguments.
+    if (moduleMap.isPresent()) {
+      // If modules are enabled for the rule, -fmodules is added to the copts already. (This implies
+      // module map usage). Otherwise, we need to pass -fmodule-maps.
+      if (!attributes.enableModules()) {
+        commandLine.add("-fmodule-maps");
+      }
+      // -fmodule-map-file only loads the module in Xcode 7, so we add the module maps's directory
+      // to the include path instead.
+      // TODO(bazel-team): Use -fmodule-map-file when Xcode 6 support is dropped.
+      commandLine
+          .add("-iquote")
+          .add(moduleMap.get().getArtifact().getExecPath().getParentDirectory().toString())
+          .add("-fmodule-name=" + moduleMap.get().getName());
+    }
+
     return commandLine.build();
   }
 
-  private void registerCompileAction(
+  @Nullable
+  private ObjcHeaderThinningInfo registerCompileAction(
       Artifact sourceFile,
       Artifact objFile,
       ObjcProvider objcProvider,
       Iterable<PathFragment> priorityHeaders,
       Optional<CppModuleMap> moduleMap,
       CompilationArtifacts compilationArtifacts,
-      Iterable<String> otherFlags,
-      ImmutableMap<Artifact, Artifact> sourceToOutput) {
+      Iterable<String> otherFlags) {
     boolean isCPlusPlusSource = ObjcRuleClasses.CPP_SOURCES.matches(sourceFile.getExecPath());
     boolean runCodeCoverage =
         buildConfiguration.isCodeCoverageEnabled() && ObjcRuleClasses.isInstrumentable(sourceFile);
@@ -446,9 +408,14 @@ public class LegacyCompilationSupport extends CompilationSupport {
             .addTransitiveMandatoryInputs(objcProvider.get(DYNAMIC_FRAMEWORK_FILE))
             .setDotdFile(dotdFile)
             .addMandatoryInputs(compilationArtifacts.getPchFile().asSet());
-    if (sourceToOutput.containsKey(sourceFile)) {
-      compileBuilder.setHeadersListFile(sourceToOutput.get(sourceFile));
+
+    Artifact headersListFile = null;
+    if (isHeaderThinningEnabled()
+        && SOURCES_FOR_HEADER_THINNING.matches(sourceFile.getFilename())) {
+      headersListFile = intermediateArtifacts.headersListFile(sourceFile);
+      compileBuilder.setHeadersListFile(headersListFile);
     }
+
     ruleContext.registerAction(
         compileBuilder
             .setMnemonic("ObjcCompile")
@@ -458,6 +425,11 @@ public class LegacyCompilationSupport extends CompilationSupport {
             .addOutputs(gcnoFile.asSet())
             .addOutput(dotdFile.artifact())
             .build(ruleContext));
+
+    return headersListFile == null
+        ? null
+        : new ObjcHeaderThinningInfo(
+            sourceFile, headersListFile, ImmutableList.copyOf(commandLine.arguments()));
   }
 
   /**
@@ -523,64 +495,6 @@ public class LegacyCompilationSupport extends CompilationSupport {
             .build(ruleContext.getActionOwner()));
   }
 
-  private ImmutableMap<Artifact, Artifact> generateHeadersListArtifactsForSources(
-      Iterable<Artifact> sources) {
-    ImmutableMap.Builder<Artifact, Artifact> sourceFileToHeadersListBuilder =
-        ImmutableMap.builder();
-    for (Artifact source : sources) {
-      // Tree artifacts are not currently supported
-      if (!source.isTreeArtifact() && SOURCES_FOR_HEADER_THINNING.matches(source.getFilename())) {
-        sourceFileToHeadersListBuilder.put(source, intermediateArtifacts.headersListFile(source));
-      }
-    }
-    return sourceFileToHeadersListBuilder.build();
-  }
-
-  private void registerHeaderScanningAction(
-      ImmutableMap<Artifact, Artifact> sourceFilesToHeadersListFiles,
-      ObjcProvider objcProvider,
-      CompilationArtifacts compilationArtifacts,
-      Iterable<PathFragment> priorityHeaders,
-      Optional<CppModuleMap> moduleMap,
-      Iterable<String> otherFlags) {
-    FilesToRunProvider headerScannerExecutable = getHeaderThinningToolExecutable();
-
-    CustomCommandLine.Builder cmdLine = CustomCommandLine.builder();
-    for (Entry<Artifact, Artifact> entry : sourceFilesToHeadersListFiles.entrySet()) {
-      cmdLine.addJoinPaths(
-          ":", Lists.newArrayList(entry.getKey().getExecPath(), entry.getValue().getExecPath()));
-    }
-    cmdLine.add("--");
-    commonCompileActionCommandLine(
-        cmdLine,
-        objcProvider,
-        priorityHeaders,
-        moduleMap,
-        compilationArtifacts.getPchFile(),
-        otherFlags,
-        true);
-
-    NestedSet<Artifact> moduleMapInputs = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-    if (objcConfiguration.moduleMapsEnabled()) {
-      moduleMapInputs = objcProvider.get(MODULE_MAP);
-    }
-
-    ruleContext.registerAction(
-        new SpawnAction.Builder()
-            .setMnemonic("ObjcHeaderScanning")
-            .setExecutable(headerScannerExecutable)
-            .setCommandLine(cmdLine.build())
-            .addInputs(sourceFilesToHeadersListFiles.keySet())
-            .addInputs(compilationArtifacts.getPrivateHdrs())
-            .addInputs(compilationArtifacts.getPchFile().asSet())
-            .addTransitiveInputs(objcProvider.get(ObjcProvider.HEADER))
-            .addTransitiveInputs(moduleMapInputs)
-            .addTransitiveInputs(objcProvider.get(ObjcProvider.STATIC_FRAMEWORK_FILE))
-            .addTransitiveInputs(objcProvider.get(ObjcProvider.DYNAMIC_FRAMEWORK_FILE))
-            .addOutputs(sourceFilesToHeadersListFiles.values())
-            .build(ruleContext));
-  }
-
   private void registerArchiveActions(List<Artifact> objFiles, Artifact archive) {
     Artifact objList = intermediateArtifacts.archiveObjList();
     registerObjFilelistAction(objFiles, objList);