[IncludeTree] Fix PPCallback for include tree that can cause missing .d file #8624
Conversation
|
@swift-ci please test llvm |
| // the same behavior as non-include-tree build where the FileEntry is for | ||
| // the header file. | ||
| Module *M = Imported; | ||
| InclusionCallback(M->getASTFile(), Imported); |
There was a problem hiding this comment.
This will behave correctly for the dependency file generator, but I'm concerned that other PPCallbacks do actually look at the file entry.
Some options
- Accept the weirdness for now and make this a FIXME comment
- Change the dependency file generator so it ignores a missing file when building with include tree. I see there is an option
AddMissingHeaderDeps, but not sure if this is exactly what we want or not. - Open the file and pass the correct FileEntryRef -- this file presumably exists in the include tree filesystem. The downside is we would need to record the path in the includetree and open it, which is unfortunate.
I'm leaning toward (1) or (2).
There was a problem hiding this comment.
I think what that parameter means is kind of debatable. Doc says:
/// \param File The actual file that may be included by this inclusion
/// directive.
So technically ASTFile is the file getting "included" here but I don't know if it is a valid assumption till now that the file means "headerfile" and you can rely on this information. From all the code in tree, dependency is the only consumer that actually uses File, and dependency graph is the only one that cares about what File points to.
In this case, the dependency graph (.dot) file will have nodes for .pcm file and that include directive is an edge from source file to pcm file.
There was a problem hiding this comment.
I see several consumers that look at the File, at least in this branch:
- clang-tidy HeaderIncludeCycleCheck
- clangd IncludeStructure::RecordHeaders
- clangd IncludeGraphCollector
- include-cleaner PPRecorder and RecordPragma
- modularize CoverageCheckerCallbacks
- clang ModuleDependencyPPCallbacks
- clang IndexingAction
- libclang CXIndexDataConsumer
There was a problem hiding this comment.
I missed a bunch in clang-tools-extra. I guess that makes (2) a less good option.
….d file For IncludeDirective callback from include-tree, do not callback with null FileEntry as that dependency collector using that to indicate a missing header include, and will not emit dependency file in this case. Unlike a regular callback from Lexer, include tree doesn't resolve headers so it cannot callback with a FileEntry to the header file being included when the include is actually a module. The behavior for the callback has to be changed slightly that it uses the ASTFile FileEntry. rdar://126885995
cachemeifyoucan
force-pushed
the
eng/PR-126885995
branch
from
6e34144 to
c3b718f
Compare
|
Sounds like (1) might actually be the best solution right now. Add FIXME and update test to also generate a |
|
@swift-ci please test llvm |
For IncludeDirective callback from include-tree, do not callback with null FileEntry as that dependency collector using that to indicate a missing header include, and will not emit dependency file in this case.
Unlike a regular callback from Lexer, include tree doesn't resolve headers so it cannot callback with a FileEntry to the header file being included when the include is actually a module. The behavior for the callback has to be changed slightly that it uses the ASTFile FileEntry.
rdar://126885995