Skip to content

Commit

Permalink
Clean up some C++-related code I came across while modifying CppCompi…
Browse files Browse the repository at this point in the history 
…leActionTemplate:

1. In production ArtifactNamePattern is always in 1-1 correspondence with ArtifactCategory. Modify data structures to make that explicit, and remove exceptions that could never be thrown.
2. CppCompileActionBuilder#build could theoretically emit multiple errors but in reality only ever emitted one. Modify code to make that explicit, which simplifies calling code.
2a. Tighten behavior of CppCompileActionBuilder so that it's clearer that getActionName() is constant over the lifetime of the method buildAndVerify().

PiperOrigin-RevId: 421039367
  • Loading branch information
@janakdr @copybara-github
janakdr authored and copybara-github committed Jan 11, 2022
1 parent 16da031 commit ce19076
Show file tree
Hide file tree
Showing 13 changed files with 245 additions and 318 deletions.
35 changes: 15 additions & 20 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1530,7 +1530,7 @@ private Artifact createCompileActionTemplate(
CppSource source,
String outputName,
CppCompileActionBuilder builder,
Iterable<ArtifactCategory> outputCategories,
ImmutableList<ArtifactCategory> outputCategories,
boolean usePic) {
if (usePic) {
builder = new CppCompileActionBuilder(builder).setPicMode(true);
Expand Down Expand Up @@ -1719,15 +1719,13 @@ private static String toPathString(Artifact a) {
* initialized.
*/
private CppCompileActionBuilder initializeCompileAction(Artifact sourceArtifact) {
CppCompileActionBuilder builder =
new CppCompileActionBuilder(
actionConstructionContext, grepIncludes, ccToolchain, configuration);
builder.setSourceFile(sourceArtifact);
builder.setCcCompilationContext(ccCompilationContext);
builder.setCoptsFilter(coptsFilter);
builder.setFeatureConfiguration(featureConfiguration);
builder.addExecutionInfo(executionInfo);
return builder;
return new CppCompileActionBuilder(
actionConstructionContext, grepIncludes, ccToolchain, configuration)
.setSourceFile(sourceArtifact)
.setCcCompilationContext(ccCompilationContext)
.setCoptsFilter(coptsFilter)
.setFeatureConfiguration(featureConfiguration)
.addExecutionInfo(executionInfo);
}

private void createModuleCodegenAction(
Expand All @@ -1751,7 +1749,7 @@ private void createModuleCodegenAction(

String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);
// TODO(djasper): This is now duplicated. Refactor the various create..Action functions.
Artifact gcnoFile =
isCodeCoverageEnabled
Expand Down Expand Up @@ -1804,7 +1802,7 @@ private void createHeaderAction(
throws RuleErrorException {
String outputNameBase =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.GENERATED_HEADER, outputName);
ccToolchain, ArtifactCategory.GENERATED_HEADER, outputName);

builder
.setOutputs(
Expand Down Expand Up @@ -1880,12 +1878,11 @@ private ImmutableList<Artifact> createSourceAction(
// generate .pic.o, .pic.d, .pic.gcno instead of .o, .d, .gcno.)
if (generatePicAction) {
String picOutputBase =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.PIC_FILE, outputName);
CppHelper.getArtifactNameForCategory(ccToolchain, ArtifactCategory.PIC_FILE, outputName);
CppCompileActionBuilder picBuilder = copyAsPicBuilder(builder, picOutputBase, outputCategory);
String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, picOutputBase);
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, picOutputBase);
Artifact gcnoFile =
enableCoverage
? CppHelper.getCompileOutputArtifact(
Expand Down Expand Up @@ -1946,14 +1943,13 @@ private ImmutableList<Artifact> createSourceAction(
CppHelper.getCompileOutputArtifact(
actionConstructionContext,
label,
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, outputCategory, outputName),
CppHelper.getArtifactNameForCategory(ccToolchain, outputCategory, outputName),
configuration);
builder.setOutputs(
actionConstructionContext, ruleErrorConsumer, label, outputCategory, outputName);
String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);

// Create no-PIC compile actions
Artifact gcnoFile =
Expand Down Expand Up @@ -2030,8 +2026,7 @@ private CppCompileActionBuilder copyAsPicBuilder(

String getOutputNameBaseWith(String base, boolean usePic) throws RuleErrorException {
return usePic
? CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.PIC_FILE, base)
? CppHelper.getArtifactNameForCategory(ccToolchain, ArtifactCategory.PIC_FILE, base)
: base;
}

Expand Down
26 changes: 11 additions & 15 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,21 +339,17 @@ public static void init(

Artifact defParser = common.getDefParser();
if (defParser != null) {
try {
generatedDefFile =
CppHelper.createDefFileActions(
ruleContext,
defParser,
ccCompilationOutputs.getObjectFiles(false),
ccToolchain
.getFeatures()
.getArtifactNameForCategory(
ArtifactCategory.DYNAMIC_LIBRARY,
ruleContext.getLabel().getName() + dllNameSuffix));
targetBuilder.addOutputGroup(DEF_FILE_OUTPUT_GROUP_NAME, generatedDefFile);
} catch (EvalException e) {
throw ruleContext.throwWithRuleError(e);
}
generatedDefFile =
CppHelper.createDefFileActions(
ruleContext,
defParser,
ccCompilationOutputs.getObjectFiles(false),
ccToolchain
.getFeatures()
.getArtifactNameForCategory(
ArtifactCategory.DYNAMIC_LIBRARY,
ruleContext.getLabel().getName() + dllNameSuffix));
targetBuilder.addOutputGroup(DEF_FILE_OUTPUT_GROUP_NAME, generatedDefFile);
}
linkingHelper.setDefFile(
CppHelper.getWindowsDefFileForLinking(
Expand Down
4 changes: 2 additions & 2 deletions 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 @@ -900,7 +900,7 @@ private Artifact getLinkedArtifact(LinkTargetType linkTargetType) throws RuleErr
if (linkTargetType.picness() == Picness.PIC) {
maybePicName =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, ArtifactCategory.PIC_FILE, maybePicName);
ccToolchain, ArtifactCategory.PIC_FILE, maybePicName);
}

String linkedName = maybePicName;
Expand All @@ -909,7 +909,7 @@ private Artifact getLinkedArtifact(LinkTargetType linkTargetType) throws RuleErr
}
linkedName =
CppHelper.getArtifactNameForCategory(
ruleErrorConsumer, ccToolchain, linkTargetType.getLinkerOutput(), linkedName);
ccToolchain, linkTargetType.getLinkerOutput(), linkedName);

PathFragment artifactFragment = PathFragment.create(linkedName);
ArtifactRoot artifactRoot = configuration.getBinDirectory(label.getRepository());
Expand Down
50 changes: 12 additions & 38 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import com.google.devtools.build.lib.rules.cpp.CcLinkingContext.LinkOptions;
import com.google.devtools.build.lib.rules.cpp.CcLinkingContext.Linkstamp;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ActionConfig;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ArtifactNamePattern;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.EnvEntry;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.EnvSet;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Feature;
Expand All @@ -76,10 +75,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
import com.google.errorprone.annotations.FormatMethod;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import javax.annotation.Nullable;
import net.starlark.java.annot.Param;
import net.starlark.java.annot.StarlarkMethod;
Expand Down Expand Up @@ -1142,16 +1138,15 @@ public CcToolchainConfigInfo ccToolchainConfigInfoFromStarlark(
.map(actionConfig -> actionConfig.getActionName())
.collect(ImmutableSet.toImmutableSet());

ImmutableList.Builder<ArtifactNamePattern> artifactNamePatternBuilder = ImmutableList.builder();
CcToolchainFeatures.ArtifactNamePatternMapper.Builder artifactNamePatternBuilder =
new CcToolchainFeatures.ArtifactNamePatternMapper.Builder();
for (Object artifactNamePattern : artifactNamePatterns) {
checkRightStarlarkInfoProvider(
artifactNamePattern, "artifact_name_patterns", "ArtifactNamePatternInfo");
artifactNamePatternBuilder.add(
artifactNamePatternFromStarlark((StarlarkInfo) artifactNamePattern));
artifactNamePatternFromStarlark(
(StarlarkInfo) artifactNamePattern, artifactNamePatternBuilder::addOverride);
}

getLegacyArtifactNamePatterns(artifactNamePatternBuilder);

// Pairs (toolName, toolPath)
ImmutableList.Builder<Pair<String, String>> toolPathPairs = ImmutableList.builder();
for (Object toolPath : toolPaths) {
Expand Down Expand Up @@ -1686,10 +1681,14 @@ static ActionConfig actionConfigFromStarlark(StarlarkInfo actionConfigStruct)
actionName, actionName, toolBuilder.build(), flagSetBuilder.build(), enabled, implies);
}

/** Creates an {@link ArtifactNamePattern} from a {@link StarlarkInfo}. */
@VisibleForTesting
static ArtifactNamePattern artifactNamePatternFromStarlark(StarlarkInfo artifactNamePatternStruct)
throws EvalException {
interface ArtifactNamePatternAdder {
void add(ArtifactCategory category, String prefix, String extension);
}

@VisibleForTesting
static void artifactNamePatternFromStarlark(
StarlarkInfo artifactNamePatternStruct, ArtifactNamePatternAdder adder) throws EvalException {
checkRightProviderType(artifactNamePatternStruct, "artifact_name_pattern");
String categoryName =
getMandatoryFieldFromStarlarkProvider(
Expand Down Expand Up @@ -1729,7 +1728,7 @@ static ArtifactNamePattern artifactNamePatternFromStarlark(StarlarkInfo artifact
Strings.nullToEmpty(
getMandatoryFieldFromStarlarkProvider(
artifactNamePatternStruct, "prefix", String.class));
return new ArtifactNamePattern(foundCategory, prefix, extension);
adder.add(foundCategory, prefix, extension);
}

private static <T> T getOptionalFieldFromStarlarkProvider(
Expand Down Expand Up @@ -1788,31 +1787,6 @@ private static ImmutableList<StarlarkInfo> getStarlarkProviderListFromStarlarkFi
: ImmutableList.copyOf(Sequence.noneableCast(v, StarlarkInfo.class, fieldName));
}

private static void getLegacyArtifactNamePatterns(
ImmutableList.Builder<ArtifactNamePattern> patterns) {
Set<ArtifactCategory> definedCategories = new HashSet<>();
for (ArtifactNamePattern pattern : patterns.build()) {
try {
definedCategories.add(
ArtifactCategory.valueOf(
pattern.getArtifactCategory().getCategoryName().toUpperCase(Locale.ENGLISH)));
} catch (IllegalArgumentException e) {
// Invalid category name, will be detected later.
continue;
}
}

for (ArtifactCategory category : ArtifactCategory.values()) {
if (!definedCategories.contains(category)
&& category.getDefaultPrefix() != null
&& category.getDefaultExtension() != null) {
patterns.add(
new ArtifactNamePattern(
category, category.getDefaultPrefix(), category.getDefaultExtension()));
}
}
}

@Nullable
private static <T> T nullIfNone(Object object, Class<T> type) {
return object != Starlark.NONE ? type.cast(object) : null;
Expand Down
46 changes: 32 additions & 14 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainConfigInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@

package com.google.devtools.build.lib.rules.cpp;


import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.packages.BuiltinProvider;
import com.google.devtools.build.lib.packages.NativeInfo;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ActionConfig;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ArtifactNamePattern;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.ArtifactNamePatternMapper;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.EnvEntry;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.EnvSet;
import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Feature;
Expand All @@ -48,7 +49,7 @@ public class CcToolchainConfigInfo extends NativeInfo implements CcToolchainConf

private final ImmutableList<ActionConfig> actionConfigs;
private final ImmutableList<Feature> features;
private final ImmutableList<ArtifactNamePattern> artifactNamePatterns;
private final ArtifactNamePatternMapper artifactNamePatterns;
private final ImmutableList<String> cxxBuiltinIncludeDirectories;

private final String toolchainIdentifier;
Expand All @@ -67,7 +68,7 @@ public class CcToolchainConfigInfo extends NativeInfo implements CcToolchainConf
CcToolchainConfigInfo(
ImmutableList<ActionConfig> actionConfigs,
ImmutableList<Feature> features,
ImmutableList<ArtifactNamePattern> artifactNamePatterns,
ArtifactNamePatternMapper artifactNamePatterns,
ImmutableList<String> cxxBuiltinIncludeDirectories,
String toolchainIdentifier,
String hostSystemName,
Expand Down Expand Up @@ -104,7 +105,9 @@ public Provider getProvider() {
return PROVIDER;
}

public static CcToolchainConfigInfo fromToolchain(CToolchain toolchain) throws EvalException {
@VisibleForTesting // Only called by tests.
public static CcToolchainConfigInfo fromToolchainForTestingOnly(CToolchain toolchain)
throws EvalException {
ImmutableList.Builder<ActionConfig> actionConfigBuilder = ImmutableList.builder();
for (CToolchain.ActionConfig actionConfig : toolchain.getActionConfigList()) {
actionConfigBuilder.add(new ActionConfig(actionConfig));
Expand All @@ -115,10 +118,26 @@ public static CcToolchainConfigInfo fromToolchain(CToolchain toolchain) throws E
featureBuilder.add(new Feature(feature));
}

ImmutableList.Builder<ArtifactNamePattern> artifactNamePatternBuilder = ImmutableList.builder();
ArtifactNamePatternMapper.Builder artifactNamePatternBuilder =
new ArtifactNamePatternMapper.Builder();
for (CToolchain.ArtifactNamePattern artifactNamePattern :
toolchain.getArtifactNamePatternList()) {
artifactNamePatternBuilder.add(new ArtifactNamePattern(artifactNamePattern));
ArtifactCategory foundCategory = null;
for (ArtifactCategory artifactCategory : ArtifactCategory.values()) {
if (artifactNamePattern.getCategoryName().equals(artifactCategory.getCategoryName())) {
foundCategory = artifactCategory;
break;
}
}
Preconditions.checkNotNull(foundCategory, artifactNamePattern);
String extension = artifactNamePattern.getExtension();
Preconditions.checkState(
foundCategory.getAllowedExtensions().contains(extension),
"%s had extension not in %s",
artifactNamePattern,
foundCategory);
artifactNamePatternBuilder.addOverride(
foundCategory, artifactNamePattern.getPrefix(), extension);
}

return new CcToolchainConfigInfo(
Expand Down Expand Up @@ -152,7 +171,7 @@ public ImmutableList<Feature> getFeatures() {
return features;
}

public ImmutableList<ArtifactNamePattern> getArtifactNamePatterns() {
public ArtifactNamePatternMapper getArtifactNamePatterns() {
return artifactNamePatterns;
}

Expand Down Expand Up @@ -218,14 +237,13 @@ public String getProto() {
.map(actionConfig -> actionConfigToProto(actionConfig))
.collect(ImmutableList.toImmutableList()));
cToolchain.addAllArtifactNamePattern(
artifactNamePatterns.stream()
artifactNamePatterns.asImmutableMap().entrySet().stream()
.map(
artifactNamePattern ->
entry ->
CToolchain.ArtifactNamePattern.newBuilder()
.setCategoryName(
artifactNamePattern.getArtifactCategory().getCategoryName())
.setPrefix(artifactNamePattern.getPrefix())
.setExtension(artifactNamePattern.getExtension())
.setCategoryName(entry.getKey().getCategoryName())
.setPrefix(entry.getValue().getPrefix())
.setExtension(entry.getValue().getExtension())
.build())
.collect(ImmutableList.toImmutableList()));
cToolchain.addAllToolPath(
Expand Down
Loading

0 comments on commit ce19076

Please sign in to comment.