Introduce -c source_file -o output_file build variables

Prior to this cl CompileCommandLine would (almost) unconditionally emit -c and
-o flags. This cl removes this logic and relies on crosstool to emit these
flags. This is another small step towards platform independent C++ rules.

Memory use is not affected, since the build variables used by this cl are already
exposed, this cl just forces crosstools to use it.

Encore of https://github.com/bazelbuild/bazel/commit/f26e8694ae78599b3e2004e3360eaf3443fa53a6.

RELNOTES: None.
PiperOrigin-RevId: 184981106

9 files changed, 111 insertions, 81 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java
index e8d5a93d1c..1ac7da832d 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.actions;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import java.util.List;
 
@@ -33,5 +34,6 @@ public interface CommandAction extends Action, ExecutionInfoSpecifier {
   ImmutableMap<String, String> getEnvironment();
 
   /** Returns inputs to this action, including inputs that may be pruned. */
+  @VisibleForTesting // productionVisibility = Visibility.PRIVATE
   Iterable<Artifact> getPossibleInputsForTesting();
 }
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 8d599be1f1..fa2c96e789 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
@@ -119,7 +119,6 @@ public final class CcCommon {
   private static final ImmutableSet<String> DEFAULT_FEATURES =
       ImmutableSet.of(
           CppRuleClasses.DEPENDENCY_FILE,
-          CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET,
           CppRuleClasses.RANDOM_SEED,
           CppRuleClasses.MODULE_MAPS,
           CppRuleClasses.MODULE_MAP_HOME_CWD,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
index 5f2a3d8a52..7eef57f135 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
@@ -24,7 +24,6 @@ import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.skyframe.serialization.InjectingObjectCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
-import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystemProvider;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -40,7 +39,6 @@ public final class CompileCommandLine {
       new CompileCommandLine_AutoCodec();
 
   private final Artifact sourceFile;
-  private final Artifact outputFile;
   private final CoptsFilter coptsFilter;
   private final FeatureConfiguration featureConfiguration;
   private final PathFragment crosstoolTopPathFragment;
@@ -52,7 +50,6 @@ public final class CompileCommandLine {
   @VisibleForSerialization
   CompileCommandLine(
       Artifact sourceFile,
-      Artifact outputFile,
       CoptsFilter coptsFilter,
       FeatureConfiguration featureConfiguration,
       PathFragment crosstoolTopPathFragment,
@@ -60,7 +57,6 @@ public final class CompileCommandLine {
       String actionName,
       DotdFile dotdFile) {
     this.sourceFile = Preconditions.checkNotNull(sourceFile);
-    this.outputFile = Preconditions.checkNotNull(outputFile);
     this.coptsFilter = coptsFilter;
     this.featureConfiguration = Preconditions.checkNotNull(featureConfiguration);
     this.crosstoolTopPathFragment = crosstoolTopPathFragment;
@@ -80,8 +76,12 @@ public final class CompileCommandLine {
     return featureConfiguration.getEnvironmentVariables(actionName, variables);
   }
 
-  protected List<String> getArgv(
-      PathFragment outputFile, CcToolchainFeatures.Variables overwrittenVariables) {
+  /**
+   * @param overwrittenVariables: Variables that will overwrite original build variables. When null,
+   *     unmodified original variables are used.
+   */
+  protected List<String> getArguments(
+      @Nullable CcToolchainFeatures.Variables overwrittenVariables) {
     List<String> commandLine = new ArrayList<>();
 
     // first: The command name.
@@ -96,17 +96,6 @@ public final class CompileCommandLine {
 
     // second: The compiler options.
     commandLine.addAll(getCompilerOptions(overwrittenVariables));
-
-    if (!featureConfiguration.isEnabled(CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET)) {
-      // third: The file to compile!
-      commandLine.add("-c");
-      commandLine.add(sourceFile.getExecPathString());
-
-      // finally: The output file. (Prefixed with -o).
-      commandLine.add("-o");
-      commandLine.add(outputFile.getPathString());
-    }
-
     return commandLine;
   }
 
@@ -124,19 +113,6 @@ public final class CompileCommandLine {
     addFilteredOptions(
         options, featureConfiguration.getPerFeatureExpansions(actionName, updatedVariables));
 
-    if (!featureConfiguration.isEnabled("compile_action_flags_in_flag_set")) {
-      if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) {
-        options.add("-S");
-      } else if (FileType.contains(
-          outputFile,
-          CppFileTypes.PREPROCESSED_C,
-          CppFileTypes.PREPROCESSED_CPP,
-          CppFileTypes.PIC_PREPROCESSED_C,
-          CppFileTypes.PIC_PREPROCESSED_CPP)) {
-        options.add("-E");
-      }
-    }
-
     return options;
   }
 
@@ -182,19 +158,16 @@ public final class CompileCommandLine {
 
   public static Builder builder(
       Artifact sourceFile,
-      Artifact outputFile,
       CoptsFilter coptsFilter,
       String actionName,
       PathFragment crosstoolTopPathFragment,
       DotdFile dotdFile) {
-    return new Builder(
-        sourceFile, outputFile, coptsFilter, actionName, crosstoolTopPathFragment, dotdFile);
+    return new Builder(sourceFile, coptsFilter, actionName, crosstoolTopPathFragment, dotdFile);
   }
 
   /** A builder for a {@link CompileCommandLine}. */
   public static final class Builder {
     private final Artifact sourceFile;
-    private final Artifact outputFile;
     private CoptsFilter coptsFilter;
     private FeatureConfiguration featureConfiguration;
     private CcToolchainFeatures.Variables variables = Variables.EMPTY;
@@ -205,7 +178,6 @@ public final class CompileCommandLine {
     public CompileCommandLine build() {
       return new CompileCommandLine(
           Preconditions.checkNotNull(sourceFile),
-          Preconditions.checkNotNull(outputFile),
           Preconditions.checkNotNull(coptsFilter),
           Preconditions.checkNotNull(featureConfiguration),
           Preconditions.checkNotNull(crosstoolTopPathFragment),
@@ -216,13 +188,11 @@ public final class CompileCommandLine {
 
     private Builder(
         Artifact sourceFile,
-        Artifact outputFile,
         CoptsFilter coptsFilter,
         String actionName,
         PathFragment crosstoolTopPathFragment,
         DotdFile dotdFile) {
       this.sourceFile = sourceFile;
-      this.outputFile = outputFile;
       this.coptsFilter = coptsFilter;
       this.actionName = actionName;
       this.crosstoolTopPathFragment = crosstoolTopPathFragment;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
index bab2726259..a12e21ddad 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java
@@ -70,6 +70,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'preprocess-assemble'",
@@ -81,6 +83,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'linkstamp-compile'",
@@ -92,6 +96,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c-compile'",
@@ -103,6 +109,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-compile'",
@@ -114,6 +122,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-header-parsing'",
@@ -125,6 +135,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-header-preprocessing'",
@@ -136,6 +148,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-module-compile'",
@@ -147,6 +161,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 "action_config {",
                 "  config_name: 'c++-module-codegen'",
@@ -158,6 +174,8 @@ public class CppActionConfigs {
                 "  implies: 'user_compile_flags'",
                 "  implies: 'sysroot'",
                 "  implies: 'unfiltered_compile_flags'",
+                "  implies: 'compiler_input_flags'",
+                "  implies: 'compiler_output_flags'",
                 "}",
                 ifTrue(
                     !existingFeatureNames.contains(CppRuleClasses.LEGACY_COMPILE_FLAGS),
@@ -1060,6 +1078,68 @@ public class CppActionConfigs {
                     "      flag: '@%{linker_param_file}'",
                     "    }",
                     "  }",
+                    "}"),
+                ifTrue(
+                    !existingFeatureNames.contains("compiler_input_flags"),
+                    "feature {",
+                    "  name: 'compiler_input_flags'",
+                    "  enabled: true",
+                    "  flag_set {",
+                    "    action: 'assemble'",
+                    "    action: 'preprocess-assemble'",
+                    "    action: 'c-compile'",
+                    "    action: 'c++-compile'",
+                    "    action: 'linkstamp-compile'",
+                    "    action: 'c++-module-compile'",
+                    "    action: 'c++-module-codegen'",
+                    "    action: 'objc-compile'",
+                    "    action: 'objc++-compile'",
+                    "    action: 'c++-header-preprocessing'",
+                    "    action: 'c++-header-parsing'",
+                    "    action: 'lto-backend'",
+                    "    expand_if_all_available: 'source_file'",
+                    "    flag_group {",
+                    "      flag: '-c'",
+                    "      flag: '%{source_file}'",
+                    "    }",
+                    "  }",
+                    "}"),
+                ifTrue(
+                    !existingFeatureNames.contains("compiler_output_flags"),
+                    "feature {",
+                    "  name: 'compiler_output_flags'",
+                    "  enabled: true",
+                    "  flag_set {",
+                    "    action: 'assemble'",
+                    "    action: 'preprocess-assemble'",
+                    "    action: 'c-compile'",
+                    "    action: 'c++-compile'",
+                    "    action: 'linkstamp-compile'",
+                    "    action: 'c++-module-compile'",
+                    "    action: 'c++-module-codegen'",
+                    "    action: 'objc-compile'",
+                    "    action: 'objc++-compile'",
+                    "    action: 'c++-header-preprocessing'",
+                    "    action: 'c++-header-parsing'",
+                    "    action: 'lto-backend'",
+                    "    flag_group {",
+                    "      expand_if_all_available: 'output_object_file'",
+                    "      flag: '-o'",
+                    "      flag: '%{output_object_file}'",
+                    "    }",
+                    "    flag_group {",
+                    "      expand_if_all_available: 'output_assembly_file'",
+                    "      flag: '-S'",
+                    "      flag: '-o'",
+                    "      flag: '%{output_assembly_file}'",
+                    "    }",
+                    "    flag_group {",
+                    "      expand_if_all_available: 'output_preprocess_file'",
+                    "      flag: '-E'",
+                    "      flag: '-o'",
+                    "      flag: '%{output_preprocess_file}'",
+                    "    }",
+                    "  }",
                     "}")));
   }
 
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 15cf6a7bfc..91534e580c 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
@@ -322,7 +322,6 @@ public class CppCompileAction extends AbstractAction
     this.compileCommandLine =
         CompileCommandLine.builder(
                 sourceFile,
-                outputFile,
                 coptsFilter,
                 actionName,
                 crosstoolTopPathFragment,
@@ -624,7 +623,7 @@ public class CppCompileAction extends AbstractAction
   @Override
   public List<String> getCmdlineIncludes() {
     ImmutableList.Builder<String> cmdlineIncludes = ImmutableList.builder();
-    List<String> args = getArgv();
+    List<String> args = getArguments();
     for (Iterator<String> argi = args.iterator(); argi.hasNext();) {
       String arg = argi.next();
       if (arg.equals("-include") && argi.hasNext()) {
@@ -686,21 +685,9 @@ public class CppCompileAction extends AbstractAction
     return ImmutableMap.copyOf(environment);
   }
 
-  /**
-   * Returns a new, mutable list of command and arguments (argv) to be passed
-   * to the gcc subprocess.
-   */
-  public final List<String> getArgv() {
-    return getArgv(getInternalOutputFile());
-  }
-
-  protected final List<String> getArgv(PathFragment outputFile) {
-    return compileCommandLine.getArgv(outputFile, overwrittenVariables);
-  }
-
   @Override
   public List<String> getArguments() {
-    return getArgv();
+    return compileCommandLine.getArguments(overwrittenVariables);
   }
 
   @Override
@@ -1050,7 +1037,8 @@ public class CppCompileAction extends AbstractAction
   public ResourceSet estimateResourceConsumptionLocal() {
     // We use a local compile, so much of the time is spent waiting for IO,
     // but there is still significant CPU; hence we estimate 50% cpu usage.
-    return ResourceSet.createWithRamCpuIo(/*memoryMb=*/200, /*cpuUsage=*/0.5, /*ioUsage=*/0.0);
+    return ResourceSet.createWithRamCpuIo(
+        /* memoryMb= */ 200, /* cpuUsage= */ 0.5, /* ioUsage= */ 0.0);
   }
 
   @Override
@@ -1067,10 +1055,10 @@ public class CppCompileAction extends AbstractAction
     // itself is fully determined by the input source files and module maps.
     // A better long-term solution would be to make the compiler to find them automatically and
     // never hand in the .pcm files explicitly on the command line in the first place.
-    f.addStrings(compileCommandLine.getArgv(getInternalOutputFile(), null));
+    f.addStrings(compileCommandLine.getArguments(/* overwrittenVariables= */ null));
 
     /*
-     * getArgv() above captures all changes which affect the compilation
+     * getArguments() above captures all changes which affect the compilation
      * command and hence the contents of the object file.  But we need to
      * also make sure that we reexecute the action if any of the fields
      * that affect whether validateIncludes() will report an error or warning
@@ -1313,9 +1301,9 @@ public class CppCompileAction extends AbstractAction
     message.append(getProgressMessage());
     message.append('\n');
     // Outputting one argument per line makes it easier to diff the results.
-    // The first element in getArgv() is actually the command to execute.
+    // The first element in getArguments() is actually the command to execute.
     String legend = "  Command: ";
-    for (String argument : ShellEscaper.escapeAll(getArgv())) {
+    for (String argument : ShellEscaper.escapeAll(getArguments())) {
       message.append(legend);
       message.append(argument);
       message.append('\n');
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 7940984e6f..62454204fa 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
@@ -147,13 +147,6 @@ public class CppRuleClasses {
   public static final String RANDOM_SEED = "random_seed";
 
   /**
-   * A string constant for the compile_action_flags_in_flag_set feature. This feature is just a
-   * transitional feature which helps telling whether -c and -o options are already in flag_set of
-   * action_config in CROSSTOOL file. Once the transition is done, it should be removed.
-   */
-  public static final String COMPILE_ACTION_FLAGS_IN_FLAG_SET = "compile_action_flags_in_flag_set";
-
-  /**
    * A string constant for the dependency_file feature. This feature generates the .d file.
    */
   public static final String DEPENDENCY_FILE = "dependency_file";
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index acdaafde3a..382d71200d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -207,14 +207,12 @@ public class FakeCppCompileAction extends CppCompileAction {
     // line to write to $TEST_TMPDIR instead.
     final String outputPrefix = "$TEST_TMPDIR/";
     String argv =
-        getArgv(outputFile.getExecPath())
+        getArguments()
             .stream()
             .map(
                 input -> {
                   String result = ShellEscaper.escapeString(input);
-                  // Once -c and -o options are added into action_config, the argument of
-                  // getArgv(outputFile.getExecPath()) won't be used anymore. There will always be
-                  // -c <tempOutputFile>, but here it has to be outputFile, so we replace it.
+                  // Replace -c <tempOutputFile> so it's -c <outputFile>.
                   if (input.equals(tempOutputFile.getPathString())) {
                     result =
                         outputPrefix + ShellEscaper.escapeString(outputFile.getExecPathString());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
index d0f68c97bd..ed3fa2210f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SpawnGccStrategy.java
@@ -41,17 +41,18 @@ public class SpawnGccStrategy implements CppCompileActionContext {
       CppCompileAction action, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
     Iterable<Artifact> inputs = Iterables.concat(action.getInputs(), action.getAdditionalInputs());
-    Spawn spawn = new SimpleSpawn(
-        action,
-        ImmutableList.copyOf(action.getArgv()),
-        ImmutableMap.copyOf(action.getEnvironment()),
-        ImmutableMap.copyOf(action.getExecutionInfo()),
-        EmptyRunfilesSupplier.INSTANCE,
-        ImmutableList.<Artifact>copyOf(inputs),
-        /*tools=*/ImmutableList.<Artifact>of(),
-        /*filesetManifests=*/ImmutableList.<Artifact>of(),
-        action.getOutputs().asList(),
-        action.estimateResourceConsumptionLocal());
+    Spawn spawn =
+        new SimpleSpawn(
+            action,
+            ImmutableList.copyOf(action.getArguments()),
+            ImmutableMap.copyOf(action.getEnvironment()),
+            ImmutableMap.copyOf(action.getExecutionInfo()),
+            EmptyRunfilesSupplier.INSTANCE,
+            ImmutableList.copyOf(inputs),
+            /* tools= */ ImmutableList.of(),
+            /* filesetManifests= */ ImmutableList.of(),
+            action.getOutputs().asList(),
+            action.estimateResourceConsumptionLocal());
 
     List<SpawnResult> spawnResults =
         actionExecutionContext
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 f768eda0f5..b6bd529655 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
@@ -517,7 +517,6 @@ public class CompilationSupport {
             .add(CppRuleClasses.COMPILE_ALL_MODULES)
             .add(CppRuleClasses.EXCLUDE_PRIVATE_HEADERS_IN_MODULE_MAPS)
             .add(CppRuleClasses.ONLY_DOTH_HEADERS_IN_MODULE_MAPS)
-            .add(CppRuleClasses.COMPILE_ACTION_FLAGS_IN_FLAG_SET)
             .add(CppRuleClasses.DEPENDENCY_FILE)
             .add(CppRuleClasses.INCLUDE_PATHS)
             .add(isHost ? "host" : "nonhost")