Fix direct clang cc1 emit-pcm commands with vfs overlay on Windows #85325
Conversation
Explicit module builds currently fail on Windows because direct-clang-cc1-module-build emit-pcm commands take overlaid system module map files as inputs but miss the clang VFS overlay. This change adds the overlay and fixes explicit module builds on Windows.
|
@swift-ci please test |
|
nice, I expect this might let us remove some of the Windows skips in swiftlang/swift-build#861, I was looking at some failures there due to a missing VFS post-scanning |
hjyamauchi
requested review from
artemcm,
cachemeifyoucan and
tshortli
as code owners
| CDP->startDiagnosticCapture(); | ||
| } | ||
|
|
||
| void CompilerInstance::setUpVFSForDirectClangCC1EmitPCM() { |
There was a problem hiding this comment.
Why is this not in setUpVirtualFileSystemOverlays?
There was a problem hiding this comment.
The reason of the location of the setUpVFSForDirectClangCC1EmitPCM call was because calling applyClangInvocationMapping would require the ASTContext (after setUpASTContextIfNeeded) but needs to before inputs are read (before setUpInputs()). If ASTContext could be set up before setUpVirtualFileSystemOverlays (which I haven't attempted), setUpVFSForDirectClangCC1EmitPCM could have been in setUpVirtualFileSystemOverlays.
There was a problem hiding this comment.
I think applyCalngInvocationMapping can probably get away with Invocation + DiagnosticEngine + an allocator instead of ASTContext.
| SourceMgr.getFileSystem(); | ||
| ClangInvocationFileMapping FileMapping = applyClangInvocationMapping( | ||
| *Context, nullptr, VFS, /*suppressDiagnostic=*/false); | ||
| if (!FileMapping.redirectedFiles.empty()) { |
There was a problem hiding this comment.
I don't think this is correct for caching. Caching build always uses direct clang cc1 mode but the input is include-tree, which doesn't have any notion of VFS anymore.
There was a problem hiding this comment.
I understand generally that in the caching build, inputs are all loaded into CAS (include-trees) by the depscan command and then read from include-trees only by the subsequent build commands. But I am not sure how that maps into the code here or I'm under-informed about the CAS case as I have started looking into (swift) CAS builds yet.
For a caching build, I imagine, direct clang cc1 mode commands receive an an include tree via flags and setUpVirtualFileSystemOverlays will set that up so input module map file (such as vcruntime.modulemap) will be read from the include tree?
For non-caching builds, we'd need something that enables VFS. so perhaps we need to move setUpVFSForDirectClangCC1EmitPCM to setUpVirtualFileSystemOverlays and only conditionally run for non caching builds? But that doesn't match with the following comment "swift-frontend should not touch any inputs and apply any clang argument transformation when compiling".
There was a problem hiding this comment.
Sounds correct.
IncludeTree already contains everything needed inside the CAS, including files, module maps, and all the VFS overlay has been flattened into lookup content so never pass a VFS to include tree build.
| // RUN: %empty-directory(%t) | ||
|
|
||
| // Test that the -direct-clang-cc1-module-build is able to create a module from the VC runtime with an overlaid modulemap file | ||
| // RUN: %swift_frontend_plain -frontend -emit-pcm -module-name SAL -o %t/SAL.pcm -direct-clang-cc1-module-build "%vctoolsinstalldir\include\module.modulemap" -sdk %S/Inputs/WinSDK -Xcc -fsystem-module -Xcc -emit-module -Xcc -isystem -Xcc "%vctoolsinstalldir\include" -Xcc -cc1 -Xcc -fmodules -Xcc -fmodules-cache-path=%t/module-cache |
There was a problem hiding this comment.
This test is very insufficient.
The explicit module build is always a combination of scanner and swift-frontend. There are few different ways to tackle this problem, and I don't think this is the correct way to fix.
The concept of direct cc1 mode is that swift-frontend should not touch any inputs and apply any clang argument transformation when compiling. Everything should be done by dependency scanner. It is dependency scanner's job to construct tasks that contains the correct VFS for swift-frontend to use. I would prefer the fix to be done that way.
For test case, we have to test the combination of scanner and swift-frontend, otherwise putting a bunch of clang cc1 arguments in the tests doesn't really test what we want.
There was a problem hiding this comment.
I agree about the test and the overall point; but, one thing that makes this trickier is that these particular VFS overlays and the files they point to (.modulemaps) are conjured up completely in-memory by the compiler.
For compilation, this is done in ClangImporter::create by calling applyClangInvocationMapping, and the scanner does something similar and calls into applyClangInvocationMapping when setting up the Clang scanner filesystem.
So now the scanner constructs invocations that assume that this virtual filesystem is present, but the CC1 task doesn't assemble the same in-memory filesystem during execution, if I understand this correctly.
Caching seems to make this particularly tricky - I don't think I have any ideas on how to fit this into include trees.
There was a problem hiding this comment.
Include tree abstract that away already. It just needs to be seen by scanner and you don't need to pass it or add that during compilation.
If the in memory overlay is needed for non-cache build, it is fine. The overlay need not be there for caching build and please add a test case for caching (if that works, if not, we can hold that off for now).
There was a problem hiding this comment.
The concept of direct cc1 mode is that swift-frontend should not touch any inputs and apply any clang argument transformation when compiling. Everything should be done by dependency scanner. It is dependency scanner's job to construct tasks that contains the correct VFS for swift-frontend to use. I would prefer the fix to be done that way.
So it sounds like instead of setUpVFSForDirectClangCC1EmitPCM, the vfs overlay somehow needs to be passed to the build (emit-pcm) command probably via flags. I will think about this and come back.
I see that we need the depscan and the build commands in the test.
4 participants
Explicit module builds currently fail on Windows because direct-clang-cc1-module-build emit-pcm commands take overlaid system module map files as inputs but miss the clang VFS overlay. This change adds the overlay and fixes explicit module builds on Windows.