Port all module map and header parsing related flags to the new crosstool

configuration.

Together with [] (change to the crosstool configuration), the
resulting blaze is able to build header modules (minus clang bugs).

Detailed changes:
1. Adapt CppCompileAction to only insert the arguments itself if the crosstool
   does not specify a feature.
2. Make CppCompileAction provide the build variables to the flag expansion.
3. Pass package features through to the new feature selection / crosstool
   configuration; allow rules to always request features and mark features
   as unsupported.
4. Add feature "header_module_includes_dependencies" that controls whether we
   can only provide top-level header modules in the ${module_files} build
   variable; the currently integrated clang does not fully support that yet.
5. Add feature "use_header_modules", which allows targets to use compiled
   header modules without being compilable as module themselves.
6. Convert tests to use the feature configuration where it makes sense; we will
   be able to delete a lot of unit tests once the control via the feature
   configuration is rolled out to the stable crosstool, and implement them
   as crosstool integration tests.

--
MOS_MIGRATED_REVID=86680884

4 files changed, 164 insertions, 52 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 3f5ff76a0d..cda1765b12 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,6 +87,14 @@ public final class CcCommon {
       }
     }
   };
+    
+  /**
+   * Features we request to enable unless a rule explicitly doesn't support them.
+   */
+  private static final ImmutableSet<String> DEFAULT_FEATURES = ImmutableSet.of(
+      CppRuleClasses.MODULE_MAPS,
+      CppRuleClasses.MODULE_MAP_HOME_CWD,
+      CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES);
 
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
@@ -639,12 +647,29 @@ public final class CcCommon {
             ruleContext, CppRuleClasses.INSTRUMENTATION_SPEC, CC_METADATA_COLLECTOR, files));
   }
 
-  public static FeatureConfiguration configureFeatures(RuleContext ruleContext) {
+  /**
+   * Creates the feature configuration for a given rule.
+   * 
+   * @param ruleContext the context of the rule we want the feature configuration for. 
+   * @param ruleSpecificRequestedFeatures features that will be requested, and thus be always
+   * enabled if the toolchain supports them.
+   * @param ruleSpecificUnsupportedFeatures features that are not supported in the current context.
+   * @return the feature configuration for the given {@code ruleContext}.
+   */
+  public static FeatureConfiguration configureFeatures(RuleContext ruleContext,
+      Set<String> ruleSpecificRequestedFeatures,
+      Set<String> ruleSpecificUnsupportedFeatures) {
     CcToolchainProvider toolchain = CppHelper.getToolchain(ruleContext);    
-    Set<String> requestedFeatures = ImmutableSet.of(CppRuleClasses.MODULE_MAP_HOME_CWD);
-    return toolchain.getFeatures().getFeatureConfiguration(requestedFeatures);
+    ImmutableSet.Builder<String> requestedFeatures = ImmutableSet.builder();
+    for (String feature : Iterables.concat(DEFAULT_FEATURES, ruleContext.getFeatures())) {
+      if (!ruleSpecificUnsupportedFeatures.contains(feature)) {
+        requestedFeatures.add(feature);
+      }
+    }
+    requestedFeatures.addAll(ruleSpecificRequestedFeatures);
+    return toolchain.getFeatures().getFeatureConfiguration(requestedFeatures.build());
   }
-
+  
   public void addTransitiveInfoProviders(RuleConfiguredTargetBuilder builder,
       NestedSet<Artifact> filesToBuild,
       CcCompilationOutputs ccCompilationOutputs,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index 3812bf5001..0797e3a509 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -49,6 +49,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -171,6 +172,11 @@ public final class CcLibraryHelper {
   private final List<LibraryToLink> picStaticLibraries = new ArrayList<>();
   private final List<LibraryToLink> dynamicLibraries = new ArrayList<>();
 
+  private final Set<String> requestedFeatures = new HashSet<>();
+  private final Set<String> unsupportedFeatures = new HashSet<>();
+  
+  // TODO(bazel-team): Remove flags that affect toolchain features after migrating their uses to
+  // requestedFeatures / unsupportedFeatures. 
   private boolean emitCppModuleMaps = true;
   private boolean enableLayeringCheck;
   private boolean compileHeaderModules;
@@ -499,6 +505,8 @@ public final class CcLibraryHelper {
   /**
    * This disables C++ module map generation for the current rule. Don't call this unless you know
    * what you are doing.
+   * 
+   * <p>TODO(bazel-team): Replace with {@code addUnsupportedFeature()}.
    */
   public CcLibraryHelper disableCppModuleMapGeneration() {
     this.emitCppModuleMaps = false;
@@ -507,6 +515,8 @@ public final class CcLibraryHelper {
 
   /**
    * This enables or disables use of module maps during compilation, i.e., layering checks.
+   * 
+   * <p>TODO(bazel-team): Replace with {@code addRequestedFeature()}.
    */
   public CcLibraryHelper setEnableLayeringCheck(boolean enableLayeringCheck) {
     this.enableLayeringCheck = enableLayeringCheck;
@@ -518,11 +528,32 @@ public final class CcLibraryHelper {
    * TODO(bazel-team): Add a cc_toolchain flag that allows fully disabling this feature and document
    * this feature.
    * See http://clang.llvm.org/docs/Modules.html.
+   * 
+   * <p>TODO(bazel-team): Replace with {@code addRequestedFeature()}.
    */
   public CcLibraryHelper setCompileHeaderModules(boolean compileHeaderModules) {
     this.compileHeaderModules = compileHeaderModules;
     return this;
   }
+  
+  /**
+   * Adds a feature to be requested from the toolchain.  
+   */
+  public CcLibraryHelper addRequestedFeature(String feature) {
+    Preconditions.checkNotNull(feature);
+    this.requestedFeatures.add(feature);
+    return this;
+  }
+  
+  /**
+   * Adds a feature that is unsupported in the current context, and must not be requested from the
+   * toolchain.
+   */
+  public CcLibraryHelper addUnsupportedFeature(String feature) {
+    Preconditions.checkNotNull(feature);
+    this.unsupportedFeatures.add(feature);
+    return this;
+  }
 
   /**
    * Enables or disables generation of compile actions if there are no sources. Some rules declare a
@@ -603,7 +634,8 @@ public final class CcLibraryHelper {
 
     CcLinkingOutputs ccLinkingOutputs = CcLinkingOutputs.EMPTY;
     CcCompilationOutputs ccOutputs = new CcCompilationOutputs.Builder().build();
-    FeatureConfiguration featureConfiguration = CcCommon.configureFeatures(ruleContext);
+    FeatureConfiguration featureConfiguration =
+        CcCommon.configureFeatures(ruleContext, requestedFeatures, unsupportedFeatures);
     
     CppModel model = new CppModel(ruleContext, semantics)
         .addSources(sources)
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 4d145826f9..a81416c434 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
@@ -68,6 +68,7 @@ import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -1160,21 +1161,29 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
       // TODO(bazel-team): Extract combinations of options into sections in the CROSSTOOL file.
       if (CppFileTypes.CPP_MODULE_MAP.matches(sourceFile.getExecPath())) {
-        options.add("-x");
-        options.add("c++");
+        if (!featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES)) {
+          options.add("-x");
+          options.add("c++");
+          options.add("-Xclang=-emit-module");
+          options.add("-Xcrosstool-module-compilation");
+        }
       } else if (CppFileTypes.CPP_HEADER.matches(sourceFile.getExecPath())) {
         // TODO(bazel-team): Read the compiler flag settings out of the CROSSTOOL file.
         // TODO(bazel-team): Handle C headers that probably don't work in C++ mode.
         if (features.contains(CppRuleClasses.PARSE_HEADERS)) {
-          options.add("-x");
-          options.add("c++-header");
-          // Specifying -x c++-header will make clang/gcc create precompiled
-          // headers, which we suppress with -fsyntax-only.
-          options.add("-fsyntax-only");
+          if (!featureConfiguration.isEnabled(CppRuleClasses.PARSE_HEADERS)) {
+            options.add("-x");
+            options.add("c++-header");
+            // Specifying -x c++-header will make clang/gcc create precompiled
+            // headers, which we suppress with -fsyntax-only.
+            options.add("-fsyntax-only");
+          }
         } else if (features.contains(CppRuleClasses.PREPROCESS_HEADERS)) {
-          options.add("-E");
-          options.add("-x");
-          options.add("c++");
+          if (!featureConfiguration.isEnabled(CppRuleClasses.PREPROCESS_HEADERS)) {
+            options.add("-E");
+            options.add("-x");
+            options.add("c++");
+          }
         } else {
           // CcCommon.collectCAndCppSources() ensures we do not add headers to
           // the compilation artifacts unless either 'parse_headers' or
@@ -1269,37 +1278,25 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       }
 
       if (cppModuleMap != null && (compileHeaderModules || enableLayeringCheck)) {
-        options.add("-Xclang-only=-fmodule-maps");
-        options.add("-Xclang-only=-fmodule-name=" + cppModuleMap.getName());
-        options.add("-Xclang-only=-fmodule-map-file="
-            + cppModuleMap.getArtifact().getExecPathString());
-        options.add("-Xclang=-fno-modules-implicit-maps");
+        if (!featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS)) {
+          // TODO(bazel-team): Remove this path once all toolchains that support module map have
+          // their configuration updated; in that case, we do not need to add the flags here, as
+          // they will be added through the corresponding crosstool feature configuration.
+          options.add("-Xclang-only=-fmodule-maps");
+          options.add("-Xclang-only=-fmodule-name=" + cppModuleMap.getName());
+          options.add("-Xclang-only=-fmodule-map-file="
+              + cppModuleMap.getArtifact().getExecPathString());
+          options.add("-Xclang=-fno-modules-implicit-maps");
+        }
               
-        if (compileHeaderModules) {
+        if (compileHeaderModules
+            && !featureConfiguration.isEnabled(CppRuleClasses.HEADER_MODULES)) {
           options.add("-Xclang-only=-fmodules");        
-          if (CppFileTypes.CPP_MODULE_MAP.matches(sourceFilename)) {
-            options.add("-Xclang=-emit-module");
-            options.add("-Xcrosstool-module-compilation");
-          }
-          // Select .pcm inputs to pass on the command line depending on whether
-          // we are in pic or non-pic mode.
-          // TODO(bazel-team): We want to add these to the compile even if the
-          // current target is not built as a module; currently that implies
-          // passing -fmodules to the compiler, which is experimental; thus, we
-          // do not use the header modules files for now if the current
-          // compilation is not modules enabled on its own.
-          boolean pic = copts.contains("-fPIC");
-          for (Artifact source : context.getTopLevelHeaderModules()) {
-            // Depending on whether this specific compile action is pic or non-pic, select the
-            // corresponding header modules. Note that the compilation context might give us both
-            // from targets that are built in both modes.
-            if ((pic && source.getFilename().endsWith(".pic.pcm"))
-                || (!pic && !source.getFilename().endsWith(".pic.pcm"))) {
-              options.add("-Xclang=-fmodule-file=" + source.getExecPathString());
-            }
+          for (String headerModulePath : getHeaderModulePaths()) {
+            options.add("-Xclang=-fmodule-file=" + headerModulePath);            
           }
         }
-        if (enableLayeringCheck) {
+        if (enableLayeringCheck && !featureConfiguration.isEnabled(CppRuleClasses.LAYERING_CHECK)) {
           options.add("-Xclang-only=-fmodules-strict-decluse");          
         }
       }
@@ -1315,12 +1312,49 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
       if (cppConfiguration.useFission()) {
         options.add("-gsplit-dwarf");
       }
-      
-      options.addAll(featureConfiguration.getCommandLine(getActionName(),
-          new CcToolchainFeatures.Variables.Builder().build()));
+
+      CcToolchainFeatures.Variables.Builder buildVariables =
+          new CcToolchainFeatures.Variables.Builder();
+      if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS)) {
+        buildVariables.addVariable("module_name", cppModuleMap.getName());
+        buildVariables.addVariable("module_map_file",
+            cppModuleMap.getArtifact().getExecPathString());
+      }
+      if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
+        buildVariables.addSequenceVariable("module_files", getHeaderModulePaths());
+      }
+      options.addAll(featureConfiguration.getCommandLine(getActionName(), buildVariables.build()));
       return options;
     }
 
+    /**
+     * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-pic
+     * mode.
+     */
+    private Collection<String> getHeaderModulePaths() {
+      Collection<String> result = new LinkedHashSet<>();
+      // TODO(bazel-team): Do not hard-code checking for -fPIC. Make it a toolchain feature
+      // instead.
+      boolean pic = copts.contains("-fPIC");
+      NestedSet<Artifact> artifacts = featureConfiguration.isEnabled(
+          CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES)
+          ? context.getTopLevelHeaderModules()
+          : context.getAdditionalInputs();
+      for (Artifact artifact : artifacts) {
+        String filename = artifact.getFilename();
+        if (!filename.endsWith(".pcm")) {
+          continue;
+        }
+        // Depending on whether this specific compile action is pic or non-pic, select the
+        // corresponding header modules. Note that the compilation context might give us both
+        // from targets that are built in both modes.
+        if (pic == filename.endsWith(".pic.pcm")) {
+          result.add(artifact.getExecPathString());
+        }          
+      }
+      return result;
+    }
+
     // For each option in 'in', add it to 'out' unless it is matched by the 'coptsFilter' regexp.
     private void addFilteredOptions(List<String> out, List<String> in) {
       Iterables.addAll(out, Iterables.filter(in, coptsFilter));
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 de7c95d4a8..bde769a3b3 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
@@ -77,11 +77,6 @@ public class CppRuleClasses {
   public static final String BUILD_INTERFACE_SO = "build_interface_so";
 
   /**
-   * A string constant for the layering_check feature.
-   */
-  public static final String LAYERING_CHECK = "layering_check";
-  
-  /**
    * A string constant for the parse_headers feature.
    */
   public static final String PARSE_HEADERS = "parse_headers";
@@ -92,13 +87,39 @@ public class CppRuleClasses {
   public static final String PREPROCESS_HEADERS = "preprocess_headers";
 
   /**
-   * A string constant for the header_modules feature.
+   * A string constant for the module_maps feature; this is a precondition to the layering_check and
+   * header_modules features.
    */
-  public static final String HEADER_MODULES = "header_modules";
+  public static final String MODULE_MAPS = "module_maps";
   
   /**
    * A string constant for the module_map_home_cwd feature.
    */
   public static final String MODULE_MAP_HOME_CWD = "module_map_home_cwd";
   
+  /**
+   * A string constant for the layering_check feature.
+   */
+  public static final String LAYERING_CHECK = "layering_check";
+  
+  /**
+   * A string constant for the header_modules feature.
+   */
+  public static final String HEADER_MODULES = "header_modules";
+  
+  /**
+   * A string constant for the use_header_modules feature.
+   * 
+   * <p>This feature is only used during rollout; we expect to default enable this once we
+   * have verified that module-enabled compilation is stable enough.
+   */
+  public static final String USE_HEADER_MODULES = "use_header_modules";
+
+  /**
+   * A string constant for switching on that a header module file includes information about
+   * all its dependencies, so we do not need to pass all transitive dependent header modules on
+   * the command line.
+   */
+  public static final String HEADER_MODULE_INCLUDES_DEPENDENCIES =
+      "header_module_includes_dependencies";
 }