[ClangImporter] Respect -working-directory in the created VFS #37946
Conversation
|
@swift-ci please test |
|
Is the goal of wrapping the VFS in a |
That was the intention, yes. I started looking back at the previous commits to understand things a bit better. It looks like the way we decided to handle -working-directory was to translate all paths in the driver before passing them to the frontend. Then we pass The inconsistency of using the option vs the filesystem should probably be fixed in clang at some point, though it is "correct" since the filesystem also uses the option now (just somewhat fragile/confusing). On the swift side we could pass -working-directory to the frontend as well and use it to set its FileSystem's CWD (+ using it wherever else There's the also the question of what we should do if the working directories for swift/clang don't match up (ie. |
There was a problem hiding this comment.
Thanks for digging up that context! I think ideally we would pass down -working-directory to the frontend and set it on the VFS, and make it an error to pass a different -Xcc working directory on the CLI. That being said, based on the additional context and looking around more at the implementation, I think your current approach is more conservative and probably less risky if you want to pull it into 5.5. In particular, using the wrapped filesystem will also prevent clang from modifying the CWD of the whole process, which would be bad for SourceKit.
I think before we want to set the working directory across the frontend we need to replace all uses of getRealFileSystem() in our code with createPhysicalFileSystem(), which avoids modifying the process-wide CWD.
lib/ClangImporter/ClangImporter.cpp
Outdated
| @@ -1016,12 +1018,12 @@ ClangImporter::createClangInvocation(ClangImporter *importer, | |||
| // to missing files and report the error that clang would throw manually. | |||
| // rdar://77516546 is tracking that the clang importer should be more | |||
| // resilient and provide a module even if there were building it. | |||
| auto VFS = clang::createVFSFromCompilerInvocation( | |||
| auto TempVFS = clang::createVFSFromCompilerInvocation( | |||
| *CI, *tempClangDiags, | |||
| importer->Impl.SwiftContext.SourceMgr.getFileSystem()); | |||
There was a problem hiding this comment.
Shouldn't we pass in VFS here?
There was a problem hiding this comment.
Ah yes, indeed it should. Thanks, nice catch.
bnbarham
force-pushed
the
importer-use-working-dir
branch
from
3bc277f to
0be0bb7
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
bnbarham
force-pushed
the
importer-use-working-dir
branch
from
0be0bb7 to
c54bf85
Compare
|
Ah, only looked at the macOS tests before restarting - the Linux ones failed because of the assert in ClangModuleDependencyScanner.cpp - the temporary @swift-ci please test |
Pass a wrapped VFS down into `clang::createInvocationFromCommandLine` so that the working directory is set and then used in the underlying Clang `CompilerInstance`. Fixes the possibility of differing modules hashes when the same arguments are used in Clang directly vs from the importer. Resolves rdar://79376364
bnbarham
force-pushed
the
importer-use-working-dir
branch
from
c54bf85 to
28bb250
Compare
|
And there were some Windows failures - some @swift-ci please test |
|
Build failed |
|
Unrelated failure. @swift-ci please test Linux platform |
|
Build failed |
|
And another... @swift-ci please test Linux platform |
Pass a wrapped VFS down into
clang::createInvocationFromCommandLinesothat the working directory is set and then used in the underlying Clang
CompilerInstance.Fixes the possibility of differing modules hashes when the same
arguments are used in Clang directly vs from the importer.
Resolves rdar://79376364