diff options
author | Manuel Klimek <klimek@google.com> | 2015-02-19 12:58:58 +0000 |
---|---|---|
committer | Han-Wen Nienhuys <hanwen@google.com> | 2015-02-19 12:58:58 +0000 |
commit | d74f00132cd5d93a9d34a67dd29e31652e793e44 (patch) | |
tree | ffc724bf305e8e2472093f2566515cc1f5a40964 /src/main | |
parent | fbef62b5ef27ecb783157f993792c6325392b1a0 (diff) |
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
Diffstat (limited to 'src/main')
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"; } |