Create a feature for per-object-file debug info files and move -gsplit-dwarf to it.

A side effect is that this made --save_temps (probably) work with --use_fission . Previously, the DWO file was also an output of the temps actions.

--
MOS_MIGRATED_REVID=123117200

6 files changed, 84 insertions, 70 deletions

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 1a88ae9967..44623f6974 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
@@ -89,6 +89,7 @@ public final class CcCommon {
       CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
       CppRuleClasses.INCLUDE_PATHS,
       CppRuleClasses.PIC,
+      CppRuleClasses.PER_OBJECT_DEBUG_INFO,
       CppRuleClasses.PREPROCESSOR_DEFINES);
 
   /** C++ configuration */
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 08a1785e5d..27c8d7099a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -163,7 +163,6 @@ public class CppCompileAction extends AbstractAction
   private final BuildConfiguration configuration;
   protected final Artifact outputFile;
   private final Label sourceLabel;
-  private final Artifact dwoFile;
   private final Artifact optionalSourceFile;
   private final NestedSet<Artifact> mandatoryInputs;
   private final boolean shouldScanIncludes;
@@ -268,7 +267,6 @@ public class CppCompileAction extends AbstractAction
     this.configuration = configuration;
     this.sourceLabel = sourceLabel;
     this.outputFile = Preconditions.checkNotNull(outputFile);
-    this.dwoFile = dwoFile;
     this.optionalSourceFile = optionalSourceFile;
     this.context = context;
     this.specialInputsHandler = specialInputsHandler;
@@ -283,7 +281,6 @@ public class CppCompileAction extends AbstractAction
         new CppCompileCommandLine(
             sourceFile,
             dotdFile,
-            dwoFile,
             copts,
             coptsFilter,
             features,
@@ -473,15 +470,6 @@ public class CppCompileAction extends AbstractAction
     return outputFile;
   }
 
-  /**
-   * Returns the path of the debug info output file (when debug info is
-   * spliced out of the .o file via fission).
-   */
-  @Nullable
-  Artifact getDwoFile() {
-    return dwoFile;
-  }
-
   protected PathFragment getInternalOutputFile() {
     return outputFile.getExecPath();
   }
@@ -1254,7 +1242,6 @@ public class CppCompileAction extends AbstractAction
   public final class CppCompileCommandLine {
     private final Artifact sourceFile;
     private final DotdFile dotdFile;
-    @Nullable private final Artifact dwoFile;
     private final List<String> copts;
     private final Predicate<String> coptsFilter;
     private final Collection<String> features;
@@ -1265,7 +1252,6 @@ public class CppCompileAction extends AbstractAction
     public CppCompileCommandLine(
         Artifact sourceFile,
         DotdFile dotdFile,
-        @Nullable Artifact dwoFile,
         ImmutableList<String> copts,
         Predicate<String> coptsFilter,
         Collection<String> features,
@@ -1275,7 +1261,6 @@ public class CppCompileAction extends AbstractAction
       this.sourceFile = Preconditions.checkNotNull(sourceFile);
       this.dotdFile = CppFileTypes.mustProduceDotdFile(sourceFile.getPath().toString())
                       ? Preconditions.checkNotNull(dotdFile) : null;
-      this.dwoFile = dwoFile;
       this.copts = Preconditions.checkNotNull(copts);
       this.coptsFilter = coptsFilter;
       this.features = Preconditions.checkNotNull(features);
@@ -1334,14 +1319,6 @@ public class CppCompileAction extends AbstractAction
       // that will be re-added by the feature flags.
       addFilteredOptions(options, featureConfiguration.getCommandLine(actionName, variables));
 
-      // TODO(bazel-team): Move this into a feature; more specifically, create a feature for both
-      // the amount of debug information requested, and whether the debug info is written in a
-      // split out file. Until then, keep this before the user-provided copts so it can be
-      // overwritten.
-      if (dwoFile != null) {
-        options.add("-gsplit-dwarf");
-      }
-
       // Users don't expect the explicit copts to be filtered by coptsFilter, add them verbatim.
       // Make sure these are added after the options from the feature configuration, so that
       // those options can be overriden.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 0df93ecd33..1997dc84a2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -61,6 +61,7 @@ public class CppCompileActionBuilder {
   private final NestedSetBuilder<Artifact> mandatoryInputsBuilder;
   private Artifact optionalSourceFile;
   private Artifact outputFile;
+  private Artifact dwoFile;
   private PathFragment tempOutputFile;
   private DotdFile dotdFile;
   private Artifact gcnoFile;
@@ -127,6 +128,7 @@ public class CppCompileActionBuilder {
         .addTransitive(other.mandatoryInputsBuilder.build());
     this.optionalSourceFile = other.optionalSourceFile;
     this.outputFile = other.outputFile;
+    this.dwoFile = other.dwoFile;
     this.tempOutputFile = other.tempOutputFile;
     this.dotdFile = other.dotdFile;
     this.gcnoFile = other.gcnoFile;
@@ -163,25 +165,6 @@ public class CppCompileActionBuilder {
     return mandatoryInputsBuilder.build();
   }
 
-  /**
-   * Returns the .dwo output file that matches the specified .o output file. If Fission mode
-   * isn't enabled for this build, this is null (we don't produce .dwo files in that case).
-   */
-  private static Artifact getDwoFile(RuleContext ruleContext, Artifact outputFile,
-      CppConfiguration cppConfiguration) {
-
-    // Only create .dwo's for .o compilations (i.e. not .ii or .S).
-    boolean isObjectOutput = CppFileTypes.OBJECT_FILE.matches(outputFile.getExecPath())
-        || CppFileTypes.PIC_OBJECT_FILE.matches(outputFile.getExecPath());
-
-    // Note configurations can be null for tests.
-    if (cppConfiguration != null && cppConfiguration.useFission() && isObjectOutput) {
-      return ruleContext.getRelatedArtifact(outputFile.getRootRelativePath(), ".dwo");
-    } else {
-      return null;
-    }
-  }
-
   private static Predicate<String> getNocoptPredicate(Collection<Pattern> patterns) {
     final ImmutableList<Pattern> finalPatterns = ImmutableList.copyOf(patterns);
     if (finalPatterns.isEmpty()) {
@@ -316,7 +299,7 @@ public class CppCompileActionBuilder {
           outputFile,
           dotdFile,
           gcnoFile,
-          getDwoFile(ruleContext, outputFile, cppConfiguration),
+          dwoFile,
           optionalSourceFile,
           configuration,
           cppConfiguration,
@@ -398,6 +381,11 @@ public class CppCompileActionBuilder {
     return this;
   }
 
+  public CppCompileActionBuilder setDwoFile(Artifact dwoFile) {
+    this.dwoFile = dwoFile;
+    return this;
+  }
+
   Artifact getOutputFile() {
     return outputFile;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 7f07352cd7..2fc10e733b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -730,6 +730,25 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
             + "}",
             toolchainBuilder);
       }
+
+      if (!features.contains("per_object_debug_info")) {
+        TextFormat.merge(""
+            + "feature {"
+            + "  name: 'per_object_debug_info'"
+            + "  flag_set {"
+            + "    action: 'c-compile'"
+            + "    action: 'c++-compile'"
+            + "    action: 'assemble'"
+            + "    action: 'preprocess-assemble'"
+            + "    expand_if_all_available: 'per_object_debug_info_file'"
+            + "    flag_group {"
+            + "      flag: '-gsplit-dwarf'"
+            + "    }"
+            + "  }"
+            + "}",
+            toolchainBuilder);
+      }
+
       if (!features.contains("preprocessor_defines")) {
         TextFormat.merge(""
             + "feature {"
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 76e49c40e9..2304de349f 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
@@ -88,6 +88,10 @@ public final class CppModel {
     cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
   }
 
+  private Artifact getDwoFile(Artifact outputFile) {
+    return ruleContext.getRelatedArtifact(outputFile.getRootRelativePath(), ".dwo");
+  }
+
   /**
    * If the cpp compilation is a fake, then it creates only a single compile action without PIC.
    * Defaults to false.
@@ -355,7 +359,8 @@ public final class CppModel {
       boolean usePic,
       PathFragment ccRelativeName,
       PathFragment autoFdoImportPath,
-      Artifact gcnoFile) {
+      Artifact gcnoFile,
+      Artifact dwoFile) {
     CcToolchainFeatures.Variables.Builder buildVariables =
         new CcToolchainFeatures.Variables.Builder();
     
@@ -422,6 +427,10 @@ public final class CppModel {
       buildVariables.addVariable("gcov_gcno_file", gcnoFile.getExecPathString());
     }
 
+    if (dwoFile != null) {
+      buildVariables.addVariable("per_object_debug_info_file", dwoFile.getExecPathString());
+    }
+
     buildVariables.addAllVariables(CppHelper.getToolchain(ruleContext).getBuildVariables());
     
     for (VariablesExtension extension : variablesExtensions) {
@@ -454,7 +463,7 @@ public final class CppModel {
       // - the compiled source file is the module map
       // - it creates a header module (.pcm file).
       createSourceAction(outputName, result, env, moduleMapArtifact, builder, ".pcm", ".pcm.d",
-          /*addObject=*/false, /*enableCoverage=*/false);
+          /*addObject=*/false, /*enableCoverage=*/false, /*generateDwo*/false);
     }
 
     for (Pair<Artifact, Label> source : sourceFiles) {
@@ -468,7 +477,8 @@ public final class CppModel {
         createHeaderAction(outputName, result, env, builder);
       } else {
         createSourceAction(outputName, result, env, sourceArtifact, builder, ".o", ".d",
-            /*addObject=*/true, isCodeCoverageEnabled());
+            /*addObject=*/true, isCodeCoverageEnabled(),
+            /*generateDwo*/cppConfiguration.useFission());
       }
     }
 
@@ -484,7 +494,7 @@ public final class CppModel {
         // If we generate pic actions, we prefer the header actions to use the pic artifacts.
         .setPicMode(this.getGeneratePicActions());
     setupBuildVariables(builder, this.getGeneratePicActions(), /*ccRelativeName=*/null,
-        /*autoFdoImportPath=*/null, /*gcnoFile=*/null);
+        /*autoFdoImportPath=*/null, /*gcnoFile=*/null, /*dwoFile=*/null);
     semantics.finalizeCompileActionBuilder(ruleContext, builder);
     CppCompileAction compileAction = builder.build();
     env.registerAction(compileAction);
@@ -500,7 +510,8 @@ public final class CppModel {
       String outputExtension,
       String dependencyFileExtension,
       boolean addObject,
-      boolean enableCoverage) {
+      boolean enableCoverage,
+      boolean generateDwo) {
     PathFragment ccRelativeName = semantics.getEffectiveSourcePath(sourceArtifact);
     if (cppConfiguration.isLipoOptimization()) {
       // TODO(bazel-team): we shouldn't be needing this, merging context with the binary
@@ -518,20 +529,21 @@ public final class CppModel {
     if (fake) {
       boolean usePic = !generateNoPicAction;
       createFakeSourceAction(outputName, result, env, builder, outputExtension,
-          dependencyFileExtension, addObject, ccRelativeName, sourceArtifact.getExecPath(), usePic);
+          dependencyFileExtension, addObject, ccRelativeName, sourceArtifact.getExecPath(), usePic,
+          generateDwo);
     } else {
       // Create PIC compile actions (same as non-PIC, but use -fPIC and
       // generate .pic.o, .pic.d, .pic.gcno instead of .o, .d, .gcno.)
       if (generatePicAction) {
-        CppCompileActionBuilder picBuilder =
-            copyAsPicBuilder(builder, outputName, outputExtension, dependencyFileExtension);
+        Artifact outputFile = ruleContext.getRelatedArtifact(outputName, ".pic" + outputExtension);
+        CppCompileActionBuilder picBuilder = copyAsPicBuilder(
+            builder, outputName, outputFile, dependencyFileExtension);
         Artifact gcnoFile =
             enableCoverage ? ruleContext.getRelatedArtifact(outputName, ".pic.gcno") : null;
-        if (gcnoFile != null) {
-          picBuilder.setGcnoFile(gcnoFile);
-        }
+        Artifact dwoFile = generateDwo ? getDwoFile(outputFile) : null;
+
         setupBuildVariables(picBuilder, /*usePic=*/ true, ccRelativeName,
-            sourceArtifact.getExecPath(), gcnoFile);
+            sourceArtifact.getExecPath(), gcnoFile, dwoFile);
 
         if (maySaveTemps) {
           result.addTemps(
@@ -539,6 +551,9 @@ public final class CppModel {
                 ccRelativeName));
         }
 
+        picBuilder.setGcnoFile(gcnoFile);
+        picBuilder.setDwoFile(dwoFile);
+
         semantics.finalizeCompileActionBuilder(ruleContext, picBuilder);
         CppCompileAction picAction = picBuilder.build();
         env.registerAction(picAction);
@@ -550,9 +565,9 @@ public final class CppModel {
             result.addLTOBitcodeFile(picAction.getOutputFile());
           }
         }
-        if (picAction.getDwoFile() != null) {
+        if (dwoFile != null) {
           // Host targets don't produce .dwo files.
-          result.addPicDwoFile(picAction.getDwoFile());
+          result.addPicDwoFile(dwoFile);
         }
         if (cppConfiguration.isLipoContextCollector() && !generateNoPicAction) {
           result.addLipoScannable(picAction);
@@ -560,19 +575,21 @@ public final class CppModel {
       }
 
       if (generateNoPicAction) {
+        Artifact noPicOutputFile = ruleContext.getRelatedArtifact(outputName, outputExtension);
         builder
-            .setOutputFile(ruleContext.getRelatedArtifact(outputName, outputExtension))
+            .setOutputFile(noPicOutputFile)
             .setDotdFile(outputName, dependencyFileExtension);
+
         // Create non-PIC compile actions
         Artifact gcnoFile =
             !cppConfiguration.isLipoOptimization() && enableCoverage
                 ? ruleContext.getRelatedArtifact(outputName, ".gcno")
                 : null;
-        if (gcnoFile != null) {
-          builder.setGcnoFile(gcnoFile);
-        }
+
+        Artifact noPicDwoFile = generateDwo ? getDwoFile(noPicOutputFile) : null;
+
         setupBuildVariables(builder, /*usePic=*/false, ccRelativeName, sourceArtifact.getExecPath(),
-            gcnoFile);
+            gcnoFile, noPicDwoFile);
 
         if (maySaveTemps) {
           result.addTemps(
@@ -584,6 +601,9 @@ public final class CppModel {
                   ccRelativeName));
         }
 
+        builder.setGcnoFile(gcnoFile);
+        builder.setDwoFile(noPicDwoFile);
+
         semantics.finalizeCompileActionBuilder(ruleContext, builder);
         CppCompileAction compileAction = builder.build();
         env.registerAction(compileAction);
@@ -595,9 +615,9 @@ public final class CppModel {
             result.addLTOBitcodeFile(objectFile);
           }
         }
-        if (compileAction.getDwoFile() != null) {
+        if (noPicDwoFile != null) {
           // Host targets don't produce .dwo files.
-          result.addDwoFile(compileAction.getDwoFile());
+          result.addDwoFile(noPicDwoFile);
         }
         if (cppConfiguration.isLipoContextCollector()) {
           result.addLipoScannable(compileAction);
@@ -609,7 +629,7 @@ public final class CppModel {
   private void createFakeSourceAction(PathFragment outputName, CcCompilationOutputs.Builder result,
       AnalysisEnvironment env, CppCompileActionBuilder builder, String outputExtension,
       String dependencyFileExtension, boolean addObject, PathFragment ccRelativeName,
-      PathFragment execPath, boolean usePic) {
+      PathFragment execPath, boolean usePic, boolean generateDwo) {
     if (usePic) {
       outputExtension = ".pic" + outputExtension;
       dependencyFileExtension = ".pic" + dependencyFileExtension;
@@ -623,7 +643,10 @@ public final class CppModel {
         .setOutputFile(outputFile)
         .setDotdFile(outputName, dependencyFileExtension)
         .setTempOutputFile(tempOutputName);
-    setupBuildVariables(builder, usePic, ccRelativeName, execPath, null);
+
+    Artifact dwoFile = generateDwo ? getDwoFile(outputFile) : null;
+    builder.setDwoFile(dwoFile);
+    setupBuildVariables(builder, usePic, ccRelativeName, execPath, null, dwoFile);
     semantics.finalizeCompileActionBuilder(ruleContext, builder);
     CppCompileAction action = builder.build();
     env.registerAction(action);
@@ -805,12 +828,13 @@ public final class CppModel {
    * changing output and dotd file names.
    */
   private CppCompileActionBuilder copyAsPicBuilder(CppCompileActionBuilder builder,
-      PathFragment outputName, String outputExtension, String dependencyFileExtension) {
+      PathFragment outputName, Artifact outputFile, String dependencyFileExtension) {
     CppCompileActionBuilder picBuilder = new CppCompileActionBuilder(builder);
     picBuilder
         .setPicMode(true)
-        .setOutputFile(ruleContext.getRelatedArtifact(outputName, ".pic" + outputExtension))
+        .setOutputFile(outputFile)
         .setDotdFile(outputName, ".pic" + dependencyFileExtension);
+
     return picBuilder;
   }
 
@@ -838,7 +862,7 @@ public final class CppModel {
     dBuilder
         .setOutputFile(ruleContext.getRelatedArtifact(outputName, picExt + iExt))
         .setDotdFile(outputName, picExt + iExt + ".d");
-    setupBuildVariables(dBuilder, usePic, ccRelativeName, source.getExecPath(), null);
+    setupBuildVariables(dBuilder, usePic, ccRelativeName, source.getExecPath(), null, null);
     semantics.finalizeCompileActionBuilder(ruleContext, dBuilder);
     CppCompileAction dAction = dBuilder.build();
     ruleContext.registerAction(dAction);
@@ -847,7 +871,7 @@ public final class CppModel {
     sdBuilder
         .setOutputFile(ruleContext.getRelatedArtifact(outputName, picExt + ".s"))
         .setDotdFile(outputName, picExt + ".s.d");
-    setupBuildVariables(sdBuilder, usePic, ccRelativeName, source.getExecPath(), null);
+    setupBuildVariables(sdBuilder, usePic, ccRelativeName, source.getExecPath(), null, null);
     semantics.finalizeCompileActionBuilder(ruleContext, sdBuilder);
     CppCompileAction sdAction = sdBuilder.build();
     ruleContext.registerAction(sdAction);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
index 81c7fb8384..2d3d0b5c4a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
@@ -171,6 +171,11 @@ public class CppRuleClasses {
   public static final String NO_LEGACY_FEATURES = "no_legacy_features";
 
   /**
+   * A string constant for the feature that makes us build per-object debug info files.
+   */
+  public static final String PER_OBJECT_DEBUG_INFO = "per_object_debug_info";
+
+  /**
    * A string constant for the PIC feature.
    *
    * <p>If this feature is active (currently it cannot be switched off) and PIC compilation is