Skip to content

Commit

Permalink
refactor createSourceAction
Browse files Browse the repository at this point in the history 
This patch refactors the C++ compilation actions in `CcCompilationHelper`. It introduces a helper method `createSourceActionHelper` to streamline the creation of source actions and reduces duplicated code related to PIC and non-PIC compilation.

## Changes
1. **Refactored Source Action Creation**:
    - Introduced `createSourceActionHelper` to handle the common logic for creating compile actions.
    - This method consolidates the setup and finalization of compile actions for both PIC and non-PIC variants.
    - The helper method now handles setting up variables, creating temp actions, and registering the compile actions.

2. **Updated `createSourceAction` Method**:
    - The `createSourceAction` method now calls `createSourceActionHelper` for both PIC and non-PIC compile actions.
    - This change simplifies the `createSourceAction` method and removes redundancy.

## Background

I'm working on support C++20 Modules in Bazel, see #19940

While constructing the action graph for compiling C++20 Modules, I noticed that the `createSourceAction` code could be reused. Therefore, I refactored `createSourceAction`. One potentially unusual aspect is the handling of `ArtifactCategory.CPP_MODULE` at the end. Since the logic for handling C++20 Modules differs from that of Clang Modules, this part of the logic was placed outside of `createSourceActionHelper`.

the code of constructing the action graph for compiling C++20 Modules is #22553

Closes #22744.

PiperOrigin-RevId: 719290267
Change-Id: I62cd8826261866ce3bd4225c099a479288a449aa
  • Loading branch information
@PikachuHyA @copybara-github
PikachuHyA authored and copybara-github committed Jan 24, 2025
1 parent 7416b71 commit eb42f91
Showing 1 changed file with 112 additions and 130 deletions.
242 changes: 112 additions & 130 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 @@ -1570,163 +1570,145 @@ private ImmutableList<Artifact> createSourceAction(
// Create PIC compile actions (same as no-PIC, but use -fPIC and
// generate .pic.o, .pic.d, .pic.gcno instead of .o, .d, .gcno.)
if (generatePicAction) {
String picOutputBase =
CppHelper.getArtifactNameForCategory(ccToolchain, ArtifactCategory.PIC_FILE, outputName);
CppCompileActionBuilder picBuilder = copyAsPicBuilder(builder, picOutputBase, outputCategory);
String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, picOutputBase);
Artifact gcnoFile =
enableCoverage && !cppConfiguration.useLLVMCoverageMapFormat()
? CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, gcnoFileName, configuration)
: null;
Artifact dwoFile =
generateDwo && !bitcodeOutput ? getDwoFile(picBuilder.getOutputFile()) : null;
Artifact ltoIndexingFile =
bitcodeOutput ? getLtoIndexingFile(picBuilder.getOutputFile()) : null;

picBuilder.setVariables(
setupCompileBuildVariables(
picBuilder,
CppCompileActionBuilder picBuilder = new CppCompileActionBuilder(builder);
picBuilder.setPicMode(true);
Artifact picOutputFile =
createSourceActionHelper(
sourceLabel,
/* usePic= */ true,
/* needsFdoBuildVariables= */ ccRelativeName != null && addObject,
outputName,
result,
sourceArtifact,
picBuilder,
outputCategory,
cppModuleMap,
addObject,
enableCoverage,
gcnoFile,
generateDwo,
dwoFile,
ltoIndexingFile,
/* additionalBuildVariables= */ ImmutableMap.of()));

result.addTemps(
createTempsActions(
sourceArtifact,
sourceLabel,
outputName,
picBuilder,
bitcodeOutput,
ccRelativeName,
/* usePic= */ true,
ccRelativeName));

picBuilder.setGcnoFile(gcnoFile);
picBuilder.setDwoFile(dwoFile);
picBuilder.setLtoIndexingFile(ltoIndexingFile);

semantics.finalizeCompileActionBuilder(configuration, featureConfiguration, picBuilder);
CppCompileAction picAction = picBuilder.buildOrThrowRuleError(ruleErrorConsumer);
actionConstructionContext.registerAction(picAction);
directOutputs.add(picAction.getPrimaryOutput());
if (addObject) {
result.addPicObjectFile(picAction.getPrimaryOutput());

if (bitcodeOutput) {
result.addLtoBitcodeFile(
picAction.getPrimaryOutput(), ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
}
}
if (dwoFile != null) {
// Exec configuration targets don't produce .dwo files.
result.addPicDwoFile(dwoFile);
}
if (gcnoFile != null) {
result.addPicGcnoFile(gcnoFile);
}
/* additionalBuildVariables= */ ImmutableMap.of());
directOutputs.add(picOutputFile);
if (outputCategory == ArtifactCategory.CPP_MODULE) {
result.addModuleFile(picAction.getPrimaryOutput());
result.addModuleFile(picOutputFile);
}
}

if (generateNoPicAction) {
Artifact noPicOutputFile =
CppHelper.getCompileOutputArtifact(
actionConstructionContext,
label,
CppHelper.getArtifactNameForCategory(ccToolchain, outputCategory, outputName),
configuration);
builder.setOutputs(
actionConstructionContext, ruleErrorConsumer, label, outputCategory, outputName);
String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ccToolchain, ArtifactCategory.COVERAGE_DATA_FILE, outputName);

// Create no-PIC compile actions
Artifact gcnoFile =
enableCoverage && !cppConfiguration.useLLVMCoverageMapFormat()
? CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, gcnoFileName, configuration)
: null;

Artifact noPicDwoFile = generateDwo && !bitcodeOutput ? getDwoFile(noPicOutputFile) : null;
Artifact ltoIndexingFile = bitcodeOutput ? getLtoIndexingFile(builder.getOutputFile()) : null;

builder.setVariables(
setupCompileBuildVariables(
builder,
createSourceActionHelper(
sourceLabel,
/* usePic= */ false,
/* needsFdoBuildVariables= */ ccRelativeName != null,
outputName,
result,
sourceArtifact,
builder,
outputCategory,
cppModuleMap,
addObject,
enableCoverage,
gcnoFile,
generateDwo,
noPicDwoFile,
ltoIndexingFile,
/* additionalBuildVariables= */ ImmutableMap.of()));

result.addTemps(
createTempsActions(
sourceArtifact,
sourceLabel,
outputName,
builder,
bitcodeOutput,
ccRelativeName,
/* usePic= */ false,
ccRelativeName));
/* additionalBuildVariables= */ ImmutableMap.of());
directOutputs.add(noPicOutputFile);
if (outputCategory == ArtifactCategory.CPP_MODULE) {
result.addModuleFile(noPicOutputFile);
}
}
return directOutputs.build();
}

builder.setGcnoFile(gcnoFile);
builder.setDwoFile(noPicDwoFile);
builder.setLtoIndexingFile(ltoIndexingFile);
private Artifact createSourceActionHelper(
Label sourceLabel,
String outputName,
CcCompilationOutputs.Builder result,
Artifact sourceArtifact,
CppCompileActionBuilder builder,
ArtifactCategory outputCategory,
CppModuleMap cppModuleMap,
boolean addObject,
boolean enableCoverage,
boolean generateDwo,
boolean bitcodeOutput,
PathFragment ccRelativeName,
boolean usePic,
ImmutableMap<String, String> additionalBuildVariables)
throws RuleErrorException, EvalException, InterruptedException {
builder.setOutputs(
actionConstructionContext,
ruleErrorConsumer,
label,
outputCategory,
getOutputNameBaseWith(outputName, usePic));
String gcnoFileName =
CppHelper.getArtifactNameForCategory(
ccToolchain,
ArtifactCategory.COVERAGE_DATA_FILE,
getOutputNameBaseWith(outputName, usePic));

Artifact gcnoFile =
enableCoverage && !cppConfiguration.useLLVMCoverageMapFormat()
? CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, gcnoFileName, configuration)
: null;

Artifact dwoFile = generateDwo && !bitcodeOutput ? getDwoFile(builder.getOutputFile()) : null;
Artifact ltoIndexingFile = bitcodeOutput ? getLtoIndexingFile(builder.getOutputFile()) : null;

builder.setVariables(
setupCompileBuildVariables(
builder,
sourceLabel,
usePic,
/* needsFdoBuildVariables= */ ccRelativeName != null && addObject,
cppModuleMap,
enableCoverage,
gcnoFile,
generateDwo,
dwoFile,
ltoIndexingFile,
additionalBuildVariables));

semantics.finalizeCompileActionBuilder(configuration, featureConfiguration, builder);
CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
actionConstructionContext.registerAction(compileAction);
Artifact objectFile = compileAction.getPrimaryOutput();
directOutputs.add(objectFile);
result.addTemps(
createTempsActions(
sourceArtifact, sourceLabel, outputName, builder, usePic, ccRelativeName));

builder.setGcnoFile(gcnoFile);
builder.setDwoFile(dwoFile);
builder.setLtoIndexingFile(ltoIndexingFile);

semantics.finalizeCompileActionBuilder(configuration, featureConfiguration, builder);
CppCompileAction compileAction = builder.buildOrThrowRuleError(ruleErrorConsumer);
actionConstructionContext.registerAction(compileAction);
Artifact objectFile = compileAction.getPrimaryOutput();
if (usePic) {
if (addObject) {
result.addPicObjectFile(objectFile);
}
if (dwoFile != null) {
// Exec configuration targets don't produce .dwo files.
result.addPicDwoFile(dwoFile);
}
if (gcnoFile != null) {
result.addPicGcnoFile(gcnoFile);
}
} else {
if (addObject) {
result.addObjectFile(objectFile);
if (bitcodeOutput) {
result.addLtoBitcodeFile(
objectFile, ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
}
}
if (noPicDwoFile != null) {
if (dwoFile != null) {
// Exec configuration targets don't produce .dwo files.
result.addDwoFile(noPicDwoFile);
result.addDwoFile(dwoFile);
}
if (gcnoFile != null) {
result.addGcnoFile(gcnoFile);
}
if (outputCategory == ArtifactCategory.CPP_MODULE) {
result.addModuleFile(compileAction.getPrimaryOutput());
}
}
return directOutputs.build();
}

/**
* Creates cpp PIC compile action builder from the given builder by adding necessary copt and
* changing output and dotd file names.
*/
private CppCompileActionBuilder copyAsPicBuilder(
CppCompileActionBuilder builder, String outputName, ArtifactCategory outputCategory)
throws RuleErrorException {
CppCompileActionBuilder picBuilder = new CppCompileActionBuilder(builder);
picBuilder
.setPicMode(true)
.setOutputs(
actionConstructionContext, ruleErrorConsumer, label, outputCategory, outputName);

return picBuilder;
if (addObject && bitcodeOutput) {
result.addLtoBitcodeFile(objectFile, ltoIndexingFile, getCopts(sourceArtifact, sourceLabel));
}
return objectFile;
}

String getOutputNameBaseWith(String base, boolean usePic) throws RuleErrorException {
Expand Down

0 comments on commit eb42f91

Please sign in to comment.