Skip to content

Commit

Permalink
Merge CppDebugFileProvider with CcInfo
Browse files Browse the repository at this point in the history
.dwo files were previously propagated in a separate provider, and this provider cannot be accessed from Starlark. We had 2 options:

* expose the provider in Starlark
* merge it with CcInfo

We went with unification with CcInfo, as it simplifies Starlark rules, and CppDebugFileProvider is not used by other languages anyway.

While at it, I realized we don't need the DwoArtifactsCollector to collect both pic and nopic dwos just for cc binary to then throw one of them away. Thus I removed the collector. And I changed unit tests to access dwo through action inputs, not by calling the same methods as the production code calls.

RELNOTES: None.
PiperOrigin-RevId: 257601965
  • Loading branch information
hlopko authored and copybara-github committed Jul 11, 2019
1 parent 80a63d7 commit ece92fb
Show file tree
Hide file tree
Showing 13 changed files with 193 additions and 327 deletions.
93 changes: 37 additions & 56 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
Original file line number Diff line number Diff line change
Expand Up @@ -540,18 +540,18 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
CppHelper.createStripAction(
ruleContext, ccToolchain, cppConfiguration, binary, strippedFile, featureConfiguration);

DwoArtifactsCollector dwoArtifacts =
NestedSet<Artifact> dwoFiles =
collectTransitiveDwoArtifacts(
ruleContext,
ccCompilationOutputs,
CppHelper.mergeCcDebugInfoContexts(
compilationInfo.getCcCompilationOutputs(),
AnalysisUtils.getProviders(deps, CcInfo.PROVIDER)),
linkingMode,
ccToolchain.shouldCreatePerObjectDebugInfo(featureConfiguration, cppConfiguration),
usePic,
ccLinkingOutputsBinary.getAllLtoArtifacts());
Artifact dwpFile =
ruleContext.getImplicitOutputArtifact(CppRuleClasses.CC_BINARY_DEBUG_PACKAGE);
createDebugPackagerActions(
ruleContext, ccToolchain, cppConfiguration, featureConfiguration, dwpFile, dwoArtifacts);
createDebugPackagerActions(ruleContext, ccToolchain, dwpFile, dwoFiles);

// The debug package should include the dwp file only if it was explicitly requested.
Artifact explicitDwpFile = dwpFile;
Expand Down Expand Up @@ -619,7 +619,6 @@ public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleCont
ccCompilationOutputs,
ccCompilationContext,
libraries,
dwoArtifacts,
fake);

// Support test execution on darwin.
Expand Down Expand Up @@ -859,33 +858,31 @@ private static final LinkingMode getLinkStaticness(
* dynamic linking, dependencies are separately linked into their own shared libraries, so we
* don't need them here.
*/
private static DwoArtifactsCollector collectTransitiveDwoArtifacts(
RuleContext context,
private static NestedSet<Artifact> collectTransitiveDwoArtifacts(
CcCompilationOutputs compilationOutputs,
Link.LinkingMode linkingMode,
boolean generateDwo,
boolean ltoBackendArtifactsUsePic,
CcDebugInfoContext ccDebugInfoContext,
LinkingMode linkingMode,
boolean usePic,
Iterable<LtoBackendArtifacts> ltoBackendArtifacts) {
if (linkingMode == LinkingMode.DYNAMIC) {
return DwoArtifactsCollector.directCollector(
compilationOutputs, generateDwo, ltoBackendArtifactsUsePic, ltoBackendArtifacts);
} else {
return CcCommon.collectTransitiveDwoArtifacts(
context, compilationOutputs, generateDwo, ltoBackendArtifactsUsePic, ltoBackendArtifacts);
NestedSetBuilder<Artifact> dwoFiles = NestedSetBuilder.stableOrder();
dwoFiles.addAll(
usePic ? compilationOutputs.getPicDwoFiles() : compilationOutputs.getDwoFiles());

if (ltoBackendArtifacts != null) {
for (LtoBackendArtifacts ltoBackendArtifact : ltoBackendArtifacts) {
if (ltoBackendArtifact.getDwoFile() != null) {
dwoFiles.add(ltoBackendArtifact.getDwoFile());
}
}
}
}

@VisibleForTesting
public static Iterable<Artifact> getDwpInputs(
RuleContext context,
CcToolchainProvider toolchain,
CppConfiguration cppConfiguration,
FeatureConfiguration featureConfiguration,
NestedSet<Artifact> picDwoArtifacts,
NestedSet<Artifact> dwoArtifacts) {
return usePic(context, toolchain, cppConfiguration, featureConfiguration)
? picDwoArtifacts
: dwoArtifacts;
if (linkingMode != LinkingMode.DYNAMIC) {
dwoFiles.addTransitive(
usePic
? ccDebugInfoContext.getTransitivePicDwoFiles()
: ccDebugInfoContext.getTransitiveDwoFiles());
}
return dwoFiles.build();
}

/**
Expand All @@ -894,27 +891,16 @@ public static Iterable<Artifact> getDwpInputs(
private static void createDebugPackagerActions(
RuleContext context,
CcToolchainProvider toolchain,
CppConfiguration cppConfiguration,
FeatureConfiguration featureConfiguration,
Artifact dwpOutput,
DwoArtifactsCollector dwoArtifactsCollector)
NestedSet<Artifact> dwoFiles)
throws RuleErrorException {
Iterable<Artifact> allInputs =
getDwpInputs(
context,
toolchain,
cppConfiguration,
featureConfiguration,
dwoArtifactsCollector.getPicDwoArtifacts(),
dwoArtifactsCollector.getDwoArtifacts());

// No inputs? Just generate a trivially empty .dwp.
//
// Note this condition automatically triggers for any build where fission is disabled.
// Because rules referencing .dwp targets may be invoked with or without fission, we need
// to support .dwp generation even when fission is disabled. Since no actual functionality
// is expected then, an empty file is appropriate.
if (Iterables.isEmpty(allInputs)) {
if (Iterables.isEmpty(dwoFiles)) {
context.registerAction(FileWriteAction.create(context, dwpOutput, "", false));
return;
}
Expand All @@ -938,7 +924,7 @@ private static void createDebugPackagerActions(
// at the leaves than the root, but that both increases parallelism and reduces the final
// action's input size.
Packager packager =
createIntermediateDwpPackagers(context, dwpOutput, toolchain, dwpFiles, allInputs, 1);
createIntermediateDwpPackagers(context, dwpOutput, toolchain, dwpFiles, dwoFiles, 1);
packager.spawnAction.setMnemonic("CcGenerateDwp").addOutput(dwpOutput);
packager.commandLine.addExecPath("-o", dwpOutput);
context.registerAction(packager.build(context));
Expand All @@ -963,25 +949,25 @@ private static Packager createIntermediateDwpPackagers(
RuleContext context,
Artifact dwpOutput,
CcToolchainProvider toolchain,
NestedSet<Artifact> dwpTools,
Iterable<Artifact> inputs,
NestedSet<Artifact> dwpFiles,
Iterable<Artifact> dwoFiles,
int intermediateDwpCount)
throws RuleErrorException {
List<Packager> packagers = new ArrayList<>();

// Step 1: generate our batches. We currently break into arbitrary batches of fixed maximum
// input counts, but we can always apply more intelligent heuristics if the need arises.
Packager currentPackager = newDwpAction(context, toolchain, dwpTools);
Packager currentPackager = newDwpAction(context, toolchain, dwpFiles);
int inputsForCurrentPackager = 0;

for (Artifact dwoInput : inputs) {
for (Artifact dwoFile : dwoFiles) {
if (inputsForCurrentPackager == MAX_INPUTS_PER_DWP_ACTION) {
packagers.add(currentPackager);
currentPackager = newDwpAction(context, toolchain, dwpTools);
currentPackager = newDwpAction(context, toolchain, dwpFiles);
inputsForCurrentPackager = 0;
}
currentPackager.spawnAction.addInput(dwoInput);
currentPackager.commandLine.addExecPath(dwoInput);
currentPackager.spawnAction.addInput(dwoFile);
currentPackager.commandLine.addExecPath(dwoFile);
inputsForCurrentPackager++;
}
packagers.add(currentPackager);
Expand All @@ -1000,7 +986,7 @@ private static Packager createIntermediateDwpPackagers(
intermediateOutputs.add(intermediateOutput);
}
return createIntermediateDwpPackagers(
context, dwpOutput, toolchain, dwpTools, intermediateOutputs, intermediateDwpCount);
context, dwpOutput, toolchain, dwpFiles, intermediateOutputs, intermediateDwpCount);
}
return Iterables.getOnlyElement(packagers);
}
Expand Down Expand Up @@ -1084,7 +1070,6 @@ private static void addTransitiveInfoProviders(
CcCompilationOutputs ccCompilationOutputs,
CcCompilationContext ccCompilationContext,
List<LibraryToLink> libraries,
DwoArtifactsCollector dwoArtifacts,
boolean fake)
throws RuleErrorException {
List<Artifact> instrumentedObjectFiles = new ArrayList<>();
Expand Down Expand Up @@ -1117,10 +1102,6 @@ private static void addTransitiveInfoProviders(
CcNativeLibraryProvider.class,
new CcNativeLibraryProvider(collectTransitiveCcNativeLibraries(ruleContext, libraries)))
.addNativeDeclaredProvider(instrumentedFilesProvider)
.addProvider(
CppDebugFileProvider.class,
new CppDebugFileProvider(
dwoArtifacts.getDwoArtifacts(), dwoArtifacts.getPicDwoArtifacts()))
// For CcBinary targets, we only want to ensure that we process headers in dependencies and
// thus only add header tokens to HIDDEN_TOP_LEVEL. If we add all HIDDEN_TOP_LEVEL artifacts
// from dependent CcLibrary targets, we'd be building .pic.o files in nopic builds.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,30 +276,6 @@ private boolean hasAttribute(String name, Type<?> type) {
return ruleContext.attributes().has(name, type);
}

/** Collects all .dwo artifacts in this target's transitive closure. */
public static DwoArtifactsCollector collectTransitiveDwoArtifacts(
RuleContext ruleContext,
CcCompilationOutputs compilationOutputs,
boolean generateDwo,
boolean ltoBackendArtifactsUsePic,
Iterable<LtoBackendArtifacts> ltoBackendArtifacts) {
ImmutableList.Builder<TransitiveInfoCollection> deps =
ImmutableList.<TransitiveInfoCollection>builder();

deps.addAll(ruleContext.getPrerequisites("deps", Mode.TARGET));

if (ruleContext.attributes().has("malloc", BuildType.LABEL)) {
deps.add(CppHelper.mallocForTarget(ruleContext));
}

return DwoArtifactsCollector.transitiveCollector(
compilationOutputs,
deps.build(),
generateDwo,
ltoBackendArtifactsUsePic,
ltoBackendArtifacts);
}

/**
* Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input source
* file and the label of the rule that generates it (or the label of the source file itself if it
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,6 @@ private Builder(
ActionConstructionContext actionConstructionContext,
BuildConfiguration configuration,
Label label) {
// private to avoid class initialization deadlock between this class and its outer class
this.actionConstructionContext = actionConstructionContext;
this.configuration = configuration;
this.label = label;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -709,20 +709,6 @@ public static Map<String, NestedSet<Artifact>> buildOutputGroups(
return outputGroups;
}

public static CppDebugFileProvider buildCppDebugFileProvider(
CcCompilationOutputs ccCompilationOutputs, Iterable<TransitiveInfoCollection> deps) {
DwoArtifactsCollector dwoArtifacts =
DwoArtifactsCollector.transitiveCollector(
ccCompilationOutputs,
deps,
/*generateDwo=*/ false,
/*ltoBackendArtifactsUsePic=*/ false,
/*ltoBackendArtifacts=*/ ImmutableList.of());
CppDebugFileProvider cppDebugFileProvider =
new CppDebugFileProvider(dwoArtifacts.getDwoArtifacts(), dwoArtifacts.getPicDwoArtifacts());
return cppDebugFileProvider;
}

public static Map<String, NestedSet<Artifact>> buildOutputGroupsForEmittingCompileProviders(
CcCompilationOutputs ccCompilationOutputs,
CcCompilationContext ccCompilationContext,
Expand Down Expand Up @@ -1259,8 +1245,7 @@ private CcCompilationOutputs createCcCompileActions() throws RuleErrorException
}
}

ImmutableMap<Artifact, String> outputNameMap = null;

ImmutableMap<Artifact, String> outputNameMap;
String outputNamePrefixDir = null;
// purpose is only used by objc rules, it ends with either "_non_objc_arc" or "_objc_arc".
// Here we use it to distinguish arc and non-arc compilation.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Copyright 2014 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.devtools.build.lib.rules.cpp;

import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import java.util.Objects;

/**
* A struct that stores .dwo files which can be combined into a .dwp in the packaging step. See
* https://gcc.gnu.org/wiki/DebugFission for details.
*/
@Immutable
public final class CcDebugInfoContext {

public static final CcDebugInfoContext EMPTY =
new CcDebugInfoContext(
/* transitiveDwoFiles= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* transitivePicDwoFiles= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER));
private final NestedSet<Artifact> transitiveDwoFiles;
private final NestedSet<Artifact> transitivePicDwoFiles;

public CcDebugInfoContext(
NestedSet<Artifact> transitiveDwoFiles, NestedSet<Artifact> transitivePicDwoFiles) {
this.transitiveDwoFiles = transitiveDwoFiles;
this.transitivePicDwoFiles = transitivePicDwoFiles;
}

/** Merge multiple {@link CcDebugInfoContext}s into one. */
public static CcDebugInfoContext merge(ImmutableList<CcDebugInfoContext> contexts) {
NestedSetBuilder<Artifact> transitiveDwoFiles = NestedSetBuilder.stableOrder();
NestedSetBuilder<Artifact> transitivePicDwoFiles = NestedSetBuilder.stableOrder();

for (CcDebugInfoContext context : contexts) {
transitiveDwoFiles.addTransitive(context.getTransitiveDwoFiles());
transitivePicDwoFiles.addTransitive(context.getTransitivePicDwoFiles());
}

return new CcDebugInfoContext(transitiveDwoFiles.build(), transitivePicDwoFiles.build());
}

public static CcDebugInfoContext from(CcCompilationOutputs outputs) {
return new CcDebugInfoContext(
NestedSetBuilder.wrap(Order.STABLE_ORDER, outputs.getDwoFiles()),
NestedSetBuilder.wrap(Order.STABLE_ORDER, outputs.getPicDwoFiles()));
}

/**
* Returns the .dwo files that should be included in this target's .dwp packaging (if this
* target is linked) or passed through to a dependant's .dwp packaging (e.g. if this is a
* cc_library depended on by a statically linked cc_binary).
*
* Assumes the corresponding link consumes .o files (vs. .pic.o files).
*/
public NestedSet<Artifact> getTransitiveDwoFiles() {
return transitiveDwoFiles;
}

/**
* Same as above, but assumes the corresponding link consumes pic.o files.
*/
public NestedSet<Artifact> getTransitivePicDwoFiles() {
return transitivePicDwoFiles;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
CcDebugInfoContext that = (CcDebugInfoContext) o;
return Objects.equals(transitiveDwoFiles, that.transitiveDwoFiles)
&& Objects.equals(transitivePicDwoFiles, that.transitivePicDwoFiles);
}

@Override
public int hashCode() {
return Objects.hash(transitiveDwoFiles, transitivePicDwoFiles);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.rules.cpp;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
Expand Down Expand Up @@ -167,18 +166,16 @@ public ConfiguredTarget create(RuleContext ruleContext)
.setCodeCoverageEnabled(CcCompilationHelper.isCodeCoverageEnabled(ruleContext))
.compile();

CppDebugFileProvider cppDebugFileProvider =
CcCompilationHelper.buildCppDebugFileProvider(
compilationInfo.getCcCompilationOutputs(), ImmutableList.of());
Map<String, NestedSet<Artifact>> outputGroups =
CcCompilationHelper.buildOutputGroups(compilationInfo.getCcCompilationOutputs());
RuleConfiguredTargetBuilder result =
new RuleConfiguredTargetBuilder(ruleContext)
.addProvider(cppDebugFileProvider)
.addNativeDeclaredProvider(
CcInfo.builder()
.setCcCompilationContext(compilationInfo.getCcCompilationContext())
.setCcLinkingContext(ccLinkingContext)
.setCcDebugInfoContext(
CcDebugInfoContext.from(compilationInfo.getCcCompilationOutputs()))
.build())
.addOutputGroups(outputGroups)
.addProvider(RunfilesProvider.class, RunfilesProvider.simple(Runfiles.EMPTY));
Expand Down
Loading

0 comments on commit ece92fb

Please sign in to comment.