Skip to content

Starlarkify and split coverage features #26355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fmeum wants to merge 1 commit into from
Closed

Starlarkify and split coverage features #26355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
716 changes: 716 additions & 0 deletions MODULE.bazel.lock
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 16 additions & 2 deletions site/en/docs/cc-toolchain-config-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,22 @@ conditions.
<td>
If enabled, virtual include header files are linked under <code>bin/_virtual_includes/&lt;hash of target path&gt;</code> instead of <code>bin/&lt;target package path&gt;/_virtual_includes/&lt;target name&gt;</code>. Useful on Windows to avoid long path issue with MSVC.
</td>
</tr>
</tr>
<tr>
<td><strong><code>coverage_enabled</code></strong>
</td>
<td>
Enabled when coverage collection is generally enabled.
</td>
</tr>
<tr>
<td><strong><code>coverage_instrumented</code></strong>
</td>
<td>
Enabled when the current target should be instrumented for
coverage.
</td>
</tr>
</table>

#### Legacy features patching logic {:#legacy-features-patching-logic}
Expand Down Expand Up @@ -1157,4 +1172,3 @@ done. For the curious reader see the implementation in
[CppActionConfigs](https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java?q=cppactionconfigs&ss=bazel),
and for production toolchains consider adding `no_legacy_features` to make
the toolchain more standalone.

22 changes: 5 additions & 17 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,19 +341,6 @@ public Sequence<String> getLooseIncludeDirsForStarlark() {
return StarlarkList.empty();
}

public static ImmutableList<String> getCoverageFeatures(CppConfiguration cppConfiguration) {
ImmutableList.Builder<String> coverageFeatures = ImmutableList.builder();
if (cppConfiguration.collectCodeCoverage()) {
coverageFeatures.add(CppRuleClasses.COVERAGE);
if (cppConfiguration.useLLVMCoverageMapFormat()) {
coverageFeatures.add(CppRuleClasses.LLVM_COVERAGE_MAP_FORMAT);
} else {
coverageFeatures.add(CppRuleClasses.GCC_COVERAGE_MAP_FORMAT);
}
}
return coverageFeatures.build();
}

/**
* Creates a feature configuration for a given rule. Assumes strictly cc sources.
*
Expand Down Expand Up @@ -468,8 +455,6 @@ public static FeatureConfiguration configureFeaturesOrThrowEvalException(
}
}

allFeatures.addAll(getCoverageFeatures(cppConfiguration));

if (!allUnsupportedFeatures.contains(CppRuleClasses.FDO_INSTRUMENT)) {
if (cppConfiguration.getFdoInstrument() != null) {
allFeatures.add(CppRuleClasses.FDO_INSTRUMENT);
Expand Down Expand Up @@ -630,8 +615,11 @@ private static List<String> computeCcFlagsFromFeatureConfig(
if (featureConfiguration.actionIsConfigured(CppActionNames.CC_FLAGS_MAKE_VARIABLE)) {
try {
CcToolchainVariables buildVariables = toolchainProvider.getBuildVars();
return CppHelper.getCommandLine(
ruleContext, featureConfiguration, buildVariables, CppActionNames.CC_FLAGS_MAKE_VARIABLE);
return CppHelper.getCommandLine(
ruleContext,
featureConfiguration,
buildVariables,
CppActionNames.CC_FLAGS_MAKE_VARIABLE);

} catch (EvalException e) {
throw new RuleErrorException(e.getMessage());
Expand Down
74 changes: 8 additions & 66 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

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


import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand All @@ -26,15 +25,12 @@
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RuleErrorConsumer;
import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
import com.google.devtools.build.lib.analysis.test.InstrumentedFilesCollector;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter;
import com.google.devtools.build.lib.rules.cpp.CcCommon.Language;
Expand Down Expand Up @@ -297,7 +293,6 @@ public CcCompilationContext getCcCompilationContext() {
private String purpose = "";
private boolean generateNoPicAction;
private boolean generatePicAction;
private boolean isCodeCoverageEnabled = true;
private String stripIncludePrefix = null;
private String includePrefix = null;

Expand Down Expand Up @@ -727,12 +722,6 @@ public CcCompilationHelper setStripIncludePrefix(@Nullable String stripIncludePr
return this;
}

@CanIgnoreReturnValue
public CcCompilationHelper setCodeCoverageEnabled(boolean codeCoverageEnabled) {
this.isCodeCoverageEnabled = codeCoverageEnabled;
return this;
}

private static StarlarkList<String> convertPathFragmentsToStarlarkList(
Iterable<PathFragment> pathFragments) {
ImmutableList.Builder<String> pathStrings = ImmutableList.builder();
Expand Down Expand Up @@ -1040,7 +1029,6 @@ private CcCompilationOutputs createCcCompileActions()
: ArtifactCategory.OBJECT_FILE,
ccCompilationContext.getCppModuleMap(),
/* addObject= */ true,
isCodeCoverageEnabled,
// The source action does not generate dwo when it has bitcode
// output (since it isn't generating a native object with debug
// info). In that case the LtoBackendAction will generate the dwo.
Expand Down Expand Up @@ -1127,7 +1115,7 @@ private Artifact createCompileActionTemplate(
ImmutableList<ArtifactCategory> outputCategories,
boolean usePic,
boolean bitcodeOutput)
throws RuleErrorException, EvalException, InterruptedException {
throws RuleErrorException, EvalException {
if (usePic) {
builder = new CppCompileActionBuilder(builder).setPicMode(true);
}
Expand All @@ -1144,7 +1132,6 @@ private Artifact createCompileActionTemplate(
usePic,
/* needsFdoBuildVariables= */ false,
ccCompilationContext.getCppModuleMap(),
/* enableCoverage= */ false,
/* gcnoFile= */ null,
/* isUsingFission= */ false,
/* dwoFile= */ null,
Expand Down Expand Up @@ -1271,13 +1258,12 @@ private CcToolchainVariables setupCompileBuildVariables(
boolean usePic,
boolean needsFdoBuildVariables,
CppModuleMap cppModuleMap,
boolean enableCoverage,
Artifact gcnoFile,
boolean isUsingFission,
Artifact dwoFile,
Artifact ltoIndexingFile,
ImmutableMap<String, String> additionalBuildVariables)
throws RuleErrorException, EvalException, InterruptedException {
throws RuleErrorException, EvalException {
Artifact sourceFile = builder.getSourceFile();
if (needsFdoBuildVariables && fdoContext.hasArtifacts()) {
// This modifies the passed-in builder, which is a surprising side-effect, and makes it unsafe
Expand Down Expand Up @@ -1353,7 +1339,6 @@ private CcToolchainVariables setupCompileBuildVariables(
buildVariables,
sourceFile,
builder.getOutputFile(),
enableCoverage,
gcnoFile,
dwoFile,
isUsingFission,
Expand Down Expand Up @@ -1406,7 +1391,8 @@ private void createModuleCodegenAction(
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);
// TODO(djasper): This is now duplicated. Refactor the various create..Action functions.
Artifact gcnoFile =
isCodeCoverageEnabled && !cppConfiguration.useLLVMCoverageMapFormat()
featureConfiguration.getRequestedFeatures().contains(CppRuleClasses.COVERAGE_INSTRUMENTED)
&& !cppConfiguration.useLLVMCoverageMapFormat()
? CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, gcnoFileName, configuration)
: null;
Expand All @@ -1427,7 +1413,6 @@ private void createModuleCodegenAction(
/* usePic= */ pic,
/* needsFdoBuildVariables= */ ccRelativeName != null,
ccCompilationContext.getCppModuleMap(),
isCodeCoverageEnabled,
gcnoFile,
generateDwo,
dwoFile,
Expand Down Expand Up @@ -1474,7 +1459,6 @@ private void createHeaderAction(
generatePicAction,
/* needsFdoBuildVariables= */ false,
ccCompilationContext.getCppModuleMap(),
/* enableCoverage= */ false,
/* gcnoFile= */ null,
/* isUsingFission= */ false,
/* dwoFile= */ null,
Expand Down Expand Up @@ -1507,7 +1491,6 @@ private ImmutableList<Artifact> createModuleAction(
ArtifactCategory.CPP_MODULE,
cppModuleMap,
/* addObject= */ false,
/* enableCoverage= */ false,
/* generateDwo= */ false,
/* bitcodeOutput= */ false);
}
Expand All @@ -1522,7 +1505,6 @@ private ImmutableList<Artifact> createSourceAction(
ArtifactCategory outputCategory,
CppModuleMap cppModuleMap,
boolean addObject,
boolean enableCoverage,
boolean generateDwo,
boolean bitcodeOutput)
throws RuleErrorException, EvalException, InterruptedException {
Expand All @@ -1544,7 +1526,6 @@ private ImmutableList<Artifact> createSourceAction(
outputCategory,
cppModuleMap,
addObject,
enableCoverage,
generateDwo,
bitcodeOutput,
ccRelativeName,
Expand All @@ -1567,7 +1548,6 @@ private ImmutableList<Artifact> createSourceAction(
outputCategory,
cppModuleMap,
addObject,
enableCoverage,
generateDwo,
bitcodeOutput,
ccRelativeName,
Expand All @@ -1590,13 +1570,12 @@ private Artifact createSourceActionHelper(
ArtifactCategory outputCategory,
CppModuleMap cppModuleMap,
boolean addObject,
boolean enableCoverage,
boolean generateDwo,
boolean bitcodeOutput,
PathFragment ccRelativeName,
boolean usePic,
ImmutableMap<String, String> additionalBuildVariables)
throws RuleErrorException, EvalException, InterruptedException {
throws RuleErrorException, EvalException {
builder.setOutputs(
actionConstructionContext,
ruleErrorConsumer,
Expand All @@ -1610,7 +1589,8 @@ private Artifact createSourceActionHelper(
getOutputNameBaseWith(outputName, usePic));

Artifact gcnoFile =
enableCoverage && !cppConfiguration.useLLVMCoverageMapFormat()
featureConfiguration.getRequestedFeatures().contains(CppRuleClasses.COVERAGE_INSTRUMENTED)
&& !cppConfiguration.useLLVMCoverageMapFormat()
? CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, gcnoFileName, configuration)
: null;
Expand All @@ -1625,7 +1605,6 @@ private Artifact createSourceActionHelper(
usePic,
/* needsFdoBuildVariables= */ ccRelativeName != null && addObject,
cppModuleMap,
enableCoverage,
gcnoFile,
generateDwo,
dwoFile,
Expand Down Expand Up @@ -1679,41 +1658,6 @@ String getOutputNameBaseWith(String base, boolean usePic) throws RuleErrorExcept
: base;
}

/** Returns true iff code coverage is enabled for the given target. */
public static boolean isCodeCoverageEnabled(RuleContext ruleContext) {
BuildConfigurationValue configuration = ruleContext.getConfiguration();
if (configuration.isCodeCoverageEnabled()) {
// If rule is matched by the instrumentation filter, enable instrumentation
if (InstrumentedFilesCollector.shouldIncludeLocalSources(
configuration, ruleContext.getLabel(), ruleContext.isTestTarget())) {
return true;
}
// At this point the rule itself is not matched by the instrumentation filter. However, we
// might still want to instrument C++ rules if one of the targets listed in "deps" is
// instrumented and, therefore, can supply header files that we would want to collect code
// coverage for. For example, think about cc_test rule that tests functionality defined in a
// header file that is supplied by the cc_library.
//
// Note that we only check direct prerequisites and not the transitive closure. This is done
// for two reasons:
// a) It is a good practice to declare libraries which you directly rely on. Including headers
// from a library hidden deep inside the transitive closure makes build dependencies less
// readable and can lead to unexpected breakage.
// b) Traversing the transitive closure for each C++ compile action would require more complex
// implementation (with caching results of this method) to avoid O(N^2) slowdown.
if (ruleContext.getRule().isAttrDefined("deps", BuildType.LABEL_LIST)) {
for (TransitiveInfoCollection dep : ruleContext.getPrerequisites("deps")) {
CcInfo ccInfo = dep.get(CcInfo.PROVIDER);
if (ccInfo != null
&& InstrumentedFilesCollector.shouldIncludeLocalSources(configuration, dep)) {
return true;
}
}
}
}
return false;
}

private ImmutableList<String> collectPerFileCopts(Artifact sourceFile, Label sourceLabel) {
return cppConfiguration.getPerFileCopts().stream()
.filter(
Expand Down Expand Up @@ -1748,7 +1692,7 @@ private ImmutableList<Artifact> createTempsActions(
CppCompileActionBuilder builder,
boolean usePic,
PathFragment ccRelativeName)
throws RuleErrorException, EvalException, InterruptedException {
throws RuleErrorException, EvalException {
if (!cppConfiguration.getSaveTemps()) {
return ImmutableList.of();
}
Expand Down Expand Up @@ -1778,7 +1722,6 @@ private ImmutableList<Artifact> createTempsActions(
usePic,
/* needsFdoBuildVariables= */ ccRelativeName != null,
ccCompilationContext.getCppModuleMap(),
/* enableCoverage= */ false,
/* gcnoFile= */ null,
/* isUsingFission= */ false,
/* dwoFile= */ null,
Expand All @@ -1804,7 +1747,6 @@ private ImmutableList<Artifact> createTempsActions(
usePic,
/* needsFdoBuildVariables= */ ccRelativeName != null,
ccCompilationContext.getCppModuleMap(),
/* enableCoverage= */ false,
/* gcnoFile= */ null,
/* isUsingFission= */ false,
/* dwoFile= */ null,
Expand Down
16 changes: 4 additions & 12 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,6 @@ public CcToolchainVariables getCompileBuildVariables(
ccToolchainProvider,
convertFromNoneable(sourceFile, /* defaultValue= */ null),
convertFromNoneable(outputFile, /* defaultValue= */ null),
/* isCodeCoverageEnabled= */ false,
/* gcnoFile= */ null,
/* isUsingFission= */ false,
/* dwoFile= */ null,
Expand Down Expand Up @@ -1605,7 +1604,6 @@ public Tuple compile(
Object additionalModuleMapsNoneable,
Object propagateModuleMapToCompileActionObject,
Object doNotGenerateModuleMapObject,
Object codeCoverageEnabledObject,
Object hdrsCheckingModeObject,
Object variablesExtension,
Object languageObject,
Expand Down Expand Up @@ -1651,7 +1649,7 @@ public Tuple compile(
asClassImmutableList(additionalModuleMapsNoneable);

String coptsFilterRegex = convertFromNoneable(coptsFilterObject, /* defaultValue= */ null);
CoptsFilter coptsFilter = null;
CoptsFilter coptsFilter;
if (Strings.isNullOrEmpty(coptsFilterRegex)) {
coptsFilter = CoptsFilter.alwaysPasses();
} else {
Expand All @@ -1678,8 +1676,6 @@ public Tuple compile(
convertFromNoneable(propagateModuleMapToCompileActionObject, /* defaultValue= */ true);
boolean doNotGenerateModuleMap =
convertFromNoneable(doNotGenerateModuleMapObject, /* defaultValue= */ false);
boolean codeCoverageEnabled =
convertFromNoneable(codeCoverageEnabledObject, /* defaultValue= */ false);
String purpose = convertFromNoneable(purposeObject, null);
ImmutableList<CcCompilationContext> implementationContexts =
asClassImmutableList(implementationCcCompilationContextsObject);
Expand Down Expand Up @@ -1757,8 +1753,7 @@ public Tuple compile(
.setPurpose(defaultPurpose)
.addAdditionalExportedHeaders(
additionalExportedHeaders.stream().map(PathFragment::create).collect(toImmutableList()))
.setPropagateModuleMapToCompileAction(propagateModuleMapToCompileAction)
.setCodeCoverageEnabled(codeCoverageEnabled);
.setPropagateModuleMapToCompileAction(propagateModuleMapToCompileAction);

if (textualHeadersObject instanceof NestedSet) {
compilationHelper.addPublicTextualHeaders(
Expand All @@ -1772,9 +1767,7 @@ public Tuple compile(
if (moduleMap != null) {
compilationHelper.setCppModuleMap(moduleMap);
}
if (coptsFilter != null) {
compilationHelper.setCoptsFilter(coptsFilter);
}
compilationHelper.setCoptsFilter(coptsFilter);
for (CppModuleMap additionalModuleMap : additionalModuleMaps) {
compilationHelper.registerAdditionalModuleMap(additionalModuleMap);
}
Expand Down Expand Up @@ -1986,7 +1979,7 @@ public void registerLinkstampCompileAction(
Object stampingObject,
Object additionalLinkstampDefines,
StarlarkThread thread)
throws EvalException, InterruptedException, TypeException, RuleErrorException {
throws EvalException, InterruptedException, TypeException {
isCalledFromStarlarkCcCommon(thread);
RuleContext ruleContext = starlarkActionFactoryApi.getRuleContext();
boolean stamping =
Expand Down Expand Up @@ -2024,7 +2017,6 @@ public void registerLinkstampCompileAction(
.toList(),
asStringImmutableList(additionalLinkstampDefines),
ccToolchain,
ruleContext.getConfiguration().isCodeCoverageEnabled(),
CppHelper.getFdoBuildStamp(
cppConfiguration,
ccToolchain.getFdoContext(),
Expand Down
Loading
Loading