-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Propagate vfs overlays and -fbuiltin-headers-in-system-modules #79926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is a new fix to what #78184 intended to fix and the |
@swift-ci please test Windows platform |
@swift-ci please test |
b3c60b5
to
ebbdc71
Compare
@swift-ci please test |
ebbdc71
to
084c0e5
Compare
@swift-ci please test |
@swift-ci please test Linux platform |
084c0e5
to
7c1f248
Compare
@@ -185,6 +193,13 @@ ModuleDependencyVector ClangImporter::bridgeClangModuleDependencies( | |||
swiftArgs.push_back("-vfsoverlay"); | |||
swiftArgs.push_back(remapPath(overlay)); | |||
} | |||
|
|||
// Pass along the SDK path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this is needed? The swift compile PCM job is pretty much only using cc1 arguments and I don't see why a swift argument her is useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how building hello.swift
with -explicit-module-build
fails on Windows without this due to diag::warn_module_config_mismatch
. I think this was caused by a mismatch due to a missing -sdk
flag in the ASTReader
code. With this, this succeeds and hello.swift
builds correctly.
If this doesn't make sense, I can dig more.
>swiftc hello.swift -explicit-module-build -v
"C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\swift-frontend.exe" -frontend -scan-dependencies -target x86_64-unknown-windows-msvc -disable-objc-interop -sdk "S:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk" -color-diagnostics -new-driver-path "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\swift-driver.exe" -empty-abi-descriptor -resource-dir "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\lib\swift" -no-auto-bridging-header-chaining -module-name hello -in-process-plugin-server-path "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\SwiftInProcPluginServer.dll" -plugin-path "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin" -plugin-path "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\local\bin" -scanner-module-validation -no-auto-bridging-header-chaining -scanner-output-dir C:\Users\hiroshi\AppData\Local\Temp\TemporaryDirectory.RYev3J\scanner hello.swift
Swift version 6.2-dev (LLVM aa33e9831968e53, Swift ff6a1e1138782ce)
Target: x86_64-unknown-windows-msvc
"C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\swift-frontend.exe" -frontend -compile-module-from-interface -target x86_64-unknown-windows-msvc -swift-version 5 -sdk "S:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk" -suppress-warnings -disable-objc-attr-requires-foundation-module -enable-ossa-modules -no-clang-include-tree -track-system-dependencies "S:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\windows\SwiftOnoneSupport.swiftmodule\x86_64-unknown-windows-msvc.private.swiftinterface" -module-name SwiftOnoneSupport -disable-objc-attr-requires-foundation-module -target x86_64-unknown-windows-msvc -disable-objc-interop -enable-experimental-feature NoncopyableGenerics2 -enable-experimental-feature SuppressedAssociatedTypes -enable-experimental-feature SE427NoInferenceOnExtension -enable-experimental-feature NonescapableTypes -enable-experimental-feature LifetimeDependence -enable-upcoming-feature MemberImportVisibility -enable-library-evolution -module-link-name swiftSwiftOnoneSupport -parse-stdlib -swift-version 5 -O -library-level api -enforce-exclusivity=unchecked -disable-objc-interop -target-min-inlining-version min -module-name SwiftOnoneSupport -strict-memory-safety -enable-lexical-lifetimes=false -enable-ossa-modules -interface-compiler-version 6.2 -explicit-interface-module-build -disable-implicit-swift-modules -Xcc -fno-implicit-modules -Xcc -fno-implicit-module-maps "-swift-module-file=Swift=S:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\windows\Swift.swiftmodule\x86_64-unknown-windows-msvc.swiftmodule" -Xcc -fmodule-file=SwiftShims=C:\Users\hiroshi\AppData\Local\clang\ModuleCache\SwiftShims-A4U7ZTF76P8Z79XYETHS7YEGN.pcm -Xcc -fmodule-file=_Builtin_stdint=C:\Users\hiroshi\AppData\Local\clang\ModuleCache\_Builtin_stdint-BP8JEOXOE7GZI1QX2C0AR3XAE.pcm -o SwiftOnoneSupport-2A4D0UPZ41A5O.swiftmodule -load-resolved-plugin "C:\Users\hiroshi\source\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\SwiftMacros.dll##SwiftMacros"
error: compile-module-from-interface command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers was disabled in PCH file but is currently enabled
<unknown>:0: error: module file C:\Users\hiroshi\AppData\Local\clang\ModuleCache\SwiftShims-A4U7ZTF76P8Z79XYETHS7YEGN.pcm cannot be loaded due to a configuration mismatch with the current compilation
S:\Program Files\Swift\Platforms\Windows.platform\Developer\SDKs\Windows.sdk\usr\lib\swift\windows\SwiftOnoneSupport.swiftmodule\x86_64-unknown-windows-msvc.private.swiftinterface:5:8: error: missing required module 'SwiftShims'
3 | // swift-module-flags: -disable-objc-attr-requires-foundation-module -target x86_64-unknown-windows-msvc -disable-objc-interop -enable-experimental-feature NoncopyableGenerics2 -enable-experimental-feature SuppressedAssociatedTypes -enable-experimental-feature SE427NoInferenceOnExtension -enable-experimental-feature NonescapableTypes -enable-experimental-feature LifetimeDependence -enable-upcoming-feature MemberImportVisibility -enable-library-evolution -module-link-name swiftSwiftOnoneSupport -parse-stdlib -swift-version 5 -O -library-level api -enforce-exclusivity=unchecked -disable-objc-interop -target-min-inlining-version min -module-name SwiftOnoneSupport
4 | // swift-module-flags-ignorable: -strict-memory-safety -enable-lexical-lifetimes=false -enable-ossa-modules -interface-compiler-version 6.2
5 | import Swift
| `- error: missing required module 'SwiftShims'
6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't really make sense. That doesn't look like the output from the new swift-driver. I don't think building explicit module with legacy driver is really supported and we don't go out of way to make that working.
Can you check which swiftc
you are running? Maybe windows bot is missing the new driver, thus it is not going down the right code path. Check lib/DriverTool/driver.cpp
and make sure it goes down the path to run swift-driver
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting :)
The swiftc that I'm using is S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin\swiftc.exe
which is produced by my local toolchain build with build.ps1
.
The new swift-driver is what's from the swift-driver
repo and is written in Swift, and the legacy driver is the one from the swift
repo and is written in mostly C++ (which I think is the same binary as swift-frontend.exe
below), right? (Note we don't use symlinks on windows)
I think the windows build process in build.ps1
installs the new driver over the legacy driver.
I understand that the tests may have not the new driver. The command line is a manual one, not part of a test.
According to ls
, diff
and dumpbin
, I think the swiftc.exe
is the same as the new driver swift-driver.exe
and swift-frontend.exe
is the legacy driver.
So I think I am using the new driver. But I'm not sure what's different here.
S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin>ls -l swift*.exe
-rwxr-xr-x 1 hiroshi 197121 154601984 Mar 14 15:33 swift-api-digester.exe
-rwxr-xr-x 1 hiroshi 197121 148999168 Mar 14 15:33 swift-ast-script.exe
-rwxr-xr-x 1 hiroshi 197121 154601984 Mar 14 15:33 swift-autolink-extract.exe
-rwxr-xr-x 1 hiroshi 197121 46592 Mar 14 15:25 swift-build-sdk-interfaces.exe
-rwxr-xr-x 1 hiroshi 197121 1731072 Mar 14 15:24 swift-build-tool.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-build.exe
-rwxr-xr-x 1 hiroshi 197121 154601984 Mar 14 15:33 swift-cache-tool.exe
-rwxr-xr-x 1 hiroshi 197121 341504 Mar 14 15:33 swift-compatibility-symbols.exe
-rwxr-xr-x 1 hiroshi 197121 885760 Mar 14 15:33 swift-def-to-strings-converter.exe
-rwxr-xr-x 1 hiroshi 197121 759296 Mar 14 15:33 swift-demangle.exe
-rwxr-xr-x 1 hiroshi 197121 41984 Mar 14 15:35 swift-driver.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-experimental-sdk.exe
-rwxr-xr-x 1 hiroshi 197121 188416 Mar 14 15:29 swift-format.exe
-rwxr-xr-x 1 hiroshi 197121 154608640 Mar 14 15:35 swift-frontend.exe
-rwxr-xr-x 1 hiroshi 197121 49664 Mar 14 15:24 swift-help.exe
-rwxr-xr-x 1 hiroshi 197121 151496192 Mar 14 15:33 swift-ide-test.exe
-rwxr-xr-x 1 hiroshi 197121 194048 Mar 14 15:30 swift-inspect.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-package.exe
-rwxr-xr-x 1 hiroshi 197121 22528 Mar 14 14:45 swift-plugin-server.exe
-rwxr-xr-x 1 hiroshi 197121 5738496 Mar 14 15:33 swift-reflection-dump.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-run.exe
-rwxr-xr-x 1 hiroshi 197121 394752 Mar 14 15:33 swift-scan-test.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-sdk.exe
-rwxr-xr-x 1 hiroshi 197121 747520 Mar 14 15:33 swift-serialize-diagnostics.exe
-rwxr-xr-x 1 hiroshi 197121 154601984 Mar 14 15:33 swift-symbolgraph-extract.exe
-rwxr-xr-x 1 hiroshi 197121 154601984 Mar 14 15:33 swift-synthesize-interface.exe
-rwxr-xr-x 1 hiroshi 197121 23552 Mar 14 15:28 swift-test.exe
-rwxr-xr-x 1 hiroshi 197121 41984 Mar 14 15:35 swift.exe
-rwxr-xr-x 1 hiroshi 197121 41984 Mar 14 15:35 swiftc.exe
S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin>diff swiftc.exe swift-driver.exe
S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin>echo %errorlevel%
0
S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin>dumpbin /dependents swiftc.exe
Microsoft (R) COFF/PE Dumper Version 14.42.34436.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file swiftc.exe
File Type: EXECUTABLE IMAGE
Image has the following dependencies:
mimalloc-redirect.dll
mimalloc.dll
SwiftDriverExecution.dll
SwiftDriver.dll
SwiftOptions.dll
TSCBasic.dll
swiftCore.dll
swiftWinSDK.dll
KERNEL32.dll
swiftCRT.dll
FoundationEssentials.dll
swiftDispatch.dll
BlocksRuntime.dll
VCRUNTIME140.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-locale-l1-1-0.dll
Summary
1000 .data
1000 .pdata
3000 .rdata
2000 .reloc
1000 .sw5acfn
1000 .sw5asty
1000 .sw5bltn
1000 .sw5cptr
1000 .sw5entr
1000 .sw5flmd
1000 .sw5hash
1000 .sw5mpen
1000 .sw5prt
1000 .sw5prtc
1000 .sw5ratt
1000 .sw5repl
1000 .sw5reps
1000 .sw5rfst
1000 .sw5test
1000 .sw5tymd
1000 .sw5tyrf
3000 .text
S:\Program Files\Swift\Toolchains\0.0.0+Asserts\usr\bin>dumpbin /dependents swift-frontend.exe
Microsoft (R) COFF/PE Dumper Version 14.42.34436.0
Copyright (C) Microsoft Corporation. All rights reserved.
Dump of file swift-frontend.exe
File Type: EXECUTABLE IMAGE
Image has the following delay load dependencies:
SHELL32.dll
ole32.dll
Image has the following dependencies:
mimalloc-redirect.dll
mimalloc.dll
swiftCore.dll
_CompilerSwiftCompilerPluginMessageHandling.dll
_CompilerSwiftSyntaxMacroExpansion.dll
_CompilerSwiftSyntaxMacros.dll
_CompilerSwiftIDEUtils.dll
_CompilerSwiftIfConfig.dll
_CompilerSwiftOperators.dll
_CompilerSwiftSyntaxBuilder.dll
_CompilerSwiftParserDiagnostics.dll
_CompilerSwiftParser.dll
_CompilerSwiftBasicFormat.dll
_CompilerSwiftDiagnostics.dll
_CompilerSwiftSyntax.dll
RPCRT4.dll
cmark-gfm.dll
VERSION.dll
ADVAPI32.dll
ntdll.dll
KERNEL32.dll
OLEAUT32.dll
MSVCP140.dll
VCRUNTIME140.dll
VCRUNTIME140_1.dll
api-ms-win-crt-runtime-l1-1-0.dll
api-ms-win-crt-heap-l1-1-0.dll
api-ms-win-crt-environment-l1-1-0.dll
api-ms-win-crt-stdio-l1-1-0.dll
api-ms-win-crt-math-l1-1-0.dll
api-ms-win-crt-time-l1-1-0.dll
api-ms-win-crt-convert-l1-1-0.dll
api-ms-win-crt-string-l1-1-0.dll
api-ms-win-crt-utility-l1-1-0.dll
api-ms-win-crt-filesystem-l1-1-0.dll
api-ms-win-crt-locale-l1-1-0.dll
_CompilerSwiftLexicalLookup.dll
Summary
188000 .data
391000 .pdata
1F4A000 .rdata
D7000 .reloc
1000 .rsrc
1000 .sw5acfn
2000 .sw5asty
1000 .sw5bltn
1000 .sw5cptr
9000 .sw5flmd
1000 .sw5hash
1000 .sw5mpen
1000 .sw5prt
2000 .sw5prtc
1000 .sw5ratt
1000 .sw5repl
1000 .sw5reps
6000 .sw5rfst
1000 .sw5test
1000 .sw5tymd
5000 .sw5tyrf
6EE6000 .text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also build http://github.com/compnerd/cassowary this way with the above change.
>git clone http://github.com/compnerd/cassowary
>cd cassowary
>swift build -Xswiftc -explicit-module-build
Building for debugging...
[23/23] Linking C:\Users\hiroshi\cassowary\.build\x86_64-unknown-windows-msvc\debug\cassowary.dll
Build complete! (2.89s)
This also fails like hello.swift
without this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember, it was too long ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it may be a different bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cachemeifyoucan WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to fix -fbuiltin-headers-in-system-modules
flag. It is missing from dependency scanner args.
I think it really should't be initialized outside getClangCC1Arguments
and it should ideally be inside getClangDriverArguments
.
I am still against hacking -sdk
here if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean we want to move the code that adds the -fbuiltin-headers-in-system-modules
to getClangDriverArguments
? I moved it to addCommonInvocationArguments
for which getClangDriverArguments
is the only caller and next to the code that adds the ivfsoverlay
flag (though we could alternatively have it in getClangDriverArguments
). Does that work?
I don't think we need to use -sdk
.
7c1f248
to
d5a35ff
Compare
@swift-ci please test |
@ian-twilightcoder @cachemeifyoucan Please take another look |
d5a35ff
to
87b19aa
Compare
@swift-ci please test |
87b19aa
to
0404200
Compare
Does this fix #70330 ? Can we re-enable no-implicit-extra-clang-opts.swift? |
Doesn't seem to. Here's the output no-implicit-extra-clang-opts.swift.txt |
Well that's unfortunate |
3f9160b
to
24dd1cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
@ian-twilightcoder Any more thoughts? |
@swift-ci please test |
24dd1cf
to
8698e59
Compare
@swift-ci please test |
lib/ClangImporter/ClangImporter.cpp
Outdated
auto swiftTargetClangArgs = | ||
getClangCC1Arguments(importer.get(), ctx, VFS, true); | ||
auto swiftTargetClangArgs = getClangCC1Arguments(importer.get(), ctx, VFS, | ||
/*requiresBuiltinHeadersInSystemModulestrue*/false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we know this is always false? Do we never use a vfs in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to match the getClangCC1Arguments
above in the same function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
8698e59
to
89083b7
Compare
Please take another look @cachemeifyoucan @ian-twilightcoder |
std::vector<std::string> commandLineArgs = | ||
ClangImporter::getClangDriverArguments(ctx); | ||
ClangImporter::getClangDriverArguments(ctx, | ||
fileMapping.requiresBuiltinHeadersInSystemModules); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to set -fbuiltin-headers-in-system-modules
here when we aren't actually using the file mapping. It's the file mapping that determines whether you need -fbuiltin-headers-in-system-modules
. @cachemeifyoucan is the scanner picking up the Swift provided module maps in another spot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain a bit more about your concern? I am not quite sure what you mean by not using the file mapping. All the file mapping is important during scanning to provide the correct module while the main clang importer will only load modules and PCH (and currently very little header file) so the file mapping has little impact there, just need to match so there isn't module loading error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I unconditionally pass requiresBuiltinHeadersInSystemModules=false
here, hello.swift
fails to build due to a diag::err_pch_langopt_mismatch
of BuiltinHeadersInSystemModulesmismatch
detected in the ASTReader
. I think it's the mismatch between the AST writing time and the AST reading time that we discussed above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this spot in the code, we're getting the file mapping and then throwing it away without adding any vfs arguments from it. Is there some other spot in the dependency scanner where it gets and uses the file mapping vfs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most VFS is configured into the VirtualFileSystem and passed to scanner directly. But if the VFS setting is affecting reader writer configuration, it will cause problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would simply adding an internal/private field in the ClangImporter
class or the ClangImporter::Implementation
class work, as opposed to part of ClangImporterOptions
or ExtraArgs
? We just need to save a bool value from ClangImporter::create
and later read it in ClangImporter::getHeaderDependencies
and ClangImporter::getModuleDependencies
(the two callers of getClangDepScanningInvocationArguments
) and then pass it here to getClangDepScanningInvocationArguments
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I missed the subtlety that the callers were ClangImporter
since they're in ClangModuleDependencyScanner.cpp. Yeah I think an internal/private field would work, and then we wouldn't have to pass it as parameters all over the place too 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess we'd have to make getClangDriverArguments
/getClangCC1Arguments
non-static to not have to pass it as a parameter. Maybe it's worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Updated accordingly. I added a private field. I made getClangDriverArguments/getClangCC1Arguments
non-static, which led to additional simplifications because getClangCC1Arguments
already took an importer instance as the first argument. I also ended up making getClangDepScanningInvocationArguments
an instance member function as it also took an importer instance as the first argument and it looked kind of similar to getClangDriverArguments/getClangCC1Arguments
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, thanks for sticking with it!
@swift-ci please test |
This fixes explicit module builds for a hello world program on Windows as well as the ucrt import build failure as in the included test.
Note: the CI passed at the current revision of this PR |
89083b7
to
8312f7a
Compare
@swift-ci please test |
@swift-ci please test macos platform |
This fixes explicit module builds for a hello world program on Windows as well as the ucrt import build failure as in the included test.