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

This required a few assorted changes:

- 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. This is a bit more consistent and would be a good cause for a future bug.

This is a retry of commit 7841a6ab100fc35a67600f1ce1a70d293c51350e, which made some bold changes to LIPO that didn't work out well.

--
MOS_MIGRATED_REVID=122829825

7 files changed, 69 insertions, 41 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 e77bef91b4..6ac68d579f 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,7 +87,8 @@ public final class CcCommon {
       CppRuleClasses.MODULE_MAP_HOME_CWD,
       CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
       CppRuleClasses.INCLUDE_PATHS,
-      CppRuleClasses.PIC);
+      CppRuleClasses.PIC,
+      CppRuleClasses.PREPROCESSOR_DEFINES);
 
   /** 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 f3f024d5e3..167f59268f 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,7 +251,6 @@ 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,
@@ -292,7 +291,6 @@ public class CppCompileAction extends AbstractAction
             features,
             featureConfiguration,
             variables,
-            fdoBuildStamp,
             actionName);
     this.actionContext = actionContext;
     this.lipoScannables = lipoScannables;
@@ -1267,9 +1265,6 @@ 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,
@@ -1279,7 +1274,6 @@ 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())
@@ -1290,7 +1284,6 @@ public class CppCompileAction extends AbstractAction
       this.features = Preconditions.checkNotNull(features);
       this.featureConfiguration = featureConfiguration;
       this.variables = variables;
-      this.fdoBuildStamp = fdoBuildStamp;
       this.actionName = actionName;
     }
 
@@ -1340,14 +1333,6 @@ 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 c3f34dadb2..97f05b8694 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,8 +71,7 @@ public class CppCompileActionBuilder {
   private final List<Pattern> nocopts = new ArrayList<>();
   private AnalysisEnvironment analysisEnvironment;
   private ImmutableList<PathFragment> extraSystemIncludePrefixes = ImmutableList.of();
-  private String fdoBuildStamp;
-  private boolean usePic; 
+  private boolean usePic;
   private SpecialInputsHandler specialInputsHandler = CppCompileAction.VOID_SPECIAL_INPUTS_HANDLER;
   private UUID actionClassId = GUID;
   private Class<? extends CppCompileActionContext> actionContext;
@@ -142,7 +141,6 @@ 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;
@@ -284,12 +282,25 @@ 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), fdoBuildStamp, ruleContext,
+          getNocoptPredicate(nocopts),
+          ruleContext,
           usePic);
     } else {
       NestedSet<Artifact> realMandatoryInputs = realMandatoryInputsBuilder.build();
@@ -314,7 +325,6 @@ public class CppCompileActionBuilder {
           actionContext,
           ImmutableList.copyOf(copts),
           getNocoptPredicate(nocopts),
-          fdoBuildStamp,
           specialInputsHandler,
           getLipoScannables(realMandatoryInputs),
           actionClassId,
@@ -456,11 +466,6 @@ 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 3424a0e227..c9085971c9 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,6 +709,24 @@ 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 07497df4ee..c4b490e8e5 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,7 +308,6 @@ public final class CppModel {
       builder.addNocopts(nocopts);
     }
 
-    builder.setFdoBuildStamp(CppHelper.getFdoBuildStamp(ruleContext));
     builder.setFeatureConfiguration(featureConfiguration);
     
     return builder;
@@ -362,7 +361,8 @@ public final class CppModel {
     
     // TODO(bazel-team): Pull out string constants for all build variables.
 
-    CppModuleMap cppModuleMap = context.getCppModuleMap();
+    CppCompilationContext builderContext = builder.getContext();
+    CppModuleMap cppModuleMap = builderContext.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 : context.getDirectModuleMaps()) {
+      for (Artifact artifact : builderContext.getDirectModuleMaps()) {
         sequence.addValue(artifact.getExecPathString());
       }
       buildVariables.addSequence("dependent_module_map_files", sequence.build());
@@ -381,11 +381,28 @@ public final class CppModel {
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
       buildVariables.addSequenceVariable("include_paths",
-          getSafePathStrings(context.getIncludeDirs()));
+          getSafePathStrings(builderContext.getIncludeDirs()));
       buildVariables.addSequenceVariable("quote_include_paths",
-          getSafePathStrings(context.getQuoteIncludeDirs()));
+          getSafePathStrings(builderContext.getQuoteIncludeDirs()));
       buildVariables.addSequenceVariable("system_include_paths",
-          getSafePathStrings(context.getSystemIncludeDirs()));
+          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);
     }
 
     if (usePic) {
@@ -491,6 +508,7 @@ public final class CppModel {
       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 074900356c..eca4a6d189 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,12 +167,17 @@ public class CppRuleClasses {
   /**
    * A string constant for the PIC feature.
    *
-   * If this feature is active (currently it cannot be switched off) and PIC compilation is
+   * <p>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 a24cc84863..644b16b409 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,8 +46,6 @@ 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.
  */
@@ -78,7 +76,6 @@ public class FakeCppCompileAction extends CppCompileAction {
       Class<? extends CppCompileActionContext> actionContext,
       ImmutableList<String> copts,
       Predicate<String> nocopts,
-      @Nullable String fdoBuildStamp,
       RuleContext ruleContext,
       boolean usePic) {
     super(
@@ -108,7 +105,6 @@ public class FakeCppCompileAction extends CppCompileAction {
         actionContext,
         copts,
         nocopts,
-        fdoBuildStamp,
         VOID_SPECIAL_INPUTS_HANDLER,
         ImmutableList.<IncludeScannable>of(),
         GUID,