Kale added inline comments.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:171-172
FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
ScanInstance.setFileManager(FileMgr);
ScanInstance.createSourceManager(*FileMgr);
----------------
dexonsmith wrote:
> Do these need to be moved lower, after the FileMgr/VFS might change?
Seems `visitPrebuiltModule` in line 179 uses the FileManager set by
`ScanInstance.setFileManager(FileMgr);`, perhaps it's better to keep it here,
and redo this after `createVFSFromCompilerInvocation`.
================
Comment at:
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:202-203
// filesystem.
- FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
+ ToolFileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
----------------
dexonsmith wrote:
> I don't see how calling this on `ToolFileMgr` threads through to updating the
> `FileMgr` in use, which has already been sent to `ScanInstance`.
>
> Note that `createVFSFromCompilerInvocation()` *always* returns a new VFS,
> even if it's equivalent to the one used before (same DepFS options and same
> `-ivfsoverlay`s from `ScanInstance.getInvocation()`). Threading it through
> would mean that `FileMgr` is never reused.
>
> IMO, never-reuse-the-filemanager is the right/correct behaviour for
> clang-scan-deps, since DepFS provides caching for things that can be cached
> soundly, but probably better to explicitly stop reusing the FileMgr (in a
> separate patch ahead of time) rather than leaving code that looks like it
> might get reused.
That makes sense. Will prepend a patch to explicitly create a new FileManager
if DepFS is used.
================
Comment at: clang/lib/Tooling/Tooling.cpp:447-448
appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
if (Files)
Files->setVirtualFileSystem(OverlayFileSystem);
}
----------------
dexonsmith wrote:
> This will *never* reuse the file manager, since it's *always* a new VFS.
> - Maybe that's correct, since we want to inject different things into
> InMemoryFS?
> - Or, maybe that's overly pessimistic, and we should pass BaseFS into the
> filemanager if `appendArgumentsAdjuster()` never added anything to InMemoryFS?
Use BaseFS as the condition to judge whether to invalidate previous file
managers? Sounds worth a try. Will amend later.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124816/new/
https://reviews.llvm.org/D124816
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
