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
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 3f5ff76..cda1765 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 @@
}
}
};
+
+ /**
+ * 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 @@
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 3812bf5..0797e3a 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.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 @@
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 @@
/**
* 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 @@
/**
* 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 @@
* 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 @@
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 4d14582..a81416c 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.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 @@
// 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 @@
}
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 @@
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 de7c95d..bde769a 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 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 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";
}
