Skip to content

Commit

Permalink
Do not crash Bazel when CcToolchainConfigInfo variables misbehave.
Browse files Browse the repository at this point in the history 
    In order to do that we have to thread EvalExceptions or RuleErrorExceptions to various analysis corners, and we have to thread ActionExecutionException to various execution corners.

    RELNOTES: None.
    PiperOrigin-RevId: 241526530
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 9a2e7b3 commit 211dc1c
Show file tree
Hide file tree
Showing 27 changed files with 430 additions and 721 deletions.
7 changes: 1 addition & 6 deletions ...a/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/actions/CommandAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,7 @@
*/
public interface CommandAction extends Action, ExecutionInfoSpecifier {

/**
* Returns a list of command line arguments that implements this action.
*
* <p>In most cases, this expands any params files. One notable exception is C/C++ compilation
* with the "compiler_param_file" feature.
*/
/** Returns a list of command line arguments that implements this action. */
List<String> getArguments() throws CommandLineExpansionException;

/**
Expand Down
16 changes: 10 additions & 6 deletions ...rc/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@
import com.google.devtools.build.lib.analysis.MakeVariableSupplier.MapBackedMakeVariableSupplier;
import com.google.devtools.build.lib.analysis.MakeVariableSupplier.TemplateVariableInfoBackedMakeVariableSupplier;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import net.starlark.java.eval.Dict;

/**
* Implements make variable expansion for make variables that depend on the configuration and the
Expand All @@ -48,7 +51,8 @@ private static ImmutableList<TemplateVariableInfo> getRuleTemplateVariableProvid
.flatMap(
attrName ->
Streams.stream(
ruleContext.getPrerequisites(attrName, TemplateVariableInfo.PROVIDER)))
ruleContext.getPrerequisites(
attrName, Mode.DONT_CHECK, TemplateVariableInfo.PROVIDER)))
.collect(Collectors.toList());
providers.addAll(fromAttributes);

Expand All @@ -62,7 +66,7 @@ private static ImmutableList<TemplateVariableInfo> getRuleTemplateVariableProvid

private final ImmutableList<? extends MakeVariableSupplier> allMakeVariableSuppliers;

// TODO(b/37567440): Remove when Starlark callers can be updated to get this from
// TODO(b/37567440): Remove when Skylark callers can be updated to get this from
// CcToolchainProvider. We should use CcCommon.CC_TOOLCHAIN_ATTRIBUTE_NAME, but we didn't want to
// pollute core with C++ specific constant.
protected static final ImmutableList<String> DEFAULT_MAKE_VARIABLE_ATTRIBUTES =
Expand Down Expand Up @@ -117,14 +121,14 @@ public String lookupVariable(String name) throws ExpansionException {
throw new ExpansionException(String.format("$(%s) not defined", name));
}

public Dict<String, String> collectMakeVariables() throws ExpansionException {
Dict.Builder<String, String> map = Dict.builder();
public SkylarkDict<String, String> collectMakeVariables() throws ExpansionException {
Map<String, String> map = new LinkedHashMap<>();
// Collect variables in the reverse order as in lookupMakeVariable
// because each update is overwriting.
for (MakeVariableSupplier supplier : allMakeVariableSuppliers.reverse()) {
map.putAll(supplier.getAllMakeVariables());
}
return map.buildImmutable();
return SkylarkDict.<String, String>copyOf(null, map);
}

@Override
Expand Down
5 changes: 3 additions & 2 deletions ...uild.bazel/src/main/java/com/google/devtools/build/lib/analysis/MakeVariableSupplier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
Expand All @@ -30,10 +31,10 @@ public interface MakeVariableSupplier {

/** Returns Make variable value or null if value is not supplied. */
@Nullable
String getMakeVariable(String variableName);
String getMakeVariable(String variableName) throws ExpansionException;

/** Returns all Make variables that it supplies */
ImmutableMap<String, String> getAllMakeVariables();
ImmutableMap<String, String> getAllMakeVariables() throws ExpansionException;

/**
* {@link MakeVariableSupplier} that reads variables from a list of {@link TemplateVariableInfo}
Expand Down
2 changes: 1 addition & 1 deletion ...azel/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ private static StructImpl buildSplitAttributeInfo(
ImmutableMap.Builder<String, Object> splitAttrInfos = ImmutableMap.builder();
for (Attribute attr : attributes) {

if (attr.getTransitionFactory().isSplit()) {
if (attr.hasSplitConfigurationTransition()) {

Map<Optional<String>, ? extends List<? extends TransitiveInfoCollection>> splitPrereqs =
ruleContext.getSplitPrerequisites(attr.getName());
Expand Down
84 changes: 8 additions & 76 deletions ...Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.rules.cpp;

import static com.google.devtools.build.lib.packages.BuildType.LABEL;

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
Expand All @@ -27,10 +25,8 @@
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.MakeVariableSupplier;
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.config.CompilationMode;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
import com.google.devtools.build.lib.analysis.platform.ToolchainInfo;
Expand Down Expand Up @@ -124,14 +120,10 @@ public void collectMetadataArtifacts(Iterable<Artifact> objectFiles,
CppActionNames.PREPROCESS_ASSEMBLE,
CppActionNames.CLIF_MATCH,
CppActionNames.LINKSTAMP_COMPILE,
CppActionNames.CC_FLAGS_MAKE_VARIABLE,
CppActionNames.LTO_BACKEND);
CppActionNames.CC_FLAGS_MAKE_VARIABLE);

public static final ImmutableSet<String> ALL_LINK_ACTIONS =
ImmutableSet.of(
CppActionNames.LTO_INDEX_EXECUTABLE,
CppActionNames.LTO_INDEX_DYNAMIC_LIBRARY,
CppActionNames.LTO_INDEX_NODEPS_DYNAMIC_LIBRARY,
LinkTargetType.EXECUTABLE.getActionName(),
Link.LinkTargetType.DYNAMIC_LIBRARY.getActionName(),
Link.LinkTargetType.NODEPS_DYNAMIC_LIBRARY.getActionName());
Expand Down Expand Up @@ -710,25 +702,9 @@ Iterable<Artifact> getLinkerScripts() {
/** Returns the Windows DEF file specified in win_def_file attribute of the rule. */
@Nullable
Artifact getWinDefFile() {
if (!ruleContext.isAttrDefined("win_def_file", LABEL)) {
return null;
}

return ruleContext.getPrerequisiteArtifact("win_def_file", Mode.TARGET);
}

/**
* Returns the parser & Windows DEF file generator specified in $def_parser attribute of the rule.
*/
@Nullable
Artifact getDefParser() {
if (!ruleContext.isAttrDefined("$def_parser", LABEL)) {
return null;
}

return ruleContext.getPrerequisiteArtifact("$def_parser", Mode.HOST);
}

/** Provides support for instrumentation. */
public InstrumentedFilesInfo getInstrumentedFilesProvider(
Iterable<Artifact> files, boolean withBaselineCoverage) throws RuleErrorException {
Expand Down Expand Up @@ -866,19 +842,14 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(

allFeatures.addAll(getCoverageFeatures(cppConfiguration));

if (!allUnsupportedFeatures.contains(CppRuleClasses.FDO_INSTRUMENT)) {
if (cppConfiguration.getFdoInstrument() != null) {
allFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
} else {
if (cppConfiguration.getCSFdoInstrument() != null) {
allFeatures.add(CppRuleClasses.CS_FDO_INSTRUMENT);
}
}
String fdoInstrument = cppConfiguration.getFdoInstrument();
if (fdoInstrument != null && !allUnsupportedFeatures.contains(CppRuleClasses.FDO_INSTRUMENT)) {
allFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
}

FdoContext.BranchFdoProfile branchFdoProvider = toolchain.getFdoContext().getBranchFdoProfile();
if (branchFdoProvider != null && cppConfiguration.getCompilationMode() == CompilationMode.OPT) {
if ((branchFdoProvider.isLlvmFdo() || branchFdoProvider.isLlvmCSFdo())
if (branchFdoProvider.isLlvmFdo()
&& !allUnsupportedFeatures.contains(CppRuleClasses.FDO_OPTIMIZE)) {
allFeatures.add(CppRuleClasses.FDO_OPTIMIZE);
// For LLVM, support implicit enabling of ThinLTO for FDO unless it has been
Expand All @@ -888,9 +859,6 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
allFeatures.add(CppRuleClasses.ENABLE_FDO_THINLTO);
}
}
if (branchFdoProvider.isLlvmCSFdo()) {
allFeatures.add(CppRuleClasses.CS_FDO_OPTIMIZE);
}
if (branchFdoProvider.isAutoFdo()) {
allFeatures.add(CppRuleClasses.AUTOFDO);
// For LLVM, support implicit enabling of ThinLTO for AFDO unless it has been
Expand Down Expand Up @@ -1006,8 +974,9 @@ private static List<String> computeCcFlagsFromFeatureConfig(
FeatureConfiguration featureConfiguration = null;
CppConfiguration cppConfiguration;
if (toolchainProvider.requireCtxInConfigureFeatures()) {
// When --incompatible_require_ctx_in_configure_features is flipped, this whole method will go
// away. But I'm keeping it there so we can experiment with flags before they are flipped.
// When this is flipped, this whole method will go away. But I'm keeping it there
// so we can experiment with flags before they are flipped.
Preconditions.checkArgument(toolchainProvider.disableGenruleCcToolchainDependency());
cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
} else {
cppConfiguration =
Expand All @@ -1032,41 +1001,4 @@ private static List<String> computeCcFlagsFromFeatureConfig(
}
return ImmutableList.of();
}

/** Returns artifacts that help debug the state of C++ features for the given ruleContext. */
public static Map<String, NestedSet<Artifact>> createSaveFeatureStateArtifacts(
CppConfiguration cppConfiguration,
FeatureConfiguration featureConfiguration,
RuleContext ruleContext) {

ImmutableMap.Builder<String, NestedSet<Artifact>> outputGroupsBuilder = ImmutableMap.builder();

if (cppConfiguration.saveFeatureState()) {
Artifact enabledFeaturesFile =
ruleContext.getUniqueDirectoryArtifact("feature_debug", "enabled_features.txt");
ruleContext.registerAction(
FileWriteAction.create(
ruleContext,
enabledFeaturesFile,
featureConfiguration.getEnabledFeatureNames().toString(),
/* makeExecutable= */ false));

Artifact requestedFeaturesFile =
ruleContext.getUniqueDirectoryArtifact("feature_debug", "requested_features.txt");
ruleContext.registerAction(
FileWriteAction.create(
ruleContext,
requestedFeaturesFile,
featureConfiguration.getRequestedFeatures().toString(),
/* makeExecutable= */ false));

outputGroupsBuilder.put(
OutputGroupInfo.DEFAULT,
NestedSetBuilder.<Artifact>stableOrder()
.add(enabledFeaturesFile)
.add(requestedFeaturesFile)
.build());
}
return outputGroupsBuilder.build();
}
}
59 changes: 22 additions & 37 deletions ...zelbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLinkingHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import com.google.devtools.build.lib.rules.cpp.Link.LinkingMode;
import com.google.devtools.build.lib.rules.cpp.Link.Picness;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.LinkingInfoApi;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
Expand Down Expand Up @@ -263,15 +262,7 @@ public CcLinkingHelper setLinkerOutputArtifact(@Nullable Artifact linkerOutputAr
* <p>This only sets the link type (see {@link #setStaticLinkType}), either to a static library or
* to an alwayslink static library (blaze uses a different file extension to signal alwayslink to
* downstream code).
*
* @deprecated This is only set here for naming always to link static libraries with the *.lo
* extension. This is no longer needed because {@link
* com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink}s already carry
* information about whether a library should always be linked or not. This method will be
* removed when we no longer use *.lo for always to link static libraries in native
* cc_library.
*/
@Deprecated
public CcLinkingHelper setAlwayslink(boolean alwayslink) {
staticLinkType =
alwayslink ? LinkTargetType.ALWAYS_LINK_STATIC_LIBRARY : LinkTargetType.STATIC_LIBRARY;
Expand All @@ -283,6 +274,7 @@ public CcLinkingHelper setAlwayslink(boolean alwayslink) {
* anything other than a static link causes this class to skip the link action creation. This
* exists only for Objective-C.
*/
@Deprecated
public CcLinkingHelper setStaticLinkType(LinkTargetType linkType) {
Preconditions.checkNotNull(linkType);
Preconditions.checkState(linkType.linkerOrArchiver() == LinkerOrArchiver.ARCHIVER);
Expand Down Expand Up @@ -450,8 +442,12 @@ private CcLinkingOutputs createCcLinkActions(CcCompilationOutputs ccOutputs)
(!dynamicLinkType.isExecutable() && usePicForDynamicLibs)
|| (dynamicLinkType.isExecutable() && usePicForBinaries);
hasBuiltDynamicLibrary =
createDynamicLinkAction(
ccLinkingOutputsBuilder, libraryToLinkBuilder, usePic, libraryIdentifier, ccOutputs);
createDynamicLibrary(
ccLinkingOutputsBuilder,
libraryToLinkBuilder,
usePic,
libraryIdentifier,
ccOutputs);
}

if (hasBuiltDynamicLibrary || shouldCreateStaticLibraries) {
Expand Down Expand Up @@ -625,7 +621,7 @@ private CppLinkAction registerActionForStaticLibrary(
return action;
}

private boolean createDynamicLinkAction(
private boolean createDynamicLibrary(
CcLinkingOutputs.Builder ccLinkingOutputs,
LibraryToLink.Builder libraryToLinkBuilder,
boolean usePic,
Expand All @@ -634,21 +630,21 @@ private boolean createDynamicLinkAction(
throws RuleErrorException, InterruptedException {
boolean hasBuiltDynamicLibrary = false;
// Create dynamic library.
Artifact linkerOutput;
Artifact soImpl;
String mainLibraryIdentifier;
if (linkerOutputArtifact == null) {
// If the crosstool is configured to select an output artifact, we use that selection.
// Otherwise, we use linux defaults.
linkerOutput = getLinkedArtifact(dynamicLinkType);
soImpl = getLinkedArtifact(LinkTargetType.NODEPS_DYNAMIC_LIBRARY);
mainLibraryIdentifier = libraryIdentifier;
} else {
// This branch is only used for vestigial Google-internal rules where the name of the output
// file is explicitly specified in the BUILD file and as such, is platform-dependent. Thus,
// we just hardcode some reasonable logic to compute the library identifier and hope that this
// will eventually go away.
linkerOutput = linkerOutputArtifact;
soImpl = linkerOutputArtifact;
mainLibraryIdentifier =
FileSystemUtils.removeExtension(linkerOutput.getRootRelativePath().getPathString());
FileSystemUtils.removeExtension(soImpl.getRootRelativePath().getPathString());
}

List<String> sonameLinkopts = ImmutableList.of();
Expand All @@ -662,12 +658,12 @@ private boolean createDynamicLinkAction(
ImmutableList.of(
"-Wl,-soname="
+ SolibSymlinkAction.getDynamicLibrarySoname(
linkerOutput.getRootRelativePath(), /* preserveName= */ false));
soImpl.getRootRelativePath(), /* preserveName= */ false));
}
}

CppLinkActionBuilder dynamicLinkActionBuilder =
newLinkActionBuilder(linkerOutput)
newLinkActionBuilder(soImpl)
.setWholeArchive(wholeArchive)
.setNativeDeps(nativeDeps)
.setAdditionalLinkstampDefines(additionalLinkstampDefines.build())
Expand All @@ -694,23 +690,15 @@ private boolean createDynamicLinkAction(
}

if (linkingMode == LinkingMode.DYNAMIC) {
try {
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.DYNAMIC_LIBRARY,
ccToolchain.getDynamicRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getDynamicRuntimeLinkInputs(featureConfiguration));
} catch (EvalException e) {
throw ruleErrorConsumer.throwWithRuleError(e.getMessage());
}
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.DYNAMIC_LIBRARY,
ccToolchain.getDynamicRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getDynamicRuntimeLinkInputs(ruleErrorConsumer, featureConfiguration));
} else {
try {
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.STATIC_LIBRARY,
ccToolchain.getStaticRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getStaticRuntimeLinkInputs(featureConfiguration));
} catch (EvalException e) {
throw ruleErrorConsumer.throwWithRuleError(e.getMessage());
}
dynamicLinkActionBuilder.setRuntimeInputs(
ArtifactCategory.STATIC_LIBRARY,
ccToolchain.getStaticRuntimeLinkMiddleman(ruleErrorConsumer, featureConfiguration),
ccToolchain.getStaticRuntimeLinkInputs(ruleErrorConsumer, featureConfiguration));
}

// On Windows, we cannot build a shared library with symbols unresolved, so here we
Expand Down Expand Up @@ -758,9 +746,6 @@ private boolean createDynamicLinkAction(
ccLinkingOutputs.addAllLtoArtifacts(dynamicLinkActionBuilder.getAllLtoBackendArtifacts());
}
CppLinkAction dynamicLinkAction = dynamicLinkActionBuilder.build();
if (dynamicLinkType.isExecutable()) {
ccLinkingOutputs.setExecutable(linkerOutput);
}
ccLinkingOutputs.addLinkActionInputs(dynamicLinkAction.getInputs());
actionConstructionContext.registerAction(dynamicLinkAction);

Expand Down
Loading

0 comments on commit 211dc1c

Please sign in to comment.