Skip to content

Commit

Permalink
Move some error reporting from cc_toolchain to rules
Browse files Browse the repository at this point in the history
    This is unfortunate, confusing for the user, and less accurate than what the
    original behavior was, but because cc_toolchain cannot access (currently, with
    platforms enabled) the target configuration, there is nothing we can do other
    than showing the error for the target.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240345500
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent c580bf3 commit bb96c93
Show file tree
Hide file tree
Showing 8 changed files with 731 additions and 148 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,6 @@ public class BazelRulesModule extends BlazeModule {
/** This is where deprecated options go to die. */
public static class GraveyardOptions extends OptionsBase {

@Option(
name = "incompatible_disable_legacy_crosstool_fields",
oldName = "experimental_disable_legacy_crosstool_fields",
defaultValue = "true",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {
OptionMetadataTag.INCOMPATIBLE_CHANGE,
OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
},
help = "Deprecated no-op.")
public boolean disableLegacyCrosstoolFields;

@Option(
name = "incompatible_require_feature_configuration_for_pic",
defaultValue = "true",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ private static Runfiles collectRunfiles(
// inputs for compilation. Node that these cannot be middlemen because Runfiles does not
// know how to expand them.
builder.addTransitiveArtifacts(toolchain.getAllFiles());
builder.addTransitiveArtifacts(toolchain.getLibcLink(cppConfiguration));
builder.addTransitiveArtifacts(toolchain.getLibcLink());
// Add the sources files that are used to compile the object files.
// We add the headers in the transitive closure and our own sources in the srcs
// attribute. We do not provide the auxiliary inputs, because they are only used when we
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,18 @@ public void collectMetadataArtifacts(Iterable<Artifact> objectFiles,
.addAll(ALL_OTHER_ACTIONS)
.build();

/** Features we request to enable unless a rule explicitly doesn't support them. */
private static final ImmutableSet<String> DEFAULT_FEATURES =
ImmutableSet.of(
CppRuleClasses.DEPENDENCY_FILE,
CppRuleClasses.RANDOM_SEED,
CppRuleClasses.MODULE_MAPS,
CppRuleClasses.MODULE_MAP_HOME_CWD,
CppRuleClasses.HEADER_MODULE_COMPILE,
CppRuleClasses.INCLUDE_PATHS,
CppRuleClasses.PIC,
CppRuleClasses.PREPROCESSOR_DEFINES);

public static final String CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME = ":cc_toolchain";
private static final String SYSROOT_FLAG = "--sysroot=";

Expand Down Expand Up @@ -412,6 +424,21 @@ public void reportInvalidOptions(RuleContext ruleContext) {

public static void reportInvalidOptions(
RuleContext ruleContext, CppConfiguration cppConfiguration, CcToolchainProvider ccToolchain) {
if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
&& !ccToolchain.supportsFission()) {
ruleContext.ruleWarning(
"Fission is not supported by this crosstool. Please use a "
+ "supporting crosstool to enable fission");
}
if (cppConfiguration.buildTestDwpIsActivated()
&& !(ccToolchain.supportsFission()
&& cppConfiguration.fissionIsActiveForCurrentCompilationMode())) {
ruleContext.ruleWarning(
"Test dwp file requested, but Fission is not enabled. To generate a "
+ "dwp for the test executable, use '--fission=yes' with a toolchain that supports "
+ "Fission to build statically.");
}

if (cppConfiguration.getLibcTopLabel() != null && ccToolchain.getDefaultSysroot() == null) {
ruleContext.ruleError(
"The selected toolchain "
Expand Down Expand Up @@ -735,6 +762,46 @@ public static ImmutableList<String> getCoverageFeatures(CppConfiguration cppConf
return coverageFeatures.build();
}

/**
* Determines whether to statically link the C++ runtimes.
*
* <p>This is complicated because it depends both on a legacy field in the CROSSTOOL
* protobuf--supports_embedded_runtimes--and the newer crosstool
* feature--statically_link_cpp_runtimes. Neither, one, or both could be present or set. Or they
* could be set in to conflicting values.
*
* @return true if we should statically link, false otherwise.
*/
private static boolean enableStaticLinkCppRuntimesFeature(
ImmutableSet<String> requestedFeatures,
ImmutableSet<String> disabledFeatures,
CcToolchainProvider toolchain) {
// All of these cases are encountered in various unit tests,
// integration tests, and obscure CROSSTOOLs.

// A. If the legacy field "supports_embedded_runtimes" is false (or not present):
// dynamically link the cpp runtimes. Done.
if (!toolchain.supportsEmbeddedRuntimes()) {
return false;
}
// From here, the toolchain _can_ statically link the cpp runtime.
//
// B. If the feature static_link_cpp_runtimes is disabled:
// dynamically link the cpp runtimes. Done.
if (disabledFeatures.contains(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES)) {
return false;
}
// C. If the feature is not requested:
// the feature is neither disabled nor requested: statically
// link (for compatibility with the legacy field).
if (!requestedFeatures.contains(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES)) {
return true;
}
// D. The feature is requested:
// statically link the cpp runtimes. Done.
return true;
}

/**
* Creates a feature configuration for a given rule. Assumes strictly cc sources.
*
Expand Down Expand Up @@ -791,6 +858,20 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
unsupportedFeaturesBuilder.add(CppRuleClasses.MODULE_MAPS);
}

if (toolchain.supportsEmbeddedRuntimes()) {
if (!cppConfiguration.disableLegacyCrosstoolFields()) {
// If we're not using legacy crosstool fields, we assume 'static_link_cpp_runtimes' feature
// is enabled by default for toolchains that support it, and can be disabled by the user
// when needed using --feature=-static_link_cpp_runtimes option or
// features = [ '-static_link_cpp_runtimes' ] rule attribute.
if (enableStaticLinkCppRuntimesFeature(requestedFeatures, unsupportedFeatures, toolchain)) {
allRequestedFeaturesBuilder.add(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES);
} else {
unsupportedFeaturesBuilder.add(CppRuleClasses.STATIC_LINK_CPP_RUNTIMES);
}
}
}

if (cppConfiguration.forcePic()) {
if (unsupportedFeatures.contains(CppRuleClasses.SUPPORTS_PIC)) {
throw new EvalException(/* location= */ null, PIC_CONFIGURATION_ERROR);
Expand Down Expand Up @@ -819,6 +900,10 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
ImmutableList.Builder<String> allFeatures =
new ImmutableList.Builder<String>()
.addAll(ImmutableSet.of(cppConfiguration.getCompilationMode().toString()))
.addAll(
cppConfiguration.disableLegacyCrosstoolFields()
? ImmutableList.of()
: DEFAULT_FEATURES)
.addAll(DEFAULT_ACTION_CONFIGS)
.addAll(requestedFeatures)
.addAll(toolchain.getFeatures().getDefaultFeaturesAndActionConfigs());
Expand All @@ -831,6 +916,12 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
}
}

if (cppConfiguration.fissionIsActiveForCurrentCompilationMode()
&& toolchain.supportsFission()
&& !cppConfiguration.disableLegacyCrosstoolFields()) {
allFeatures.add(CppRuleClasses.PER_OBJECT_DEBUG_INFO);
}

allFeatures.addAll(getCoverageFeatures(cppConfiguration));

String fdoInstrument = cppConfiguration.getFdoInstrument();
Expand Down Expand Up @@ -894,7 +985,7 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
+ "This is most likely a misconfiguration in the C++ toolchain.");
}
}
if ((cppConfiguration.forcePic())
if ((cppConfiguration.forcePic() || toolchain.toolchainNeedsPic())
&& (!featureConfiguration.isEnabled(CppRuleClasses.PIC)
&& !featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_PIC))) {
throw new EvalException(/* location= */ null, PIC_CONFIGURATION_ERROR);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
ccToolchain.getFdoContext())
.addPublicHeaders(common.getHeaders())
.setHeadersCheckingMode(HeadersCheckingMode.STRICT)
.addQuoteIncludeDirs(semantics.getQuoteIncludes(ruleContext))
.setCodeCoverageEnabled(CcCompilationHelper.isCodeCoverageEnabled(ruleContext))
.compile();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,12 +282,12 @@ public static void init(

CompilationInfo compilationInfo = compilationHelper.compile();
CcCompilationOutputs precompiledFilesObjects =
CcCompilationOutputs.builder()
new CcCompilationOutputs.Builder()
.addObjectFiles(precompiledFiles.getObjectFiles(/* usePic= */ true))
.addPicObjectFiles(precompiledFiles.getObjectFiles(/* usePic= */ true))
.build();
CcCompilationOutputs ccCompilationOutputs =
CcCompilationOutputs.builder()
new CcCompilationOutputs.Builder()
.merge(precompiledFilesObjects)
.merge(compilationInfo.getCcCompilationOutputs())
.build();
Expand Down
Loading

0 comments on commit bb96c93

Please sign in to comment.