[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 10 11:18:34 PDT 2022
dexonsmith added a comment.
This looks mostly correct to me, but perhaps pessimistic enough it'll regress performance unnecessarily. A few comments inline.
================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:169-170
ScanInstance.getFrontendOpts().UseGlobalModuleIndex = false;
-
+ auto FileMgr = ToolFileMgr->getOrCreateFileManager().get();
FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
ScanInstance.setFileManager(FileMgr);
----------------
Should `WorkingDirectory` be an argument to `getOrCreateFileManager()` here?
================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:171-172
FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
ScanInstance.setFileManager(FileMgr);
ScanInstance.createSourceManager(*FileMgr);
----------------
Do these need to be moved lower, after the FileMgr/VFS might change?
================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:202-203
// filesystem.
- FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
+ ToolFileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
----------------
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.
================
Comment at: clang/lib/Tooling/Tooling.cpp:447-448
appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
if (Files)
Files->setVirtualFileSystem(OverlayFileSystem);
}
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124816/new/
https://reviews.llvm.org/D124816
More information about the cfe-commits
mailing list