Skip to content
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

Add feature to produce indexstore files for CC builds #17984

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add feature to produce indexstore files for CC builds #17984

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
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/ArtifactCategory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ public enum ArtifactCategory {
DYNAMIC_LIBRARY("lib", ".so", ".dylib", ".dll", ".wasm"),
EXECUTABLE("", "", ".exe", ".wasm"),
INTERFACE_LIBRARY("lib", ".ifso", ".tbd", ".if.lib", ".lib"),
INDEXSTORE_FILE("", ".indexstore"),
PIC_FILE("", ".pic"),
INCLUDED_FILE_LIST("", ".d"),
SERIALIZED_DIAGNOSTICS_FILE("", ".dia"),
Expand Down
16 changes: 14 additions & 2 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 @@ -1468,8 +1468,8 @@ private Artifact createCompileActionTemplate(
SpecialArtifact outputFiles =
CppHelper.getCompileOutputTreeArtifact(
actionConstructionContext, label, sourceArtifact, outputName, usePic);
// Dotd and dia file outputs are specified in the execution phase.
builder.setOutputs(outputFiles, /* dotdFile= */ null, /* diagnosticsFile= */ null);
// Dotd, dia and indexstore file outputs are specified in the execution phase.
builder.setOutputs(outputFiles, /* dotdFile= */ null, /* diagnosticsFile= */ null, /* indexstoreFiles */ null);
builder.setVariables(
setupCompileBuildVariables(
builder,
Expand Down Expand Up @@ -1497,12 +1497,19 @@ private Artifact createCompileActionTemplate(
CppHelper.getDiagnosticsOutputTreeArtifact(
actionConstructionContext, label, sourceArtifact, outputName, usePic);
}
SpecialArtifact indexstoreTreeArtifact = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be against having another SpecialArtifact introduced for cc related stuff. Because this is basically un-Starlarkifiable. @oquenchil WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this should not go in. We shouldn't have any more special casing in native. Someone would need to contribute to move #17237 forward, then this PR should be based on that or some other generic mechanism that doesn't introduce more native code.

No more SpecialArtifacts should be accepted. #17237 and Starlark tree artifacts should provide the necessary APIs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you perhaps know if aspect with shadow actions can support a use case like this? What I mean is, can you copy CcCompileAction in an aspect and add an output to it, that you can collect?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean compiling twice, right? (since we can't have our action be depended on by the original compilation?), where just adding the flag and output only causes a small increase in original compilation time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use outputs from both actions, then yes, you compile/cache twice. If you need the outputs for IDE only, then you’d compile once. But the regular compile the users probably do/need couldn’t reuse that.

Replacing outputs in CcInfo might be possible within an extended rule if shadow actions were supported there.

I’m a bit conflicted with the idea that you need to have a complete set of new rules to support IDEs. Or even special toolchains.

If only you could split compilation actions away from parsing+index generation for IDEs that would be ideal solution. That is because I’m assuming parsing is an order faster and because those actions can then run in parallel. You redo some work, but the data for IDEs is ready much sooner.

Copy link
Contributor

@brentleyjones brentleyjones Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reflected in this PR yet, but @yongjincho92 took the rules_swift implementation of a "global index store" and implemented it for this. That makes it (more) efficient. We also plan on changing it to produce a .tar file instead of a tree artifact, similar to the comment from @DavidGoldman: #15983 (comment) (because that helps with some remote cache performance).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"global index store" link seems like it's using a worker. Using a worker makes much more sense, because if I'm not mistaken, you can use a global cache directory for the entire build, that behaves just like expected by clang. It can even read from it. And they are more lax on the hermeticity/sandboxing constraints.

This PR/implementation is touching C++ action that don't have a worker. Bazel has much higher expectations for actions like this. They are executed in a sandbox. I don't see a relation to a worker with this PR. If you implemented a worker, you wouldn't need to pass anything additional parameters to it. Just set an env variable to the path to that directory, or in the toolchain configuration and parse it from IDEs.

Maybe some details are missing? I don't know if it's currently even possible to insert a worker into C++ toolchain.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a pr in apple_support that does adopt how rules_swift did global index store.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change above, I still observed a huge spike in remote cache fetching, so I implemented another change that tars the generated .indexstore directory here

Copy link
Contributor

@brentleyjones brentleyjones Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So after those changes, we just need the ability in Bazel to declare new outputs (like this PR does), though ideally new arguments as well. The outputs need to be declared in order to be cached properly.

if (builder.indexstoreFilesEnabled()) {
indexstoreTreeArtifact =
CppHelper.getIndexstoreOutputTreeArtifact(
actionConstructionContext, label, sourceArtifact, outputName);
}
CppCompileActionTemplate actionTemplate =
new CppCompileActionTemplate(
sourceArtifact,
outputFiles,
dotdTreeArtifact,
diagnosticsTreeArtifact,
indexstoreTreeArtifact,
builder,
ccToolchain,
outputCategories,
Expand Down Expand Up @@ -1574,6 +1581,10 @@ private CcToolchainVariables setupCompileBuildVariables(
if (builder.getDiagnosticsFile() != null) {
diagnosticsFileExecPath = builder.getDiagnosticsFile().getExecPathString();
}
String indexstoreFilesExecPath = null;
if (builder.getIndexstoreFiles() != null) {
indexstoreFilesExecPath = builder.getIndexstoreFiles().getExecPathString();
}
if (needsFdoBuildVariables && fdoContext.hasArtifacts(cppConfiguration)) {
// This modifies the passed-in builder, which is a surprising side-effect, and makes it unsafe
// to call this method multiple times for the same builder.
Expand Down Expand Up @@ -1650,6 +1661,7 @@ private CcToolchainVariables setupCompileBuildVariables(
getCopts(builder.getSourceFile(), sourceLabel),
dotdFileExecPath,
diagnosticsFileExecPath,
indexstoreFilesExecPath,
usePic,
ccCompilationContext.getExternalIncludeDirs(),
additionalBuildVariables);
Expand Down
1 change: 1 addition & 0 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 @@ -360,6 +360,7 @@ public CcToolchainVariables getCompileBuildVariables(
/* fdoStamp= */ null,
/* dotdFileExecPath= */ null,
/* diagnosticsFileExecPath= */ null,
/* indexstoreFilesExecPath= */ null,
variablesExtensions,
/* additionalBuildVariables= */ ImmutableMap.of(),
/* directModuleMaps= */ ImmutableList.of(),
Expand Down
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CompileBuildVariables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public enum CompileBuildVariables {
DEPENDENCY_FILE("dependency_file"),
/** Variable for the serialized diagnostics file path */
SERIALIZED_DIAGNOSTICS_FILE("serialized_diagnostics_file"),
/** Variable for the indexstore file paths */
INDEXSTORE_FILES("indexstore_files"),
/** Variable for the module file name. */
MODULE_NAME("module_name"),
/**
Expand Down Expand Up @@ -154,6 +156,7 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
String fdoStamp,
String dotdFileExecPath,
String diagnosticsFileExecPath,
String indexstoreFilesExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Expand Down Expand Up @@ -188,6 +191,7 @@ public static CcToolchainVariables setupVariablesOrReportRuleError(
fdoStamp,
dotdFileExecPath,
diagnosticsFileExecPath,
indexstoreFilesExecPath,
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
Expand Down Expand Up @@ -224,6 +228,7 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
String fdoStamp,
String dotdFileExecPath,
String diagnosticsFileExecPath,
String indexstoreFilesExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Expand Down Expand Up @@ -258,6 +263,7 @@ public static CcToolchainVariables setupVariablesOrThrowEvalException(
fdoStamp,
dotdFileExecPath,
diagnosticsFileExecPath,
indexstoreFilesExecPath,
variablesExtensions,
additionalBuildVariables,
directModuleMaps,
Expand Down Expand Up @@ -288,6 +294,7 @@ private static CcToolchainVariables setupVariables(
String fdoStamp,
String dotdFileExecPath,
String diagnosticsFileExecPath,
String indexstoreFilesExecPath,
ImmutableList<VariablesExtension> variablesExtensions,
ImmutableMap<String, String> additionalBuildVariables,
Iterable<Artifact> directModuleMaps,
Expand Down Expand Up @@ -327,6 +334,7 @@ private static CcToolchainVariables setupVariables(
userCompileFlags,
dotdFileExecPath,
diagnosticsFileExecPath,
indexstoreFilesExecPath,
usePic,
ImmutableList.of(),
ImmutableMap.of());
Expand All @@ -347,6 +355,7 @@ public static void setupSpecificVariables(
Iterable<String> userCompileFlags,
String dotdFileExecPath,
String diagnosticsFileExecPath,
String indexstoreFilesExecPath,
boolean usePic,
ImmutableList<PathFragment> externalIncludeDirs,
Map<String, String> additionalBuildVariables) {
Expand All @@ -372,6 +381,12 @@ public static void setupSpecificVariables(
SERIALIZED_DIAGNOSTICS_FILE.getVariableName(), diagnosticsFileExecPath);
}

// Set indexstore_files to enable <object>.indexstore files generation.
if (indexstoreFilesExecPath != null) {
buildVariables.addStringVariable(
INDEXSTORE_FILES.getVariableName(), indexstoreFilesExecPath);
}

if (gcnoFile != null) {
buildVariables.addStringVariable(GCOV_GCNO_FILE.getVariableName(), gcnoFile);
}
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
* @param outputFile the object file that is written as result of the compilation
* @param dotdFile the .d file that is generated as a side-effect of compilation
* @param diagnosticsFile the .dia file that is generated as a side-effect of compilation
* @param indexstoreFiles the .indexstore files that are generated as a side-effect of compilation
* @param gcnoFile the coverage notes that are written in coverage mode, can be null
* @param dwoFile the .dwo output file where debug information is stored for Fission builds (null
* if Fission mode is disabled)
Expand Down Expand Up @@ -252,6 +253,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
Artifact outputFile,
@Nullable Artifact dotdFile,
@Nullable Artifact diagnosticsFile,
@Nullable Artifact indexstoreFiles,
@Nullable Artifact gcnoFile,
@Nullable Artifact dwoFile,
@Nullable Artifact ltoIndexingFile,
Expand All @@ -275,6 +277,7 @@ public class CppCompileAction extends AbstractAction implements IncludeScannable
Preconditions.checkNotNull(outputFile, "outputFile"),
dotdFile,
diagnosticsFile,
indexstoreFiles,
gcnoFile,
dwoFile,
ltoIndexingFile,
Expand Down Expand Up @@ -327,6 +330,7 @@ private static ImmutableSet<Artifact> collectOutputs(
Artifact outputFile,
@Nullable Artifact dotdFile,
@Nullable Artifact diagnosticsFile,
@Nullable Artifact indexstoreFiles,
@Nullable Artifact gcnoFile,
@Nullable Artifact dwoFile,
@Nullable Artifact ltoIndexingFile,
Expand All @@ -343,6 +347,9 @@ private static ImmutableSet<Artifact> collectOutputs(
if (diagnosticsFile != null) {
outputs.add(diagnosticsFile);
}
if (indexstoreFiles != null) {
outputs.add(indexstoreFiles);
}
if (dwoFile != null) {
outputs.add(dwoFile);
}
Expand Down
22 changes: 21 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public class CppCompileActionBuilder {
private Artifact ltoIndexingFile;
private Artifact dotdFile;
private Artifact diagnosticsFile;
private Artifact indexstoreFiles;
private Artifact gcnoFile;
private CcCompilationContext ccCompilationContext = CcCompilationContext.EMPTY;
private final List<String> pluginOpts = new ArrayList<>();
Expand Down Expand Up @@ -320,6 +321,7 @@ public CppCompileAction buildAndVerify() throws UnconfiguredActionConfigExceptio
outputFile,
dotdFile,
diagnosticsFile,
indexstoreFiles,
gcnoFile,
dwoFile,
ltoIndexingFile,
Expand Down Expand Up @@ -493,12 +495,17 @@ public boolean serializedDiagnosticsFilesEnabled() {
return featureConfiguration.isEnabled(CppRuleClasses.SERIALIZED_DIAGNOSTICS_FILE);
}

public boolean indexstoreFilesEnabled() {
return featureConfiguration.isEnabled(CppRuleClasses.INDEXSTORE_FILES);
}

@CanIgnoreReturnValue
public CppCompileActionBuilder setOutputs(
Artifact outputFile, Artifact dotdFile, Artifact diagnosticsFile) {
Artifact outputFile, Artifact dotdFile, Artifact diagnosticsFile, Artifact indexstoreFiles) {
this.outputFile = outputFile;
this.dotdFile = dotdFile;
this.diagnosticsFile = diagnosticsFile;
this.indexstoreFiles = indexstoreFiles;
return this;
}

Expand Down Expand Up @@ -533,6 +540,15 @@ public CppCompileActionBuilder setOutputs(
} else {
diagnosticsFile = null;
}
if (indexstoreFilesEnabled()) {
String indexstoreFilesName =
CppHelper.getIndexstoreFilesName(ccToolchain, outputCategory, outputName);
indexstoreFiles =
CppHelper.getCompileOutputArtifact(
actionConstructionContext, label, indexstoreFilesName, configuration);
} else {
indexstoreFiles = null;
}
return this;
}

Expand Down Expand Up @@ -564,6 +580,10 @@ public Artifact getDiagnosticsFile() {
return this.diagnosticsFile;
}

public Artifact getIndexstoreFiles() {
return this.indexstoreFiles;
}

@CanIgnoreReturnValue
public CppCompileActionBuilder setGcnoFile(Artifact gcnoFile) {
this.gcnoFile = gcnoFile;
Expand Down
19 changes: 18 additions & 1 deletion src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public final class CppCompileActionTemplate extends ActionKeyCacher
private final SpecialArtifact outputTreeArtifact;
private final SpecialArtifact dotdTreeArtifact;
private final SpecialArtifact diagnosticsTreeArtifact;
private final SpecialArtifact indexstoreTreeArtifact;
private final CcToolchainProvider toolchain;
private final ImmutableList<ArtifactCategory> categories;
private final ActionOwner actionOwner;
Expand All @@ -61,6 +62,7 @@ public final class CppCompileActionTemplate extends ActionKeyCacher
* @param outputTreeArtifact the TreeArtifact that contains compilation outputs.
* @param dotdTreeArtifact the TreeArtifact that contains dotd files.
* @param diagnosticsTreeArtifact the TreeArtifact that contains serialized diagnostics files.
* @param indexstoreTreeArtifact the TreeArtifact that contains indexstore files.
* @param cppCompileActionBuilder An almost completely configured {@link CppCompileActionBuilder}
* without the input and output files set. It is used as a template to instantiate expanded
* {CppCompileAction}s.
Expand All @@ -74,6 +76,7 @@ public final class CppCompileActionTemplate extends ActionKeyCacher
SpecialArtifact outputTreeArtifact,
SpecialArtifact dotdTreeArtifact,
SpecialArtifact diagnosticsTreeArtifact,
SpecialArtifact indexstoreTreeArtifact,
CppCompileActionBuilder cppCompileActionBuilder,
CcToolchainProvider toolchain,
ImmutableList<ArtifactCategory> categories,
Expand All @@ -83,6 +86,7 @@ public final class CppCompileActionTemplate extends ActionKeyCacher
this.outputTreeArtifact = outputTreeArtifact;
this.dotdTreeArtifact = dotdTreeArtifact;
this.diagnosticsTreeArtifact = diagnosticsTreeArtifact;
this.indexstoreTreeArtifact = indexstoreTreeArtifact;
this.toolchain = toolchain;
this.categories = categories;
this.actionOwner = checkNotNull(actionOwner, outputTreeArtifact);
Expand Down Expand Up @@ -146,12 +150,19 @@ public ImmutableList<CppCompileAction> generateActionsForInputArtifacts(
TreeFileArtifact.createTemplateExpansionOutput(
diagnosticsTreeArtifact, outputName + ".dia", artifactOwner);
}
TreeFileArtifact indexstoreFilesArtifact = null;
if (indexstoreTreeArtifact != null) {
indexstoreFilesArtifact =
TreeFileArtifact.createTemplateExpansionOutput(
indexstoreTreeArtifact, outputName + ".indexstore", artifactOwner);
}
expandedActions.add(
createAction(
inputTreeFileArtifact,
outputTreeFileArtifact,
dotdFileArtifact,
diagnosticsFileArtifact,
indexstoreFilesArtifact,
privateHeaders));
}

Expand Down Expand Up @@ -201,13 +212,14 @@ private CppCompileAction createAction(
TreeFileArtifact outputTreeFileArtifact,
@Nullable Artifact dotdFileArtifact,
@Nullable Artifact diagnosticsFileArtifact,
@Nullable Artifact indexstoreFilesArtifact,
NestedSet<Artifact> privateHeaders)
throws ActionExecutionException {
CppCompileActionBuilder builder =
new CppCompileActionBuilder(cppCompileActionBuilder)
.setAdditionalPrunableHeaders(privateHeaders)
.setSourceFile(sourceTreeFileArtifact)
.setOutputs(outputTreeFileArtifact, dotdFileArtifact, diagnosticsFileArtifact);
.setOutputs(outputTreeFileArtifact, dotdFileArtifact, diagnosticsFileArtifact, indexstoreFilesArtifact);

CcToolchainVariables.Builder buildVariables =
CcToolchainVariables.builder(cppCompileActionBuilder.getVariables());
Expand All @@ -227,6 +239,11 @@ private CppCompileAction createAction(
CompileBuildVariables.SERIALIZED_DIAGNOSTICS_FILE.getVariableName(),
diagnosticsFileArtifact.getExecPathString());
}
if (indexstoreFilesArtifact != null) {
buildVariables.overrideStringVariable(
CompileBuildVariables.INDEXSTORE_FILES.getVariableName(),
indexstoreFilesArtifact.getExecPathString());
}

builder.setVariables(buildVariables.build());

Expand Down
37 changes: 37 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public class CppHelper {
static final PathFragment PIC_DOTD_FILES = PathFragment.create("_pic_dotd");
static final PathFragment DIA_FILES = PathFragment.create("_dia");
static final PathFragment PIC_DIA_FILES = PathFragment.create("_pic_dia");
static final PathFragment INDEXSTORE_FILES = PathFragment.create("_indexstore");

// TODO(bazel-team): should this use Link.SHARED_LIBRARY_FILETYPES?
public static final FileTypeSet SHARED_LIBRARY_FILETYPES =
Expand Down Expand Up @@ -307,6 +308,13 @@ private static PathFragment getDiagnosticsDirectory(
ruleLabel, usePic ? PIC_DIA_FILES : DIA_FILES, siblingRepositoryLayout);
}

/** Returns the directory where indexstore files are created. */
private static PathFragment getIndexstoreDirectory(
Label ruleLabel, boolean siblingRepositoryLayout) {
return AnalysisUtils.getUniqueDirectory(
ruleLabel, INDEXSTORE_FILES, siblingRepositoryLayout);
}

public static Artifact getLinkedArtifact(
Label label,
ActionConstructionContext actionConstructionContext,
Expand Down Expand Up @@ -583,6 +591,23 @@ public static SpecialArtifact getDiagnosticsOutputTreeArtifact(
sourceTreeArtifact.getRoot());
}

/**
* Returns the corresponding indexstore files TreeArtifact given the source
* TreeArtifact.
*/
public static SpecialArtifact getIndexstoreOutputTreeArtifact(
ActionConstructionContext actionConstructionContext,
Label label,
Artifact sourceTreeArtifact,
String outputName) {
return actionConstructionContext.getTreeArtifact(
getIndexstoreDirectory(
label,
actionConstructionContext.getConfiguration().isSiblingRepositoryLayout())
.getRelative(outputName),
sourceTreeArtifact.getRoot());
}

public static String getArtifactNameForCategory(
CcToolchainProvider toolchain,
ArtifactCategory category,
Expand Down Expand Up @@ -618,6 +643,18 @@ static String getDiagnosticsFileName(
toolchain, ArtifactCategory.SERIALIZED_DIAGNOSTICS_FILE, baseName);
}

static String getIndexstoreFilesName(
CcToolchainProvider toolchain, ArtifactCategory outputCategory, String outputName)
throws RuleErrorException {
String baseName =
outputCategory == ArtifactCategory.OBJECT_FILE
|| outputCategory == ArtifactCategory.PROCESSED_HEADER
? outputName
: getArtifactNameForCategory(toolchain, outputCategory, outputName);
return getArtifactNameForCategory(
toolchain, ArtifactCategory.INDEXSTORE_FILE, baseName);
}

/**
* Returns true if the build implied by the given config and toolchain uses --start-lib/--end-lib
* ld options.
Expand Down
Loading