[clang][modules] Detect invalidation of SDKSettings.json #10661
Conversation
|
I think the long-term solution is to decouple input files from the |
| SmallString<128> SDKSettingsJSON = Sysroot; | ||
| llvm::sys::path::append(SDKSettingsJSON, "SDKSettings.json"); | ||
| if (auto FE = PP->getFileManager().getOptionalFileRef(SDKSettingsJSON)) | ||
| PP->getSourceManager().createFileID(*FE, SourceLocation(), |
There was a problem hiding this comment.
We could conditionalize this code on FrontendOptions::ForIncludeTreeScan to avoid it when it's unnecessary.
Have you considered moving this inside WriteInputFiles and directly appending an entry to SystemFiles? That would avoid needing to allocate it in the SourceManager.
There was a problem hiding this comment.
We could conditionalize this code on
FrontendOptions::ForIncludeTreeScanto avoid it when it's unnecessary.
I'd argue changing SDKSettings.json should invalidate the module cache even without include tree enabled.
Have you considered moving this inside
WriteInputFilesand directly appending an entry toSystemFiles? That would avoid needing to allocate it in theSourceManager.
I did try that, but we can't mess with SourceManager after calling computeNonAffectingInputFiles(). That's why I'm doing it super early here.
There was a problem hiding this comment.
I'd argue changing SDKSettings.json should invalidate the module cache even without include tree enabled.
In that case should we be doing this change upstream?
I did try that, but we can't mess with SourceManager after calling computeNonAffectingInputFiles().
I meant construct an InputFileEntry directly for this file and not load it in the SourceManager at all. We already know all the boolean flags for it, the only real annoyance is computing ContentHash if ValidateASTInputFilesContent is enabled. When scanning, the file content is cached, but it may not be for a module/pch build without compilation caching.
There was a problem hiding this comment.
In that case should we be doing this change upstream?
I think so, it's just that this patch currently looks like a hack to me - we probably shouldn't register random JSON files with the SourceManager.
Moreover, the INPUT_FILES_BLOCK in the current PCM format is more like a block of "file-based SLocEntries of the SourceManger" (it says whether they are user/system, top level, a module map, etc.).
I'd like the upstream fix would be to be clean: make that block independent of the SourceManager, add an extension point to ASTWriter so callers can extend the input file list, and then downstream use it to pass the files from include tree.
I meant construct an
InputFileEntrydirectly for this file and not load it in theSourceManagerat all. We already know all the boolean flags for it, the only real annoyance is computingContentHashifValidateASTInputFilesContentis enabled. When scanning, the file content is cached, but it may not be for a module/pch build without compilation caching.
Ah, I see. I did try that as well, but didn't like the duplication I had to do around ContentHash. I updated this PR with possible approach and can upstream the function creation to minimize the diff.
There was a problem hiding this comment.
I'd like the upstream fix would be to be clean: make that block independent of the
SourceManager, add an extension point toASTWriterso callers can extend the input file list, and then downstream use it to pass the files from include tree.
I agree this would be better, but I'm not convinced the current approach now that you're no longer adding the json file to the SourceManager is bad enough to block on this.
| @@ -601,7 +601,8 @@ template <typename Container> | |||
| static auto toJSONStrings(llvm::json::OStream &JOS, Container &&Strings) { | |||
| return [&JOS, Strings = std::forward<Container>(Strings)] { | |||
| for (StringRef Str : Strings) | |||
| JOS.value(Str); | |||
| if (!Str.ends_with("SDKSettings.json")) | |||
There was a problem hiding this comment.
We should add a brief comment here (and to the other places you're skipping this output) to explain why we have this downstream change.
There was a problem hiding this comment.
Makes sense. I'll do that before merging.
|
|
||
| void trySetContentHash(Preprocessor &PP, | ||
| std::optional<llvm::MemoryBufferRef> MemBuff) { | ||
| if (!PP.getHeaderSearchInfo() |
There was a problem hiding this comment.
How about making the memory buffer a lambda so we can avoid re-opening the file if ValidateASTInputFilesContent is false?
The input file section in module files only stores files loaded into the
SourceManager. When rebuilding the module cache, include-trees also include other files, like the SDKSettings.json file. If we don't invalidate the module cache when that file changes, the corresponding include-trees won't agree with the primary TU include-tree on the file contents. This PR makes it so that we do track that file in module files, triggering module cache and therefore include-tree rebuilds, which makes the whole system have a consistent view of the file.The is implemented by loading the SDKSettings.json file into
SourceManager. Note that this doesn't handle all other extra files contained in the include-tree, because I don't think this is a reasonable long-term solution, but the SDKSettings.json file is the one immediately causing issues.I intentionally suppress reporting of this new file in tests, so that I don't have go updating ~all of them. This file is getting reported by the scanning C API, so that the build system is given the ability to eventually act on this file being out-of-date.
rdar://149868539