Skip to content

Commit

Permalink
Don't thread coverage information through CcToolchainProvider
Browse files Browse the repository at this point in the history 
    Instead read it from the CppConfiguration passed from the target.

    bazelbuild/bazel#6516

    RELNOTES: None
    PiperOrigin-RevId: 240317365
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent e8911d8 commit 7dff199
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 72 deletions.
4 changes: 1 addition & 3 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 @@ -159,7 +159,6 @@ public void collectMetadataArtifacts(Iterable<Artifact> objectFiles,
private final RuleContext ruleContext;

private final CcToolchainProvider ccToolchain;
private final CppConfiguration cppConfiguration;

private final FdoContext fdoContext;

Expand All @@ -168,7 +167,6 @@ public CcCommon(RuleContext ruleContext) {
this.ccToolchain =
Preconditions.checkNotNull(
CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext));
this.cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
this.fdoContext = ccToolchain.getFdoContext();
}

Expand Down Expand Up @@ -716,7 +714,7 @@ public InstrumentedFilesInfo getInstrumentedFilesProvider(
CC_METADATA_COLLECTOR,
files,
CppHelper.getGcovFilesIfNeeded(ruleContext, ccToolchain),
CppHelper.getCoverageEnvironmentIfNeeded(cppConfiguration, ccToolchain),
CppHelper.getCoverageEnvironmentIfNeeded(ruleContext, ccToolchain),
withBaselineCoverage,
virtualToOriginalHeaders);
}
Expand Down
13 changes: 13 additions & 0 deletions ...uild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skylarkbuildapi.cpp.CcToolchainProviderApi;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.CToolchain;
import java.util.Map;
Expand Down Expand Up @@ -79,6 +80,7 @@ public final class CcToolchainProvider extends ToolchainInfo
/* supportsHeaderParsing= */ false,
CcToolchainVariables.EMPTY,
/* builtinIncludeFiles= */ ImmutableList.of(),
/* coverageEnvironment= */ NestedSetBuilder.emptySet(Order.COMPILE_ORDER),
/* linkDynamicLibraryTool= */ null,
/* builtInIncludeDirectories= */ ImmutableList.of(),
/* sysroot= */ null,
Expand Down Expand Up @@ -112,6 +114,7 @@ public final class CcToolchainProvider extends ToolchainInfo
private final boolean supportsHeaderParsing;
private final CcToolchainVariables buildVariables;
private final ImmutableList<Artifact> builtinIncludeFiles;
private final NestedSet<Pair<String, String>> coverageEnvironment;
@Nullable private final Artifact linkDynamicLibraryTool;
private final ImmutableList<PathFragment> builtInIncludeDirectories;
@Nullable private final PathFragment sysroot;
Expand Down Expand Up @@ -153,6 +156,7 @@ public CcToolchainProvider(
boolean supportsHeaderParsing,
CcToolchainVariables buildVariables,
ImmutableList<Artifact> builtinIncludeFiles,
NestedSet<Pair<String, String>> coverageEnvironment,
Artifact linkDynamicLibraryTool,
ImmutableList<PathFragment> builtInIncludeDirectories,
@Nullable PathFragment sysroot,
Expand Down Expand Up @@ -190,6 +194,7 @@ public CcToolchainProvider(
this.supportsHeaderParsing = supportsHeaderParsing;
this.buildVariables = buildVariables;
this.builtinIncludeFiles = builtinIncludeFiles;
this.coverageEnvironment = coverageEnvironment;
this.linkDynamicLibraryTool = linkDynamicLibraryTool;
this.builtInIncludeDirectories = builtInIncludeDirectories;
this.sysroot = sysroot;
Expand Down Expand Up @@ -617,6 +622,14 @@ public ImmutableList<Artifact> getBuiltinIncludeFiles() {
return builtinIncludeFiles;
}

/**
* Returns the environment variables that need to be added to tests that collect code
* coverageFiles.
*/
public NestedSet<Pair<String, String>> getCoverageEnvironment() {
return coverageEnvironment;
}

/**
* Returns the tool which should be used for linking dynamic libraries, or in case it's not
* specified by the crosstool this will be @tools_repository/tools/cpp:link_dynamic_library
Expand Down
10 changes: 10 additions & 0 deletions ...azel/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,8 @@ static CcToolchainProvider getCcToolchainProvider(
}
final CcCompilationContext ccCompilationContext = ccCompilationContextBuilder.build();

NestedSetBuilder<Pair<String, String>> coverageEnvironment = NestedSetBuilder.compileOrder();

PathFragment sysroot = calculateSysroot(attributes, toolchainInfo.getDefaultSysroot());

ImmutableList.Builder<PathFragment> builtInIncludeDirectoriesBuilder = ImmutableList.builder();
Expand All @@ -612,6 +614,13 @@ static CcToolchainProvider getCcToolchainProvider(
ImmutableList<PathFragment> builtInIncludeDirectories =
builtInIncludeDirectoriesBuilder.build();

coverageEnvironment.add(
Pair.of(
"COVERAGE_GCOV_PATH", toolchainInfo.getToolPathFragment(Tool.GCOV).getPathString()));
if (cppConfiguration.getFdoInstrument() != null) {
coverageEnvironment.add(Pair.of("FDO_DIR", cppConfiguration.getFdoInstrument()));
}

Artifact prefetchHintsArtifact = getPrefetchHintsArtifact(prefetchHints, ruleContext);

reportInvalidOptions(ruleContext, toolchainInfo);
Expand Down Expand Up @@ -647,6 +656,7 @@ static CcToolchainProvider getCcToolchainProvider(
toolchainInfo.getDefaultSysroot(),
attributes.getAdditionalBuildVariables()),
getBuiltinIncludes(attributes.getLibc()),
coverageEnvironment.build(),
attributes.getLinkDynamicLibraryTool(),
builtInIncludeDirectories,
sysroot,
Expand Down
64 changes: 8 additions & 56 deletions ...elbuild.bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ public String toString() {
private final boolean stripBinaries;
private final CompilationMode compilationMode;
private final boolean collectCodeCoverage;
private final boolean isThisHostConfigurationDoNotUseWillBeRemovedFor129045294;

static CppConfiguration create(CpuTransformer cpuTransformer, BuildOptions options)
throws InvalidConfigurationException {
Expand Down Expand Up @@ -215,8 +214,7 @@ static CppConfiguration create(CpuTransformer cpuTransformer, BuildOptions optio
|| (cppOptions.stripBinaries == StripMode.SOMETIMES
&& compilationMode == CompilationMode.FASTBUILD)),
compilationMode,
commonOptions.collectCodeCoverage,
commonOptions.isHost);
commonOptions.collectCodeCoverage);
}

private CppConfiguration(
Expand All @@ -233,8 +231,7 @@ private CppConfiguration(
CppOptions cppOptions,
boolean stripBinaries,
CompilationMode compilationMode,
boolean collectCodeCoverage,
boolean isHostConfiguration) {
boolean collectCodeCoverage) {
this.transformedCpuFromOptions = transformedCpuFromOptions;
this.desiredCpu = desiredCpu;
this.fdoPath = fdoPath;
Expand All @@ -249,7 +246,6 @@ private CppConfiguration(
this.stripBinaries = stripBinaries;
this.compilationMode = compilationMode;
this.collectCodeCoverage = collectCodeCoverage;
this.isThisHostConfigurationDoNotUseWillBeRemovedFor129045294 = isHostConfiguration;
}

/** Returns the label of the <code>cc_compiler</code> rule for the C++ configuration. */
Expand Down Expand Up @@ -506,64 +502,27 @@ public boolean isStrictSystemIncludes() {
return cppOptions.strictSystemIncludes;
}

String getFdoInstrument() {
if (isThisHostConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO in the host configuration
return null;
}
public String getFdoInstrument() {
return cppOptions.fdoInstrumentForBuild;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
PathFragment getFdoPathUnsafeSinceItCanReturnValueFromWrongConfiguration() {
public PathFragment getFdoPath() {
return fdoPath;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoOptimizeLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
public Label getFdoOptimizeLabel() {
return fdoOptimizeLabel;
}

Label getFdoPrefetchHintsLabel() {
if (isThisHostConfigurationDoNotUseWillBeRemovedFor129045294()) {
// We don't want FDO in the host configuration
return null;
}
return getFdoPrefetchHintsLabelUnsafeSinceItCanReturnValueFromWrongConfiguration();
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoPrefetchHintsLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
public Label getFdoPrefetchHintsLabel() {
return cppOptions.getFdoPrefetchHintsLabel();
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
public Label getFdoProfileLabel() {
return cppOptions.fdoProfileLabel;
}

/**
* @deprecated Unsafe because it returns a value from target configuration even in the host
* configuration.
*/
@Deprecated
Label getXFdoProfileLabelUnsafeSinceItCanReturnValueFromWrongConfiguration() {
public Label getXFdoProfileLabel() {
if (cppOptions.fdoOptimizeForBuild != null
|| cppOptions.fdoInstrumentForBuild != null
|| cppOptions.fdoProfileLabel != null
Expand Down Expand Up @@ -647,11 +606,4 @@ public boolean requireCtxInConfigureFeatures() {
public boolean collectCodeCoverage() {
return collectCodeCoverage;
}

/** @deprecated this is only a temporary workaround, will be removed by b/129045294. */
// TODO(b/129045294): Remove at first opportunity
@Deprecated
boolean isThisHostConfigurationDoNotUseWillBeRemovedFor129045294() {
return isThisHostConfigurationDoNotUseWillBeRemovedFor129045294;
}
}
19 changes: 6 additions & 13 deletions ...uild.bazel/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,11 +580,8 @@ private FeatureConfiguration getFeatureConfiguration(
// Add a feature identifying the Xcode version so CROSSTOOL authors can enable flags for
// particular versions of Xcode. To ensure consistency across platforms, use exactly two
// components in the version number.
activatedCrosstoolSelectables.add(
XCODE_VERSION_FEATURE_NAME_PREFIX
+ XcodeConfig.getXcodeConfigProvider(ruleContext)
.getXcodeVersion()
.toStringWithComponents(2));
activatedCrosstoolSelectables.add(XCODE_VERSION_FEATURE_NAME_PREFIX
+ XcodeConfig.getXcodeVersion(ruleContext).toStringWithComponents(2));

activatedCrosstoolSelectables.addAll(ruleContext.getFeatures());

Expand Down Expand Up @@ -686,9 +683,7 @@ static Iterable<String> commonFrameworkNames(

ImmutableList.Builder<String> frameworkNames =
new ImmutableList.Builder<String>()
.add(
AppleToolchain.sdkFrameworkDir(
platform, XcodeConfig.getXcodeConfigProvider(ruleContext)));
.add(AppleToolchain.sdkFrameworkDir(platform, ruleContext));
// As of sdk8.1, XCTest is in a base Framework dir.
if (platform.getType() != PlatformType.WATCHOS) { // WatchOS does not have this directory.
frameworkNames.add(AppleToolchain.platformDeveloperFrameworkDir(platform));
Expand Down Expand Up @@ -1833,14 +1828,12 @@ private void registerHeaderScanningAction(
.add("--platform", appleConfiguration.getSingleArchPlatform().getLowerCaseNameInPlist())
.add(
"--sdk_version",
XcodeConfig.getXcodeConfigProvider(ruleContext)
.getSdkVersionForPlatform(appleConfiguration.getSingleArchPlatform())
XcodeConfig.getSdkVersionForPlatform(
ruleContext, appleConfiguration.getSingleArchPlatform())
.toStringWithMinimumComponents(2))
.add(
"--xcode_version",
XcodeConfig.getXcodeConfigProvider(ruleContext)
.getXcodeVersion()
.toStringWithMinimumComponents(2))
XcodeConfig.getXcodeVersion(ruleContext).toStringWithMinimumComponents(2))
.add("--");
for (ObjcHeaderThinningInfo info : infos) {
cmdLine.addFormatted(
Expand Down

0 comments on commit 7dff199

Please sign in to comment.