Merge the baseline coverage code path into the coverage codepath.

We're currently doing too much work for baseline coverage - every rule creates
an action for its entire transitive closure; these actions are added to the
output group for baseline coverage, but not transitively accumulated.

It would be better for every rule to create an action for local baseline
coverage, and to aggregate the baseline coverage artifacts down the dependency
tree.

By merging the code paths, the InstrumentedFilesCollector can perform the
aggregation, because it can distinguish local and transitive files; I'm
planning to implement that in a subsequent change.

--
MOS_MIGRATED_REVID=101914334

9 files changed, 68 insertions, 44 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
index 5380cf7ca9..fea33aee36 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/OutputFileConfiguredTarget.java
@@ -68,6 +68,12 @@ public class OutputFileConfiguredTarget extends FileConfiguredTarget
   }
 
   @Override
+  public NestedSet<Artifact> getBaselineCoverageArtifacts() {
+    return getProvider(InstrumentedFilesProvider.class, InstrumentedFilesProviderImpl.EMPTY)
+        .getBaselineCoverageArtifacts();
+  }
+
+  @Override
   public Map<String, String> getExtraEnv() {
     return ImmutableMap.of();
   }
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 1dde2a44fd..1b202340a1 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
@@ -44,7 +44,6 @@ import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
-import com.google.devtools.build.lib.rules.test.BaselineCoverageAction;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.FileType;
@@ -301,7 +300,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     RuleConfiguredTargetBuilder ruleBuilder = new RuleConfiguredTargetBuilder(ruleContext);
     addTransitiveInfoProviders(
         ruleContext, cppConfiguration, common, ruleBuilder, filesToBuild, ccCompilationOutputs,
-        cppCompilationContext, linkingOutputs, dwoArtifacts, transitiveLipoInfo);
+        cppCompilationContext, linkingOutputs, dwoArtifacts, transitiveLipoInfo, fake);
 
     Map<Artifact, IncludeScannable> scannableMap = new LinkedHashMap<>();
     if (cppConfiguration.isLipoContextCollector()) {
@@ -325,8 +324,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
             cppCompilationContext, ImmutableMap.copyOf(scannableMap)))
         .addProvider(CppLinkAction.Context.class, linkContext)
         .addSkylarkTransitiveInfo(CcSkylarkApiProvider.NAME, new CcSkylarkApiProvider())
-        .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE,
-            createBaselineCoverageArtifacts(ruleContext, common, ccCompilationOutputs, fake))
         .build();
   }
 
@@ -569,19 +566,6 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     return builder.build();
   }
 
-  private static NestedSet<Artifact> createBaselineCoverageArtifacts(
-      RuleContext context, CcCommon common, CcCompilationOutputs compilationOutputs,
-      boolean fake) {
-    if (!TargetUtils.isTestRule(context.getRule()) && !fake) {
-      Iterable<Artifact> objectFiles = compilationOutputs.getObjectFiles(
-          CppHelper.usePic(context, !isLinkShared(context)));
-      return BaselineCoverageAction.getBaselineCoverageArtifacts(context,
-          common.getInstrumentedFilesProvider(objectFiles).getInstrumentedFiles());
-    } else {
-      return NestedSetBuilder.emptySet(Order.STABLE_ORDER);
-    }
-  }
-
   private static void addTransitiveInfoProviders(
       RuleContext ruleContext,
       CppConfiguration cppConfiguration,
@@ -592,10 +576,14 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       CppCompilationContext cppCompilationContext,
       CcLinkingOutputs linkingOutputs,
       DwoArtifactsCollector dwoArtifacts,
-      TransitiveLipoInfoProvider transitiveLipoInfo) {
+      TransitiveLipoInfoProvider transitiveLipoInfo,
+      boolean fake) {
     List<Artifact> instrumentedObjectFiles = new ArrayList<>();
     instrumentedObjectFiles.addAll(ccCompilationOutputs.getObjectFiles(false));
     instrumentedObjectFiles.addAll(ccCompilationOutputs.getObjectFiles(true));
+    InstrumentedFilesProvider instrumentedFilesProvider = common.getInstrumentedFilesProvider(
+        instrumentedObjectFiles, !TargetUtils.isTestRule(ruleContext.getRule()) && !fake);
+
     builder
         .setFilesToBuild(filesToBuild)
         .add(CppCompilationContext.class, cppCompilationContext)
@@ -605,8 +593,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
                 ruleContext, linkingOutputs.getExecutionDynamicLibraries())))
         .add(CcNativeLibraryProvider.class, new CcNativeLibraryProvider(
             collectTransitiveCcNativeLibraries(ruleContext, linkingOutputs.getDynamicLibraries())))
-        .add(InstrumentedFilesProvider.class, common.getInstrumentedFilesProvider(
-            instrumentedObjectFiles))
+        .add(InstrumentedFilesProvider.class, instrumentedFilesProvider)
         .add(CppDebugFileProvider.class, new CppDebugFileProvider(
             dwoArtifacts.getDwoArtifacts(), dwoArtifacts.getPicDwoArtifacts()))
         .addOutputGroup(OutputGroupProvider.TEMP_FILES,
@@ -614,7 +601,9 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
         .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE,
             common.getFilesToCompile(ccCompilationOutputs))
         .addOutputGroup(OutputGroupProvider.COMPILATION_PREREQUISITES,
-            CcCommon.collectCompilationPrerequisites(ruleContext, cppCompilationContext));
+            CcCommon.collectCompilationPrerequisites(ruleContext, cppCompilationContext))
+        .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE,
+            instrumentedFilesProvider.getBaselineCoverageArtifacts());
   }
 
   private static NestedSet<Artifact> collectExecutionDynamicLibraryArtifacts(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 26340fb82b..aff5dd7143 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -487,11 +487,13 @@ public final class CcCommon {
         compilationOutputs.getObjectFiles(CppHelper.usePic(ruleContext, false)));
   }
 
-  InstrumentedFilesProvider getInstrumentedFilesProvider(Iterable<Artifact> files) {
+  InstrumentedFilesProvider getInstrumentedFilesProvider(Iterable<Artifact> files,
+      boolean withBaselineCoverage) {
     return cppConfiguration.isLipoContextCollector()
         ? InstrumentedFilesProviderImpl.EMPTY
         : InstrumentedFilesCollector.collect(
-            ruleContext, CppRuleClasses.INSTRUMENTATION_SPEC, CC_METADATA_COLLECTOR, files);
+            ruleContext, CppRuleClasses.INSTRUMENTATION_SPEC, CC_METADATA_COLLECTOR, files,
+            withBaselineCoverage);
   }
 
   /**
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 cc5a02a054..b80fe17688 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
@@ -37,7 +37,6 @@ import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
-import com.google.devtools.build.lib.rules.test.BaselineCoverageAction;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.util.FileType;
@@ -258,7 +257,7 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
     instrumentedObjectFiles.addAll(info.getCcCompilationOutputs().getObjectFiles(false));
     instrumentedObjectFiles.addAll(info.getCcCompilationOutputs().getObjectFiles(true));
     InstrumentedFilesProvider instrumentedFilesProvider =
-        common.getInstrumentedFilesProvider(instrumentedObjectFiles);
+        common.getInstrumentedFilesProvider(instrumentedObjectFiles, /*withBaselineCoverage=*/true);
     targetBuilder
         .setFilesToBuild(filesToBuild)
         .addProviders(info.getProviders())
@@ -269,9 +268,8 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
         // Remove this?
         .add(CppRunfilesProvider.class, new CppRunfilesProvider(staticRunfiles, sharedRunfiles))
         .addOutputGroup(OutputGroupProvider.HIDDEN_TOP_LEVEL, artifactsToForce)
-        .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE, BaselineCoverageAction
-                .getBaselineCoverageArtifacts(ruleContext,
-                    instrumentedFilesProvider.getInstrumentedFiles()));
+        .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE,
+            instrumentedFilesProvider.getBaselineCoverageArtifacts());
 
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
index d786c37517..e17ba06779 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCommon.java
@@ -43,7 +43,6 @@ import com.google.devtools.build.lib.rules.cpp.CppCompilationContext;
 import com.google.devtools.build.lib.rules.cpp.LinkerInput;
 import com.google.devtools.build.lib.rules.java.DirectDependencyProvider.Dependency;
 import com.google.devtools.build.lib.rules.java.JavaCompilationArgs.ClasspathType;
-import com.google.devtools.build.lib.rules.test.BaselineCoverageAction;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector.LocalMetadataCollector;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
@@ -519,20 +518,16 @@ public class JavaCommon {
 
   public void addTransitiveInfoProviders(RuleConfiguredTargetBuilder builder,
       NestedSet<Artifact> filesToBuild, @Nullable Artifact classJar) {
-    InstrumentedFilesProvider instrumentedFilesProvider =
-        InstrumentedFilesCollector.collect(ruleContext, semantics.getCoverageInstrumentationSpec(),
-            JAVA_METADATA_COLLECTOR, filesToBuild);
+    InstrumentedFilesProvider instrumentedFilesProvider = InstrumentedFilesCollector.collect(
+        ruleContext, semantics.getCoverageInstrumentationSpec(), JAVA_METADATA_COLLECTOR,
+        filesToBuild, /*withBaselineCoverage*/!TargetUtils.isTestRule(ruleContext.getTarget()));
 
     builder
         .add(InstrumentedFilesProvider.class, instrumentedFilesProvider)
         .add(JavaExportsProvider.class, new JavaExportsProvider(collectTransitiveExports()))
-        .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, getFilesToCompile(classJar));
-
-    if (!TargetUtils.isTestRule(ruleContext.getTarget())) {
-      builder.addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE,
-          BaselineCoverageAction.getBaselineCoverageArtifacts(ruleContext,
-              instrumentedFilesProvider.getInstrumentedFiles()));
-    }
+        .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, getFilesToCompile(classJar))
+        .addOutputGroup(OutputGroupProvider.BASELINE_COVERAGE,
+            instrumentedFilesProvider.getBaselineCoverageArtifacts());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
index f83c798c36..fc281d8f10 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/BaselineCoverageAction.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.rules.test;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.eventbus.EventBus;
@@ -41,7 +42,8 @@ import java.util.List;
 /**
  * Generates baseline (empty) coverage for the given non-test target.
  */
-public class BaselineCoverageAction extends AbstractFileWriteAction
+@VisibleForTesting
+public final class BaselineCoverageAction extends AbstractFileWriteAction
     implements NotifyOnActionCacheHit {
 
   private final Iterable<Artifact> instrumentedFiles;
@@ -118,7 +120,7 @@ public class BaselineCoverageAction extends AbstractFileWriteAction
    * Returns collection of baseline coverage artifacts associated with the given target.
    * Will always return 0 or 1 elements.
    */
-  public static NestedSet<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext,
+  static NestedSet<Artifact> getBaselineCoverageArtifacts(RuleContext ruleContext,
       Iterable<Artifact> instrumentedFiles) {
     // Baseline coverage artifacts will still go into "testlogs" directory.
     Artifact coverageData = ruleContext.getPackageRelativeArtifact(
@@ -129,5 +131,4 @@ public class BaselineCoverageAction extends AbstractFileWriteAction
 
     return NestedSetBuilder.create(Order.STABLE_ORDER, coverageData);
   }
-
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
index 73f4cc1bc4..e636c8a449 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesCollector.java
@@ -41,10 +41,26 @@ import java.util.List;
 public final class InstrumentedFilesCollector {
   public static InstrumentedFilesProvider collect(RuleContext ruleContext, InstrumentationSpec spec,
       LocalMetadataCollector localMetadataCollector, Iterable<Artifact> rootFiles) {
+    return collect(ruleContext, spec, localMetadataCollector, rootFiles, false);
+  }
+
+  public static InstrumentedFilesProvider collect(RuleContext ruleContext,
+      InstrumentationSpec spec, LocalMetadataCollector localMetadataCollector,
+      Iterable<Artifact> rootFiles, boolean withBaselineCoverage) {
     InstrumentedFilesCollector collector = new InstrumentedFilesCollector(ruleContext, spec,
         localMetadataCollector, rootFiles);
+    NestedSet<Artifact> baselineCoverageArtifacts;
+    if (withBaselineCoverage) {
+      baselineCoverageArtifacts =
+          BaselineCoverageAction.getBaselineCoverageArtifacts(ruleContext,
+              collector.instrumentedFiles);
+    } else {
+      baselineCoverageArtifacts = NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
+    }
     return new InstrumentedFilesProviderImpl(collector.instrumentedFiles,
-        collector.instrumentationMetadataFiles, ImmutableMap.<String, String>of());
+        collector.instrumentationMetadataFiles,
+        baselineCoverageArtifacts,
+        ImmutableMap.<String, String>of());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
index 166c7e27aa..2d06204bcc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProvider.java
@@ -35,6 +35,8 @@ public interface InstrumentedFilesProvider extends TransitiveInfoProvider {
    */
   NestedSet<Artifact> getInstrumentationMetadataFiles();
 
+  NestedSet<Artifact> getBaselineCoverageArtifacts();
+
   /**
    * Returns environment variables which should be set for coverage to function.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
index 90605e605f..b25b632935 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/InstrumentedFilesProviderImpl.java
@@ -28,19 +28,29 @@ public final class InstrumentedFilesProviderImpl implements InstrumentedFilesPro
   public static final InstrumentedFilesProvider EMPTY = new InstrumentedFilesProviderImpl(
       NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER),
       NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER),
+      NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER),
       ImmutableMap.<String, String>of());
 
   private final NestedSet<Artifact> instrumentedFiles;
   private final NestedSet<Artifact> instrumentationMetadataFiles;
+  private final NestedSet<Artifact> baselineCoverageArtifacts;
   private final ImmutableMap<String, String> extraEnv;
 
   public InstrumentedFilesProviderImpl(NestedSet<Artifact> instrumentedFiles,
-      NestedSet<Artifact> instrumentationMetadataFiles, Map<String, String> extraEnv) {
+      NestedSet<Artifact> instrumentationMetadataFiles,
+      NestedSet<Artifact> baselineCoverageArtifacts, Map<String, String> extraEnv) {
     this.instrumentedFiles = instrumentedFiles;
     this.instrumentationMetadataFiles = instrumentationMetadataFiles;
+    this.baselineCoverageArtifacts = baselineCoverageArtifacts;
     this.extraEnv = ImmutableMap.copyOf(extraEnv);
   }
 
+  public InstrumentedFilesProviderImpl(NestedSet<Artifact> instrumentedFiles,
+      NestedSet<Artifact> instrumentationMetadataFiles, Map<String, String> extraEnv) {
+    this(instrumentedFiles, instrumentationMetadataFiles,
+        NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER), extraEnv);
+  }
+
   @Override
   public NestedSet<Artifact> getInstrumentedFiles() {
     return instrumentedFiles;
@@ -52,6 +62,11 @@ public final class InstrumentedFilesProviderImpl implements InstrumentedFilesPro
   }
 
   @Override
+  public NestedSet<Artifact> getBaselineCoverageArtifacts() {
+    return baselineCoverageArtifacts;
+  }
+
+  @Override
   public Map<String, String> getExtraEnv() {
     return extraEnv;
   }