[Driver] Recognize embedded stdlib layout when picking plugin paths#2150
Conversation
hnrklssn
requested review from
DougGregor,
artemcm,
cachemeifyoucan,
compnerd and
owenv
as code owners
For embedded `noneOS` targets the stdlib lives at `<resource-dir>/embedded/<triple>/Swift.swiftmodule`, but the existing `hasToolchainStdlib` probe only checked `<resource-dir>/<platform>/`, which is `<resource-dir>/Swift.swiftmodule` (no platform subdir) for `noneOS` because `Triple.platformName()` returns nil. The probe always returned false, so the conditional swap in `addCommonFrontendOptions` placed the SDK's `-external-plugin-path` ahead of the toolchain's `-plugin-path`. The first-emplace race in `PluginLoader` then registered the bootstrap Xcode SDK's (potentially stale) `libSwiftMacros.dylib` as `SwiftMacros`, shadowing the freshly-built one and producing "type 'SwiftMacros.SwiftifyImportMacro' could not be found" errors when the embedded headers used macros that post-date the bootstrap platform (and some CI machines run a pretty old host OS, so they lack `_SwiftifyImport`). This should mostly be an issue when bootstrapping. Branch the probe on `triple.os == .noneOS` so the embedded layout is checked directly, with a single `fileSystem.exists` call. Covers Apple `*-apple-none-macho` (the visible failure), plus Wasm `*-unknown-none-wasm`, ARM/RISC-V/x86 `*-none-*-eabi|elf`, and AVR. Add 8 regression tests in `ToolchainTests` (4 Apple macho + 4 non-Apple `noneOS`), each gated with `requireFrontendSupportsTarget` so they self-skip on toolchains lacking the relevant LLVM backend. rdar://178814859
hnrklssn
force-pushed
the
embedded-stdlib-macros
branch
from
840eddd to
6763ab5
Compare
DougGregor
left a comment
DougGregor
left a comment
There was a problem hiding this comment.
This looks reasonable, thank you!
|
@swift-ci please test |
|
|
||
| // Check that we pass the toolchain plugin path before the external plugin | ||
| // path for embedded targets. | ||
| @Test(.requireFrontendSupportsTarget("armv6-apple-none-macho")) |
There was a problem hiding this comment.
This is a perfect case to use parameterized tests?
There was a problem hiding this comment.
I was not familiar with that feature so I looked it up, but it doesn't seem like that would be compatible with .requireFrontendSupportsTarget since that's only evaluated once (unless I'm missing something). I could add a guard inside the test function if you want, but then it would show up as passing instead of unsupported. Maybe that's still preferable?
There was a problem hiding this comment.
You don't need to use requireFrontendSupportsTarget in your Test trait. You just need to construct Collection to be passed as argument: and you can construct this collection with supported target.
There was a problem hiding this comment.
Ahh yeah that works, thanks!
|
@swift-ci please test |
|
@swift-ci please test windows platform |
3 participants
For embedded
noneOStargets the stdlib lives at<resource-dir>/embedded/<triple>/Swift.swiftmodule, but the existinghasToolchainStdlibprobe only checked<resource-dir>/<platform>/, which is<resource-dir>/Swift.swiftmodule(no platform subdir) fornoneOSbecauseTriple.platformName()returns nil. The probe always returned false, so the conditional swap inaddCommonFrontendOptionsplaced the SDK's-external-plugin-pathahead of the toolchain's-plugin-path. The first-emplace race inPluginLoaderthen registered the bootstrap Xcode SDK's (potentially stale)libSwiftMacros.dylibasSwiftMacros, shadowing the freshly-built one and producing"type 'SwiftMacros.SwiftifyImportMacro' could not be found" errors when the embedded headers used macros that post-date the bootstrap platform (and some CI machines run a pretty old host OS, so they lack
_SwiftifyImport). This should mostly be an issue when bootstrapping.Branch the probe on
triple.os == .noneOSso the embedded layout is checked directly, with a singlefileSystem.existscall. Covers Apple*-apple-none-macho(the visible failure), plus Wasm*-unknown-none-wasm, ARM/RISC-V/x86*-none-*-eabi|elf, and AVR.Add 8 regression tests in
ToolchainTests(4 Apple macho + 4 non-ApplenoneOS), each gated withrequireFrontendSupportsTargetso they self-skip on toolchains lacking the relevant LLVM backend.rdar://178814859
Assisted-by: Claude Opus 4.7