windows: establish the MSVC host compiler foundation - #2327
Conversation
There was a problem hiding this comment.
Review: Windows LLVM target triple -gnu → -msvc
This change to GetTargetTriple is internally coherent and well-tested: the arm64 Windows case, the pc vendor validation, and the i686/i386 split are all consistent, and the ABI-choice comment is accurate. Security and performance passes found nothing (pure string builder; triple flows to clang as a discrete -target arg and into filepath.Join cache paths — no injection or traversal exposure).
One cross-cutting concern is worth confirming before merge (details below).
Parallel triple builder in ssa/target.go was not updated
GetTargetTriple (this PR) and Spec() in ssa/target.go are near-identical copy-paste implementations of the same triple logic — same "Target triples ... historical reasons" comment, same arch/vendor/os switches. This PR updated only the copy in llvm.go. After this change the two disagree for Windows:
ssa/target.go:156,176-178still yields vendorunknown, env-gnu, andi386for windows/386 →x86_64-unknown-windows-gnu/i386-unknown-windows-gnu.GetTargetTriplenow yieldsx86_64-pc-windows-msvc/i686-pc-windows-msvc.
These are consumed on the same Windows build:
ssa/package.go:598—mod.SetTarget(p.Target().Spec().Triple)embeds thessa/target.gotriple (-gnu) into the emitted LLVM module, driving codegen ABI/mangling.internal/crosscompile/crosscompile.go:221andinternal/plan9asm/translate.go:108feedGetTargetTriple(-msvc) to the clang-targetand asm translation.
Tagging a module windows-gnu while invoking clang/lld with -target ...windows-msvc is a MinGW-vs-MSVC ABI/environment mismatch (name mangling, struct/EH layout, C runtime differ). If the intent is to move Windows to the MSVC ABI, ssa/target.go should be updated in lockstep — ideally by having it call llvm.GetTargetTriple (or a shared helper) so the two cannot drift again. The llvm.go:25-26 comment ("keep the default in sync with ssa.Target.Spec") reinforces that these are expected to agree. If this is deliberately deferred to a follow-up in this "foundation" series, a note to that effect would help.
Relatedly, the ssa triple test table (ssa/ssa_test.go:2594-2598) has no Windows case, so the stale ssa/target.go value is unguarded by CI. Adding windows/{amd64,386,arm64} cases mirroring the new llvm_test.go expectations (once both sites agree) would lock them together.
Additional findings
internal/xtool/llvm/llvm.go:44: [P3] Redundantllvmos = "macosx"assignment:llvmos = "macosx"is already set at line 39; this assignment inside theaarch64branch is dead. Not introduced by this PR, but since the function is being edited, consider removing it for clarity.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
Addressed the review in e2405d8:
The updated helper, full SSA suite, crosscompile/plan9asm/internal-build consumers, and vet all pass locally. The PR remains Draft. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
be4791b to
c49235e
Compare
|
@fennoai The consolidated Windows foundation is now at b997b3b and all CI checks pass, including the native Windows host smoke and the final dual-platform coverage merge (92.11% patch). Could you please re-review the current head, especially the consolidated metadata mapping, cache locking, shared target-triple logic, and Windows CI changes? |
Re-review: Windows MSVC host compiler foundationReviewed the locked range Shared target-triple logic — ✅
Metadata mapping (
|
|
Thanks for the re-review. No code changes are needed for the two non-blocking notes:
|
There was a problem hiding this comment.
Review: Windows toolchain foundation
Solid, well-scoped PR. The cross-platform mmap/filelock splits are minimal and correct, the releaseLock behavior change (retaining the lock file to avoid a TOCTOU inode swap) is sound and well-documented, and the new tests are thorough (the concurrency test asserts real mutual exclusion via the active counter). No performance regressions found; lock files are keyed to a small fixed set of dirs and every handle/mapping is closed on all paths.
Two things worth confirming, plus a couple of minor cleanups. Details inline and below.
Minor (no inline location):
-
ssa/target.go— duplicated arch mapping.Spec()now delegates the triple tointllvm.GetTargetTripleWithGOARM, but retains its ownswitch goarchcomputingllvmarch, now used only to pick ARM feature strings. The386/amd64/arm64/wasmarms are dead for triple purposes and duplicatellvm.go; the local copy usesi386for 386 whilellvm.gospecial-casesi686for windows/386 — a latent divergence if that local value were ever reused. Consider reducing the local switch to just the ARM-version resolution it needs. -
internal/xtool/llvm/llvm.go—-gnueabihffor GOARM 5/6. Forgoarch=armthe arch honors GOARM (armv5/armv6/armv7) but the environment suffix is unconditionally-gnueabihf. armv5/armv6 targets are frequently soft-float /gnueabi; the hard-float suffix for the lower GOARM values may not match the intended ABI. It's tested, so likely intentional — worth confirming.
There was a problem hiding this comment.
Review: Windows toolchain foundation
Solid, well-scoped PR. The cross-platform mmap/filelock splits are minimal and correct, the releaseLock behavior change (retaining the lock file to avoid a TOCTOU inode swap) is sound and well-documented, and the new tests are thorough (the concurrency test asserts real mutual exclusion via the active counter). No performance regressions found; lock files are keyed to a small fixed set of dirs and every handle/mapping is closed on all paths.
Two things worth confirming, plus a couple of minor cleanups. Details inline and below.
Minor (no inline location):
-
ssa/target.go— duplicated arch mapping.Spec()now delegates the triple tointllvm.GetTargetTripleWithGOARM, but retains its ownswitch goarchcomputingllvmarch, now used only to pick ARM feature strings. The386/amd64/arm64/wasmarms are dead for triple purposes and duplicatellvm.go; the local copy usesi386for 386 whilellvm.gospecial-casesi686for windows/386 — a latent divergence if that local value were ever reused. Consider reducing the local switch to just the ARM-version resolution it needs. -
internal/xtool/llvm/llvm.go—-gnueabihffor GOARM 5/6. Forgoarch=armthe arch honors GOARM (armv5/armv6/armv7) but the environment suffix is unconditionally-gnueabihf. armv5/armv6 targets are frequently soft-float /gnueabi; the hard-float suffix for the lower GOARM values may not match the intended ABI. It's tested, so likely intentional — worth confirming.
|
The latest revision is fully green across the complete CI matrix, including:
The review fixes are included: target architecture mapping now has a single source of truth, and bare GOARM=5 uses soft-float while GOARM=6/7 retain the Go-compatible hard-float default. @fennoai, could you please re-review the current head? |
Re-review: current head
|
996f005 to
342dfcc
Compare
342dfcc to
f2d5f7f
Compare
|
@fennoai The final head Could you please re-review the current head, especially the incremental C ABI lowering change that preserves COFF COMDAT and the small helper extractions used to cover metadata-size and cache-lock error paths? The same head also passed local Win11 ARM64 host validation with LLVM 19.1.7. |
Re-review: current head
|
Part of #2325, the MSVC-first Windows support proposal.
Summary
GOARM=5soft-float;GOARM=6/7hard-float)The COMDAT preservation prevents Go ODR symbols such as generic instantiations from degrading into COFF weak fallbacks with
NoDuplicatessection selection after ABI lowering. Without it, linking larger standard-library programs can report duplicate symbols even though SSA originally emittedcomdat any.This consolidates the former #2328, #2330, and #2332 into one foundation PR so later Windows work has a single dependency.
Dependency
None. The branch is based directly on xgo-dev/llgo main.
Commit structure
Scope
This is the host/toolchain foundation for #2325. It does not yet claim that LLGo-generated Windows programs, the Windows runtime, goroutines/threads, GC, closure environments, defer/panic/recover, debug information, FFI/CABI, or cross-compilation SDKs are complete. Those remain follow-up work.
The native smoke deliberately runs the tests owned by this PR. It does not run unrelated embedded libc, ESP download, Unix-permission, or full crosscompile tests whose assumptions are not yet Windows-portable.
Local validation
go test -race ./internal/metago test -race -count=20 -run '^TestAcquire(AndReleaseLock(Errors)?|LockConcurrency)$' ./internal/crosscompilego test ./internal/crosscompile ./internal/meta ./internal/xtool/llvmgo test ./ssago test -tags=byollvm,llvm19 ./internal/abi ./internal/cabigo vet ./internal/meta ./internal/crosscompile ./internal/xtool/llvm ./ssawindows/386,windows/amd64, andwindows/arm64Native Windows validation
Windows run 32052116512 passed on
windows-amd64at the current head with Go 1.26.5 and LLVM 19.1.7 in 2m55s. The workflow pins the matching Clang, LLVM, LLD, compiler-rt, libc++, libunwind, and runtime dependency closure so MSYS2 rolling updates cannot silently change the compiler version. It:llgo-windows-smoke.exellgo-windows-smoke.exe versionThe same current head also passed on a real Windows 11 ARM64 VM using Go 1.26.5, LLVM 19.1.7, and the Visual Studio ARM64 SDK/toolchain. The VM built directly from the macOS Home share at
\\Mac\Home\source\goplus\llgo-wt-windows-toolchain-20260815(without copying source), ran the affected tests, built the compiler, and reportedllgo (devel) windows/arm64.The COFF regression was also reproduced and verified on a real Windows 11 ARM64 VM with LLVM 19.1.7 and Go 1.26.5. Before the fix, the lowered
io/fsobject contained a genericslices.SortFuncfallback in aNoDuplicatesCOMDAT and lld-link rejected duplicate definitions. After the fix, the same section usesAny; the stacked runtime, standard-library, and callback/FFI smoke binaries all linked and ran successfully.Final CI and coverage
All 43 checks passed on
ce04acdd7, including Linux and macOS Go 1.26.5 tests, Go 1.24 compatibility, Windows AMD64 host smoke, release-artifact smoke, benchmarks, and the development LTO jobs. Codecov reports that every modified and coverable line is covered by tests (100% patch coverage); both Linux and macOS coverage uploads completed successfully.The PR is ready for review.