Rollback of commit 7841a6ab100fc35a67600f1ce1a70d293c51350e.

*** Reason for rollback ***

breaks []

*** Original change description ***

Move the command line arguments for C++ preprocessor defines to a feature.

This required a few assorted changes:

- The LIPO compilation context is not merged anymore. Include paths for auxiliary files are apparently taken from the profile files, so it is not necessary.
- The FDO build stamp is not special-cased anymore, it is treated as a preprocessor define like any other.
- When compiling a .pcm file, use interfaceContext instead of the regular context when setting up the build variables. Th...

***

--
MOS_MIGRATED_REVID=122823591

7 files changed, 49 insertions, 68 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 6ac68d579f..e77bef91b4 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
@@ -87,8 +87,7 @@ public final class CcCommon {
       CppRuleClasses.MODULE_MAP_HOME_CWD,
       CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
       CppRuleClasses.INCLUDE_PATHS,
-      CppRuleClasses.PIC,
-      CppRuleClasses.PREPROCESSOR_DEFINES);
+      CppRuleClasses.PIC);
 
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
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 167f59268f..f3f024d5e3 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
@@ -251,6 +251,7 @@ public class CppCompileAction extends AbstractAction
       Class<? extends CppCompileActionContext> actionContext,
       ImmutableList<String> copts,
       Predicate<String> coptsFilter,
+      @Nullable String fdoBuildStamp,
       SpecialInputsHandler specialInputsHandler,
       Iterable<IncludeScannable> lipoScannables,
       UUID actionClassId,
@@ -291,6 +292,7 @@ public class CppCompileAction extends AbstractAction
             features,
             featureConfiguration,
             variables,
+            fdoBuildStamp,
             actionName);
     this.actionContext = actionContext;
     this.lipoScannables = lipoScannables;
@@ -1265,6 +1267,9 @@ public class CppCompileAction extends AbstractAction
     @VisibleForTesting public final CcToolchainFeatures.Variables variables;
     private final String actionName;
 
+    // The value of the BUILD_FDO_TYPE macro to be defined on command line
+    @Nullable private final String fdoBuildStamp;
+
     public CppCompileCommandLine(
         Artifact sourceFile,
         DotdFile dotdFile,
@@ -1274,6 +1279,7 @@ public class CppCompileAction extends AbstractAction
         Collection<String> features,
         FeatureConfiguration featureConfiguration,
         CcToolchainFeatures.Variables variables,
+        @Nullable String fdoBuildStamp,
         String actionName) {
       this.sourceFile = Preconditions.checkNotNull(sourceFile);
       this.dotdFile = CppFileTypes.mustProduceDotdFile(sourceFile.getPath().toString())
@@ -1284,6 +1290,7 @@ public class CppCompileAction extends AbstractAction
       this.features = Preconditions.checkNotNull(features);
       this.featureConfiguration = featureConfiguration;
       this.variables = variables;
+      this.fdoBuildStamp = fdoBuildStamp;
       this.actionName = actionName;
     }
 
@@ -1333,6 +1340,14 @@ public class CppCompileAction extends AbstractAction
       for (String warn : cppConfiguration.getCWarns()) {
         options.add("-W" + warn);
       }
+      for (String define : context.getDefines()) {
+        options.add("-D" + define);
+      }
+
+      // Stamp FDO builds with FDO subtype string
+      if (fdoBuildStamp != null) {
+        options.add("-D" + CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\"");
+      }
 
       // TODO(bazel-team): This needs to be before adding getUnfilteredCompilerOptions() and after
       // adding the warning flags until all toolchains are migrated; currently toolchains use the
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 97f05b8694..c3f34dadb2 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
@@ -71,7 +71,8 @@ public class CppCompileActionBuilder {
   private final List<Pattern> nocopts = new ArrayList<>();
   private AnalysisEnvironment analysisEnvironment;
   private ImmutableList<PathFragment> extraSystemIncludePrefixes = ImmutableList.of();
-  private boolean usePic;
+  private String fdoBuildStamp;
+  private boolean usePic; 
   private SpecialInputsHandler specialInputsHandler = CppCompileAction.VOID_SPECIAL_INPUTS_HANDLER;
   private UUID actionClassId = GUID;
   private Class<? extends CppCompileActionContext> actionContext;
@@ -141,6 +142,7 @@ public class CppCompileActionBuilder {
     this.actionClassId = other.actionClassId;
     this.actionContext = other.actionContext;
     this.cppConfiguration = other.cppConfiguration;
+    this.fdoBuildStamp = other.fdoBuildStamp;
     this.usePic = other.usePic;
     this.lipoScannableMap = other.lipoScannableMap;
     this.ruleContext = other.ruleContext;
@@ -282,25 +284,12 @@ public class CppCompileActionBuilder {
     
     // Copying the collections is needed to make the builder reusable.
     if (fake) {
-      return new FakeCppCompileAction(
-          owner,
-          ImmutableList.copyOf(features),
-          featureConfiguration,
-          variables,
-          sourceFile,
-          shouldScanIncludes,
-          sourceLabel,
-          realMandatoryInputsBuilder.build(),
-          outputFile,
-          tempOutputFile,
-          dotdFile,
-          configuration,
-          cppConfiguration,
-          context,
-          actionContext,
+      return new FakeCppCompileAction(owner, ImmutableList.copyOf(features), featureConfiguration,
+          variables, sourceFile, shouldScanIncludes, sourceLabel,
+          realMandatoryInputsBuilder.build(), outputFile,
+          tempOutputFile, dotdFile, configuration, cppConfiguration, context, actionContext,
           ImmutableList.copyOf(copts),
-          getNocoptPredicate(nocopts),
-          ruleContext,
+          getNocoptPredicate(nocopts), fdoBuildStamp, ruleContext,
           usePic);
     } else {
       NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
@@ -325,6 +314,7 @@ public class CppCompileActionBuilder {
           actionContext,
           ImmutableList.copyOf(copts),
           getNocoptPredicate(nocopts),
+          fdoBuildStamp,
           specialInputsHandler,
           getLipoScannables(realMandatoryInputs),
           actionClassId,
@@ -466,6 +456,11 @@ public class CppCompileActionBuilder {
     return this;
   }
 
+  public CppCompileActionBuilder setFdoBuildStamp(String fdoBuildStamp) {
+    this.fdoBuildStamp = fdoBuildStamp;
+    return this;
+  }
+
   /**
    * Sets whether the CompileAction should use pic mode.
    */
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 c9085971c9..3424a0e227 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
@@ -709,24 +709,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
             + "}",
             toolchainBuilder);
       }
-      if (!features.contains("preprocessor_defines")) {
-        TextFormat.merge(""
-            + "feature {"
-            + "  name: 'preprocessor_defines'"
-            + "  flag_set {"
-            + "    action: 'preprocess-assemble'"
-            + "    action: 'c-compile'"
-            + "    action: 'c++-compile'"
-            + "    action: 'c++-header-parsing'"
-            + "    action: 'c++-header-preprocessing'"
-            + "    action: 'c++-module-compile'"
-            + "    flag_group {"
-            + "      flag: '-D%{preprocessor_defines}'"
-            + "    }"
-            + "  }"
-            + "}",
-            toolchainBuilder);
-      }
       if (!features.contains("include_paths")) {
         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 a7d62add6b..07497df4ee 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
@@ -308,6 +308,7 @@ public final class CppModel {
       builder.addNocopts(nocopts);
     }
 
+    builder.setFdoBuildStamp(CppHelper.getFdoBuildStamp(ruleContext));
     builder.setFeatureConfiguration(featureConfiguration);
     
     return builder;
@@ -361,8 +362,7 @@ public final class CppModel {
     
     // TODO(bazel-team): Pull out string constants for all build variables.
 
-    CppCompilationContext builderContext = builder.getContext();
-    CppModuleMap cppModuleMap = builderContext.getCppModuleMap();
+    CppModuleMap cppModuleMap = context.getCppModuleMap();
     if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS) && cppModuleMap != null) {
       // If the feature is enabled and cppModuleMap is null, we are about to fail during analysis
       // in any case, but don't crash.
@@ -371,7 +371,7 @@ public final class CppModel {
           cppModuleMap.getArtifact().getExecPathString());
       CcToolchainFeatures.Variables.ValueSequence.Builder sequence =
           new CcToolchainFeatures.Variables.ValueSequence.Builder();
-      for (Artifact artifact : builderContext.getDirectModuleMaps()) {
+      for (Artifact artifact : context.getDirectModuleMaps()) {
         sequence.addValue(artifact.getExecPathString());
       }
       buildVariables.addSequence("dependent_module_map_files", sequence.build());
@@ -381,28 +381,11 @@ public final class CppModel {
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
       buildVariables.addSequenceVariable("include_paths",
-          getSafePathStrings(builderContext.getIncludeDirs()));
+          getSafePathStrings(context.getIncludeDirs()));
       buildVariables.addSequenceVariable("quote_include_paths",
-          getSafePathStrings(builderContext.getQuoteIncludeDirs()));
+          getSafePathStrings(context.getQuoteIncludeDirs()));
       buildVariables.addSequenceVariable("system_include_paths",
-          getSafePathStrings(builderContext.getSystemIncludeDirs()));
-    }
-
-    if (featureConfiguration.isEnabled(CppRuleClasses.PREPROCESSOR_DEFINES)) {
-      String fdoBuildStamp = CppHelper.getFdoBuildStamp(ruleContext);
-      ImmutableList<String> defines;
-      if (fdoBuildStamp != null) {
-        // Stamp FDO builds with FDO subtype string
-        defines = ImmutableList.<String>builder()
-            .addAll(builderContext.getDefines())
-            .add(CppConfiguration.FDO_STAMP_MACRO
-                + "=\"" + CppHelper.getFdoBuildStamp(ruleContext) + "\"")
-            .build();
-      } else {
-        defines = builderContext.getDefines();
-      }
-
-      buildVariables.addSequenceVariable("preprocessor_defines", defines);
+          getSafePathStrings(context.getSystemIncludeDirs()));
     }
 
     if (usePic) {
@@ -500,6 +483,14 @@ public final class CppModel {
       boolean addObject,
       boolean enableCoverage) {
     PathFragment ccRelativeName = semantics.getEffectiveSourcePath(sourceArtifact);
+    if (cppConfiguration.isLipoOptimization()) {
+      // TODO(bazel-team): we shouldn't be needing this, merging context with the binary
+      // is a superset of necessary information.
+      LipoContextProvider lipoProvider =
+          Preconditions.checkNotNull(CppHelper.getLipoContextProvider(ruleContext), outputName);
+      builder.setContext(CppCompilationContext.mergeForLipo(lipoProvider.getLipoContext(),
+          context));
+    }
     boolean generatePicAction = getGeneratePicActions();
     // If we always need pic for everything, then don't bother to create a no-pic action.
     boolean generateNoPicAction = getGenerateNoPicActions();
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 eca4a6d189..074900356c 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
@@ -167,17 +167,12 @@ public class CppRuleClasses {
   /**
    * A string constant for the PIC feature.
    *
-   * <p>If this feature is active (currently it cannot be switched off) and PIC compilation is
+   * If this feature is active (currently it cannot be switched off) and PIC compilation is
    * requested, the "pic" build variable will be defined with an empty string as its value.
    */
   public static final String PIC = "pic";
 
   /**
-   * A string constant for the feature the represents preprocessor defines.
-   */
-  public static final String PREPROCESSOR_DEFINES = "preprocessor_defines";
-
-  /**
    * A string constant for the include_paths feature.
    */
   public static final String INCLUDE_PATHS = "include_paths";
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 644b16b409..a24cc84863 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
@@ -46,6 +46,8 @@ import java.io.IOException;
 import java.util.UUID;
 import java.util.logging.Logger;
 
+import javax.annotation.Nullable;
+
 /**
  * Action that represents a fake C++ compilation step.
  */
@@ -76,6 +78,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       Class<? extends CppCompileActionContext> actionContext,
       ImmutableList<String> copts,
       Predicate<String> nocopts,
+      @Nullable String fdoBuildStamp,
       RuleContext ruleContext,
       boolean usePic) {
     super(
@@ -105,6 +108,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         actionContext,
         copts,
         nocopts,
+        fdoBuildStamp,
         VOID_SPECIAL_INPUTS_HANDLER,
         ImmutableList.<IncludeScannable>of(),
         GUID,