Add feature to produce indexstore files for CC builds #17984
Conversation
github-actions
bot
added
the
awaiting-review
sgowroji
added
team-Rules-CPP
|
Susan, mind routing this? |
BalestraPatrick
force-pushed
the
indexstore-cc
branch
from
0dce082
to
1d0a4ee
Compare
BalestraPatrick
force-pushed
the
indexstore-cc
branch
from
1d0a4ee
to
d2704da
Compare
|
@susinmotion friendly ping |
|
@googlewalt is this something in your team's purview? |
|
cc @oquenchil |
| @@ -1497,12 +1497,19 @@ private Artifact createCompileActionTemplate( | |||
| CppHelper.getDiagnosticsOutputTreeArtifact( | |||
| actionConstructionContext, label, sourceArtifact, outputName, usePic); | |||
| } | |||
| SpecialArtifact indexstoreTreeArtifact = null; | |||
There was a problem hiding this comment.
I would be against having another SpecialArtifact introduced for cc related stuff. Because this is basically un-Starlarkifiable. @oquenchil WDYT?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
"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.
There was a problem hiding this comment.
I have a pr in apple_support that does adopt how rules_swift did global index store.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| ACTION_NAMES.assemble, | ||
| ACTION_NAMES.preprocess_assemble, |
There was a problem hiding this comment.
@BalestraPatrick
My local clang invocation shows that although clang generates .o files for assemble files (.s and .S), it doesn't generate indexstore files. It just doesn't generate the indexstore files silently. Is assembly file compilation supposed to generate the indexstore file?
Using the
indexstore_filesfeature will add the-index-store-pathflag to C/C++/Objective-C/Objective-C++ compiles, causing a.indexstoretree artifact output to be produced.Users of this feature might also consider passing
-noindex-ignore-system-symbolsin order to greatly reduce the size of the indexstore and ignore system symbols.This is heavily inspired by #15403. I tested this locally in a sample project and it worked fine.
I also prepared a change for the Apple toolchain in apple_support to support this feature: bazelbuild/apple_support@master...BalestraPatrick:apple_support:add-indexstore-files-feature-to-toolchain
This feature is required in order to improve the rules_xcodeproj IDE integration and consume these files for indexing purposes.
Closes #15983.