Automated rollback of commit f26e8694ae78599b3e2004e3360eaf3443fa53a6.

*** Reason for rollback ***

Breaks clang_tidy.

*** Original change description ***

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.

RELNOTES: None.
PiperOrigin-RevId: 168834576

9 files changed, 82 insertions, 107 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 1ac7da832d..e8d5a93d1c 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,7 +14,6 @@
 
 package com.google.devtools.build.lib.actions;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
 import java.util.List;
 
@@ -34,6 +33,5 @@ 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 da4ec05b07..0a253d34c5 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
@@ -90,6 +90,7 @@ 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 73bf3e85bb..3215cbb36e 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
@@ -20,8 +20,10 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
+import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -31,6 +33,7 @@ import javax.annotation.Nullable;
 public final class CompileCommandLine {
 
   private final Artifact sourceFile;
+  private final Artifact outputFile;
   private final Predicate<String> coptsFilter;
   private final FeatureConfiguration featureConfiguration;
   private final CcToolchainFeatures.Variables variables;
@@ -40,6 +43,7 @@ public final class CompileCommandLine {
 
   private CompileCommandLine(
       Artifact sourceFile,
+      Artifact outputFile,
       Predicate<String> coptsFilter,
       FeatureConfiguration featureConfiguration,
       CppConfiguration cppConfiguration,
@@ -47,6 +51,7 @@ 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.cppConfiguration = Preconditions.checkNotNull(cppConfiguration);
@@ -66,12 +71,8 @@ public final class CompileCommandLine {
     return featureConfiguration.getEnvironmentVariables(actionName, variables);
   }
 
-  /**
-   * @param overwrittenVariables: Variables that will overwrite original build variables. When null,
-   *     unmodified original variables are used.
-   */
-  protected List<String> getArguments(
-      @Nullable CcToolchainFeatures.Variables overwrittenVariables) {
+  protected List<String> getArgv(
+      PathFragment outputFile, CcToolchainFeatures.Variables overwrittenVariables) {
     List<String> commandLine = new ArrayList<>();
 
     // first: The command name.
@@ -86,6 +87,17 @@ public final class CompileCommandLine {
 
     // second: The compiler options.
     commandLine.addAll(getCompilerOptions(overwrittenVariables));
+
+    if (!featureConfiguration.isEnabled("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;
   }
 
@@ -104,6 +116,19 @@ 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;
   }
 
@@ -149,16 +174,19 @@ public final class CompileCommandLine {
 
   public static Builder builder(
       Artifact sourceFile,
+      Artifact outputFile,
       Predicate<String> coptsFilter,
       String actionName,
       CppConfiguration cppConfiguration,
       DotdFile dotdFile) {
-    return new Builder(sourceFile, coptsFilter, actionName, cppConfiguration, dotdFile);
+    return new Builder(
+        sourceFile, outputFile, coptsFilter, actionName, cppConfiguration, dotdFile);
   }
 
   /** A builder for a {@link CompileCommandLine}. */
   public static final class Builder {
     private final Artifact sourceFile;
+    private final Artifact outputFile;
     private Predicate<String> coptsFilter;
     private FeatureConfiguration featureConfiguration;
     private CcToolchainFeatures.Variables variables = Variables.EMPTY;
@@ -169,6 +197,7 @@ public final class CompileCommandLine {
     public CompileCommandLine build() {
       return new CompileCommandLine(
           Preconditions.checkNotNull(sourceFile),
+          Preconditions.checkNotNull(outputFile),
           Preconditions.checkNotNull(coptsFilter),
           Preconditions.checkNotNull(featureConfiguration),
           Preconditions.checkNotNull(cppConfiguration),
@@ -179,11 +208,13 @@ public final class CompileCommandLine {
 
     private Builder(
         Artifact sourceFile,
+        Artifact outputFile,
         Predicate<String> coptsFilter,
         String actionName,
         CppConfiguration cppConfiguration,
         DotdFile dotdFile) {
       this.sourceFile = sourceFile;
+      this.outputFile = outputFile;
       this.coptsFilter = coptsFilter;
       this.actionName = actionName;
       this.cppConfiguration = cppConfiguration;
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 29a00d0142..e965d4a82f 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,8 +70,6 @@ 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'",
@@ -83,8 +81,6 @@ 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'",
@@ -96,8 +92,6 @@ 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'",
@@ -109,8 +103,6 @@ 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'",
@@ -122,8 +114,6 @@ 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'",
@@ -135,8 +125,6 @@ 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'",
@@ -148,8 +136,6 @@ 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'",
@@ -161,8 +147,6 @@ 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),
@@ -1043,66 +1027,6 @@ 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: '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: '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: '-o'",
-                    "      flag: '%{output_assembly_file}'",
-                    "      flag: '-S'",
-                    "    }",
-                    "    flag_group {",
-                    "      expand_if_all_available: 'output_preprocess_file'",
-                    "      flag: '-o'",
-                    "      flag: '%{output_preprocess_file}'",
-                    "      flag: '-E'",
-                    "    }",
-                    "  }",
                     "}")));
   }
 
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 735e24ecb8..0de697be74 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
@@ -341,6 +341,7 @@ public class CppCompileAction extends AbstractAction
     this.compileCommandLine =
         CompileCommandLine.builder(
                 sourceFile,
+                outputFile,
                 coptsFilter,
                 actionName,
                 cppConfiguration,
@@ -662,7 +663,7 @@ public class CppCompileAction extends AbstractAction
   @Override
   public List<String> getCmdlineIncludes() {
     ImmutableList.Builder<String> cmdlineIncludes = ImmutableList.builder();
-    List<String> args = getArguments();
+    List<String> args = getArgv();
     for (Iterator<String> argi = args.iterator(); argi.hasNext();) {
       String arg = argi.next();
       if (arg.equals("-include") && argi.hasNext()) {
@@ -724,9 +725,21 @@ 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());
+  }
+
   @Override
   public List<String> getArguments() {
-    return compileCommandLine.getArguments(overwrittenVariables);
+    return getArgv();
+  }
+
+  protected final List<String> getArgv(PathFragment outputFile) {
+    return compileCommandLine.getArgv(outputFile, overwrittenVariables);
   }
 
   @Override
@@ -1087,8 +1100,7 @@ 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
@@ -1105,10 +1117,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.getArguments(/* overwrittenVariables= */ null));
+    f.addStrings(compileCommandLine.getArgv(getInternalOutputFile(), null));
 
     /*
-     * getArguments() above captures all changes which affect the compilation
+     * getArgv() 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
@@ -1336,9 +1348,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 getArguments() is actually the command to execute.
+    // The first element in getArgv() is actually the command to execute.
     String legend = "  Command: ";
-    for (String argument : ShellEscaper.escapeAll(getArguments())) {
+    for (String argument : ShellEscaper.escapeAll(getArgv())) {
       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 4e8139e096..5239a2f5eb 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
@@ -190,6 +190,13 @@ 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 24afe83de2..1daec4300b 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
@@ -199,12 +199,14 @@ public class FakeCppCompileAction extends CppCompileAction {
     // line to write to $TEST_TMPDIR instead.
     final String outputPrefix = "$TEST_TMPDIR/";
     String argv =
-        getArguments()
+        getArgv(outputFile.getExecPath())
             .stream()
             .map(
                 input -> {
                   String result = ShellEscaper.escapeString(input);
-                  // Replace -c <tempOutputFile> so it's -c <outputFile>.
+                  // 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.
                   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 cf8612a89e..f24be92101 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
@@ -53,18 +53,17 @@ public class SpawnGccStrategy implements CppCompileActionContext {
           + action.getPrimaryInput().getExecPathString());
     }
     Iterable<Artifact> inputs = Iterables.concat(action.getInputs(), action.getAdditionalInputs());
-    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());
+    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());
 
     actionExecutionContext
         .getSpawnActionContext(action.getMnemonic())
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java
index 69d071eeee..8241b68cfb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CrosstoolCompilationSupport.java
@@ -502,6 +502,7 @@ public class CrosstoolCompilationSupport extends 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")