Switch to a CROSSTOOL feature for include flags.

This allows supporting compilers which don't understand -iquote. It
looks like support for -iquote was released in GCC 4.0
(https://gcc.gnu.org/gcc-4.0/changes.html). GCC 3.3.2 definitely does
not have support.

Here's what the error looks like when you try to use an older GCC
without this change:
cc1plus: error: unrecognized option `-iquote.'
cc1plus: error: unrecognized option `
   -iquotebazel-out/powerpc-603e-fastbuild/genfiles'

--
Change-Id: Ibf3b74d058a741e23da51963b39ad7505c507d57
Reviewed-on: https://bazel-review.googlesource.com/#/c/1263/
MOS_MIGRATED_REVID=93888319

5 files changed, 73 insertions, 30 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 d847353cbb..a26585e614 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
@@ -92,7 +92,8 @@ public final class CcCommon {
   private static final ImmutableSet<String> DEFAULT_FEATURES = ImmutableSet.of(
       CppRuleClasses.MODULE_MAPS,
       CppRuleClasses.MODULE_MAP_HOME_CWD,
-      CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES);
+      CppRuleClasses.HEADER_MODULE_INCLUDES_DEPENDENCIES,
+      CppRuleClasses.INCLUDE_PATHS);
 
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
index 25d01b989b..1e5837ecc7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeatures.java
@@ -604,7 +604,8 @@ public class CcToolchainFeatures implements Serializable {
    * @param toolchain the toolchain configuration as specified by the user.
    * @throws InvalidConfigurationException if the configuration has logical errors.
    */
-  CcToolchainFeatures(CToolchain toolchain) throws InvalidConfigurationException {
+  @VisibleForTesting
+  public CcToolchainFeatures(CToolchain toolchain) throws InvalidConfigurationException {
     // Build up the feature graph.
     // First, we build up the map of name -> features in one pass, so that earlier features can
     // reference later features in their configuration.
@@ -698,7 +699,7 @@ public class CcToolchainFeatures implements Serializable {
   /**
    * Convenience method taking a variadic string argument list for testing.   
    */
-  FeatureConfiguration getFeatureConfiguration(String... requestedFeatures) {
+  public FeatureConfiguration getFeatureConfiguration(String... requestedFeatures) {
     return getFeatureConfiguration(Arrays.asList(requestedFeatures));
   }
 
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 20f0addf94..2ceecab5ef 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
@@ -1217,23 +1217,8 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
 
     public List<String> getCompilerOptions() {
       List<String> options = new ArrayList<>();
-
-      for (PathFragment quoteIncludePath : context.getQuoteIncludeDirs()) {
-        // "-iquote" is a gcc-specific option.  For C compilers that don't support "-iquote",
-        // we should instead use "-I".
-        options.add("-iquote");
-        options.add(quoteIncludePath.getSafePathString());
-      }
-      for (PathFragment includePath : context.getIncludeDirs()) {
-        options.add("-I" + includePath.getSafePathString());
-      }
-      for (PathFragment systemIncludePath : context.getSystemIncludeDirs()) {
-        options.add("-isystem");
-        options.add(systemIncludePath.getSafePathString());
-      }
-
       CppConfiguration toolchain = cppConfiguration;
-
+      
       // pluginOpts has to be added before defaultCopts because -fplugin must precede -plugin-arg.
       options.addAll(pluginOpts);
       addFilteredOptions(options, toolchain.getCompilerOptions(features));
@@ -1268,6 +1253,31 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         options.add("-D" + CppConfiguration.FDO_STAMP_MACRO + "=\"" + fdoBuildStamp + "\"");
       }
 
+      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());
+      }      
+      if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
+        buildVariables.addSequenceVariable("include_paths",
+            getSafePathStrings(context.getIncludeDirs()));
+        buildVariables.addSequenceVariable("quote_include_paths",
+            getSafePathStrings(context.getQuoteIncludeDirs()));
+        buildVariables.addSequenceVariable("system_include_paths",
+            getSafePathStrings(context.getSystemIncludeDirs()));
+      }
+      // 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
+      // unfiltered compiler options to inject include paths, which is superseded by the feature
+      // configuration; on the other hand toolchains switch off warnings for the layering check
+      // that will be re-added by the feature flags.
+      options.addAll(featureConfiguration.getCommandLine(getActionName(), buildVariables.build()));
+
       options.addAll(toolchain.getUnfilteredCompilerOptions(features));
 
       // GCC gives randomized names to symbols which are defined in
@@ -1318,21 +1328,21 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
         options.add("-fPIC");
       }
 
-      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;
     }
 
     /**
+     * Get the safe path strings for a list of paths to use in the build variables.
+     */
+    private Collection<String> getSafePathStrings(Collection<PathFragment> paths) {
+      ImmutableSet.Builder<String> result = ImmutableSet.builder();
+      for (PathFragment path : paths) {
+        result.add(path.getSafePathString());
+      }
+      return result.build();
+    }
+
+    /**
      * Select .pcm inputs to pass on the command line depending on whether we are in pic or non-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 3f74ecfbd8..e0860644f8 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
@@ -800,6 +800,32 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
             + "}",
             toolchainBuilder);
       }
+      if (!features.contains("include_paths")) {
+        TextFormat.merge(""
+            + "feature {"
+            + "  name: 'include_paths'"
+            + "  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: '-iquote'"
+            + "      flag: '%{quote_include_paths}'"
+            + "    }"
+            + "    flag_group {"
+            + "      flag: '-I%{include_paths}'"
+            + "    }"
+            + "    flag_group {"
+            + "      flag: '-isystem'"
+            + "      flag: '%{system_include_paths}'"
+            + "    }"
+            + "  }"
+            + "}",
+            toolchainBuilder);
+      }
     } catch (ParseException e) {
       // Can only happen if we change the proto definition without changing our configuration above.
       throw new RuntimeException(e);
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 9fa675d3dc..c6b3a216d6 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
@@ -140,4 +140,9 @@ public class CppRuleClasses {
    * default legacy feature set.
    */
   public static final String NO_LEGACY_FEATURES = "no_legacy_features";
+
+  /**
+   * A string constant for the include_paths feature.
+   */
+  public static final String INCLUDE_PATHS = "include_paths";
 }