Skip to content

windows: establish the MSVC host compiler foundation - #2327

Open
cpunion wants to merge 22 commits into
xgo-dev:mainfrom
cpunion:codex/windows-toolchain-foundation-20260815
Open

windows: establish the MSVC host compiler foundation#2327
cpunion wants to merge 22 commits into
xgo-dev:mainfrom
cpunion:codex/windows-toolchain-foundation-20260815

Conversation

@cpunion

@cpunion cpunion commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Part of #2325, the MSVC-first Windows support proposal.

Summary

  • default Go's official Windows targets (386, amd64, and arm64) to LLVM's MSVC environment
  • share target-triple construction between SSA modules and clang invocations
  • keep ARM version/float defaults aligned with Go (GOARM=5 soft-float; GOARM=6/7 hard-float)
  • map package metadata with Windows file mappings while retaining the Unix mmap backend
  • implement Windows cache locking with LockFileEx/UnlockFileEx while retaining Unix flock
  • preserve COFF COMDAT ownership when aggregate/C ABI lowering replaces functions and when C ABI callback wrappers are synthesized
  • add a native windows-amd64 host smoke job using a matching MSYS2 CLANG64 LLVM toolchain
  • build llgo.exe and run its version command on Windows

The COMDAT preservation prevents Go ODR symbols such as generic instantiations from degrading into COFF weak fallbacks with NoDuplicates section selection after ABI lowering. Without it, linking larger standard-library programs can report duplicate symbols even though SSA originally emitted comdat 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

  1. select MSVC target triples
  2. share triple construction with SSA and preserve GOARM handling
  3. document the Windows 386 i686 choice
  4. add Windows package-metadata mapping
  5. add Windows cache-file locking
  6. add and harden the native Windows host smoke job
  7. remove duplicate SSA architecture mapping and match Go's ARM float defaults
  8. preserve COFF COMDAT through aggregate/C ABI lowering
  9. cover metadata-size, cache-lock cleanup, and GOARM 5/6 edge cases

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/meta
  • go test -race -count=20 -run '^TestAcquire(AndReleaseLock(Errors)?|LockConcurrency)$' ./internal/crosscompile
  • go test ./internal/crosscompile ./internal/meta ./internal/xtool/llvm
  • go test ./ssa
  • go test -tags=byollvm,llvm19 ./internal/abi ./internal/cabi
  • go vet ./internal/meta ./internal/crosscompile ./internal/xtool/llvm ./ssa
  • ran the affected target-triple and SSA tests with Go 1.24.11 and Go 1.26.5
  • compiled the affected packages for windows/386, windows/amd64, and windows/arm64
  • workflow YAML and patch whitespace validation

Native Windows validation

Windows run 32052116512 passed on windows-amd64 at 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:

  • ran the package-metadata mapping and boundary-validation tests
  • ran Windows cache locking, mutual exclusion, and failed-lock cleanup tests
  • checked helper/SSA MSVC triples and GOARM 5/6 target features
  • built llgo-windows-smoke.exe
  • executed llgo-windows-smoke.exe version

The 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 reported llgo (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/fs object contained a generic slices.SortFunc fallback in a NoDuplicates COMDAT and lld-link rejected duplicate definitions. After the fix, the same section uses Any; 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.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-178 still yields vendor unknown, env -gnu, and i386 for windows/386 → x86_64-unknown-windows-gnu / i386-unknown-windows-gnu.
  • GetTargetTriple now yields x86_64-pc-windows-msvc / i686-pc-windows-msvc.

These are consumed on the same Windows build:

  • ssa/package.go:598mod.SetTarget(p.Target().Spec().Triple) embeds the ssa/target.go triple (-gnu) into the emitted LLVM module, driving codegen ABI/mangling.
  • internal/crosscompile/crosscompile.go:221 and internal/plan9asm/translate.go:108 feed GetTargetTriple (-msvc) to the clang -target and 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] Redundant llvmos = "macosx" assignment: llvmos = "macosx" is already set at line 39; this assignment inside the aarch64 branch is dead. Not introduced by this PR, but since the function is being edited, consider removing it for clarity.

Comment thread internal/xtool/llvm/llvm.go
@github-actions

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

0fa60d6f7162 | workflow run | long-term charts

Program measurements

Platform Workload File size vs base Build vs base Run vs base
Linux cprintf 19384 B +0.0% 358.126 ms +9.5% (worse) 1.405 ms +6.0% (worse)
Linux fmtprintf 1880776 B +0.0% 2.731 s +1.9% (worse) 3.350 ms +4.6% (worse)
Linux println 68880 B +0.0% 347.599 ms +7.8% (worse) 1.735 ms +8.5% (worse)
macOS cprintf 84672 B +0.0% 613.313 ms +33.8% (worse) 3.538 ms +5.9% (worse)
macOS fmtprintf 1892208 B +0.0% 3.049 s -0.8% (better) 14.308 ms +10.0% (worse)
macOS println 121376 B +0.0% 486.372 ms +13.6% (worse) 4.404 ms +1.1% (worse)
Core language and compiler benchmarks
Platform Benchmark ns/op vs base
Linux BenchmarkLookupPCRandom 13.310 ns/op -0.6% (better)
Linux BenchmarkMergeCompilerFlags 153.900 ns/op +1.9% (worse)
Linux BenchmarkMergeLinkerFlags 100.500 ns/op +5.2% (worse)
Linux BenchmarkChannelBuffered 34.460 ns/op +2.3% (worse)
Linux BenchmarkChannelHandoff 29251 ns/op +3.2% (worse)
Linux BenchmarkDefer 45.100 ns/op +2.9% (worse)
Linux BenchmarkDirectCall 1.557 ns/op -0.1% (better)
Linux BenchmarkGlobalRead 1.557 ns/op -0.1% (better)
Linux BenchmarkGlobalWrite 2.481 ns/op +0.1% (worse)
Linux BenchmarkGoroutine 31203 ns/op -0.6% (better)
Linux BenchmarkInterfaceCall 8.233 ns/op +1.7% (worse)
Linux BenchmarkRuntimeGetG 1.877 ns/op +0.5% (worse)
macOS BenchmarkLookupPCRandom 12.110 ns/op -33.9% (better)
macOS BenchmarkMergeCompilerFlags 128.100 ns/op +5.5% (worse)
macOS BenchmarkMergeLinkerFlags 80.590 ns/op -6.0% (better)
macOS BenchmarkChannelBuffered 25.890 ns/op +14.2% (worse)
macOS BenchmarkChannelHandoff 8753 ns/op +2.1% (worse)
macOS BenchmarkDefer 38.540 ns/op -18.5% (better)
macOS BenchmarkDirectCall 1.130 ns/op +8.9% (worse)
macOS BenchmarkGlobalRead 1.067 ns/op -3.5% (better)
macOS BenchmarkGlobalWrite 1.071 ns/op -6.5% (better)
macOS BenchmarkGoroutine 44773 ns/op -12.0% (better)
macOS BenchmarkInterfaceCall 6.085 ns/op -2.9% (better)
macOS BenchmarkRuntimeGetG 2.076 ns/op -1.5% (better)

Compared with 0f480253029a measured in the same runner job.

@cpunion cpunion changed the title xtool/llvm: default Windows targets to MSVC target: default Windows triples to MSVC Aug 15, 2026
@cpunion

cpunion commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review in e2405d8:

  • SSA Target.Spec now uses the shared internal triple builder, so emitted LLVM modules and clang invocations both use the MSVC environment.
  • The shared builder accepts GOARM and preserves the existing ARMv5/ARMv6/ARMv7 behavior.
  • Added SSA regression coverage for Windows 386, amd64, and arm64, plus helper coverage for GOARM variants.
  • Removed the redundant macosx assignment.

The updated helper, full SSA suite, crosscompile/plan9asm/internal-build consumers, and vet all pass locally. The PR remains Draft.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cpunion
cpunion force-pushed the codex/windows-toolchain-foundation-20260815 branch from be4791b to c49235e Compare August 15, 2026 07:55
@cpunion cpunion changed the title target: default Windows triples to MSVC windows: establish the MSVC host compiler foundation Aug 15, 2026
@cpunion

cpunion commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

@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?

@fennoai

fennoai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Re-review: Windows MSVC host compiler foundation

Reviewed the locked range 4a5bd76...b997b3b (13 files, +283/-49) with focus on the four areas you flagged. Overall this is a clean, well-scoped foundation — the platform splits are idiomatic, the triple logic is now shared, and the tests meaningfully exercise the new paths. No blocking issues found.

Shared target-triple logic — ✅

  • ssa/target.go now delegates to intllvm.GetTargetTripleWithGOARM, eliminating the duplicated triple builder. GOARM is threaded through, and the ARM armv5/6/7 selection in the helper matches the local llvmarch used for spec.Features, so they can't diverge.
  • No triple regressions for existing targets: darwin/amd64 → x86_64-apple-macosx, darwin/arm64 → arm64-apple-macosx, linux/arm → armv7-unknown-linux-gnueabihf all still hold (confirmed against the test table). The redundant double llvmos = "macosx" from the old code is gone.
  • Windows now correctly yields -msvc triples with pc vendor and i686 for 386. The other two callers (internal/crosscompile, internal/plan9asm) go through the same helper, so they pick up the MSVC change consistently.

Metadata mapping (internal/meta) — ✅

  • The mmap_unix.go / mmap_windows.go split is clean. The Windows CreateFileMapping/MapViewOfFile path defers CloseHandle(mapping) correctly (the view stays valid after the section handle closes).
  • Good catch adding the size guard in Open (size <= 0 || size > maxInt): a 0-byte file would make CreateFileMapping(..., 0, 0) fail on Windows, and it also bounds the int(fi.Size()) conversion.
  • Close now clears both raw and mmap, making it idempotent — the new double-Close test covers this. go test ./internal/meta passes locally.

Cache locking (internal/crosscompile) — ✅ with one note

  • filelock_unix.go (flock) / filelock_windows.go (LockFileEx/UnlockFileEx, exclusive, 1-byte range) is a correct, symmetric split. Blocking semantics match on both platforms (no LOCKFILE_FAIL_IMMEDIATELY).
  • Not removing the lock file on release is the right call — the comment explains the TOCTOU risk correctly, and the retained-file + reacquire test guards it.
  • Minor (non-blocking): in TestAcquireLockConcurrency, active.Add(-1) runs before releaseLock(lockFile). The real lock is still held until the release, so this can't produce a false positive, but decrementing the guard after the release would model the critical section more precisely.

Windows CI (.github/workflows/windows.yml) — ✅ with one note

  • MSYS2 CLANG64 + llvm-config-derived CGO flags is a sound host setup, and gating on 22.* keeps the toolchain pinned. set -euo pipefail and the explicit version check harden it well.
  • Minor (non-blocking): the build uses -tags=byollvm,llvm22, but llvm22 matches no //go:build constraint anywhere in the tree — with byollvm the LLVM path comes entirely from llvm-config/ldflags, so llvm22 is currently a no-op. Harmless, but you may want to drop it or add a matching constraint to avoid implying a version-specific config exists.

Verification

  • go test ./internal/meta — pass (mmap refactor + double-Close).
  • go vet / go test on ssa, internal/crosscompile, internal/xtool/llvm could not run here: the sandbox lacks LLVM headers (llvm-c/Core.h) for the xgo-dev/llvm cgo dep, and clang for the triple cross-check test. These are covered by your native Windows run and existing CI.

Scope looks appropriately narrow for a foundation PR — nothing here overreaches into runtime/GC/FFI territory. Nice work.

@cpunion

cpunion commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the re-review. No code changes are needed for the two non-blocking notes:

  • The active counter intentionally brackets the protected test payload; the OS lock is held slightly longer through teardown, so the assertion remains conservative for detecting overlapping critical sections.
  • llvm22 is consumed by the pinned github.com/xgo-dev/llvm v0.9.6 dependency (switch_llvm22.go) even when byollvm is set. byollvm replaces the packaged cgo path configuration, while llvm22 selects the LLVM 22-specific LLVMGetSwitchCaseValue API shim, so both tags are required.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to intllvm.GetTargetTripleWithGOARM, but retains its own switch goarch computing llvmarch, now used only to pick ARM feature strings. The 386/amd64/arm64/wasm arms are dead for triple purposes and duplicate llvm.go; the local copy uses i386 for 386 while llvm.go special-cases i686 for 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-gnueabihf for GOARM 5/6. For goarch=arm the 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.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to intllvm.GetTargetTripleWithGOARM, but retains its own switch goarch computing llvmarch, now used only to pick ARM feature strings. The 386/amd64/arm64/wasm arms are dead for triple purposes and duplicate llvm.go; the local copy uses i386 for 386 while llvm.go special-cases i686 for 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-gnueabihf for GOARM 5/6. For goarch=arm the 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.

Comment thread internal/meta/meta.go
Comment thread internal/xtool/llvm/llvm.go
@cpunion

cpunion commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

The latest revision is fully green across the complete CI matrix, including:

  • Go 1.26.5 on Linux and macOS
  • Go 1.24.2 compatibility jobs
  • Windows AMD64 host smoke with LLVM 19
  • Linux/macOS coverage and benchmark jobs
  • release artifact smoke tests

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?

@fennoai

fennoai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Re-review: current head 775151b5

Reviewed the full locked range 4a5bd76...775151b5 and the incremental delta since my last review (b997b3b → 775151b5). The review fixes landed cleanly and the revision adds substantial, well-tested hardening. No blocking issues.

Review fixes — all resolved ✅

  • Single source of truth for arch mapping: the duplicate llvmarch switch in ssa/target.go is gone. Spec() now takes the triple entirely from GetTargetTripleWithGOARM, and the ARM spec.Features switch keys directly on p.GOARM (5/6/default). This closes the divergence risk I raised — there's now exactly one place mapping GOARCH/GOARM to an LLVM arch.
  • GOARM float ABI: GOARM=5 → -gnueabi (soft-float) and 6/7/default → -gnueabihf (hard-float), with a comment tying it to Go's defaults and the test table updated. Matches Go's convention correctly.
  • Using default: for armv7 features also means an unexpected GOARM value falls back to armv7 instead of empty features — a safer default than before.

New: metadata validation hardening (internal/meta) — ✅ strong

This is the most significant new work and it's a genuine robustness/safety improvement. Because Open mmaps a file whose bytes are then aliased by the CSR slice helpers, a corrupt or malformed .meta file could previously drive out-of-bounds reads. The new newPackageMetavalidate() path now fully bounds-checks before exposing mapped bytes:

  • section offsets validated monotonic, 4-aligned, and within len(raw);
  • symbols-section size checked against nsyms (4 + nsyms*12);
  • every CSR section validated (header count matches nsyms, data size divisible by record size, offset array starts at 0, monotonic, and terminates exactly at nrecords);
  • all name references (Symbols, FuncDemand named-methods, MethodInfo, InterfaceInfo) bounds-checked against the string-table size.
  • Open also tightened its size guard to [headerSize, uint32 max], and Builder.Build now uses the shared packageMetaView (skipping re-validation of bytes it just wrote — correct, since those are trusted).

The layout constants (headerSize, numSections, record sizes 4/12/20/12) line up with builder.go's writers, and the new table-driven tests cover every failure branch (short header, offset past-end/out-of-order/unaligned, truncated/mismatched symbols, CSR corruption, and out-of-range name refs in all four sections). go test ./internal/meta passes locally, including the double-Close idempotency test.

Windows CI (.github/workflows/windows.yml) — ✅

  • Now pins LLVM 19.1.7 with exact MSYS2 package versions and verifies each installed version, replacing the looser "latest 22.x" match — reproducible and much less likely to drift.
  • Added permissions: contents: read (least privilege) and a comment clarifying CLANG64 host libraries vs. the MSVC target ABI.
  • The llvm19 build tag is now consistent with the repo's tag convention; under byollvm the LLVM path still comes from llvm-config/CGO flags, so the tag is effectively a label here — harmless and no longer worth flagging.

Verification

  • go test ./internal/meta — pass (validation suite + mmap/Close).
  • ssa / internal/crosscompile / internal/xtool/llvm still can't be built in this sandbox (no LLVM headers for the xgo-dev/llvm cgo dep, no clang for the triple cross-check test). Those are covered by your green CI matrix and the native Windows smoke.

This is in good shape — the fixes are complete and the metadata validation is a solid addition well beyond the minimum. LGTM for the foundation scope.

@cpunion
cpunion force-pushed the codex/windows-toolchain-foundation-20260815 branch 3 times, most recently from 996f005 to 342dfcc Compare August 16, 2026 11:43
@cpunion
cpunion force-pushed the codex/windows-toolchain-foundation-20260815 branch from 342dfcc to f2d5f7f Compare August 17, 2026 13:40
@cpunion

cpunion commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

@fennoai The final head ce04acdd7 now has all 43 checks green, including Linux/macOS test-with-coverage, the native Windows AMD64 LLVM 19 smoke, release artifacts, and 100% Codecov patch coverage. There are no unresolved review threads.

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.

@fennoai

fennoai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Re-review: current head ce04acdd

Reviewed the full locked range 0f48025...ce04acd (the branch was rebased onto newer main, so this is a fresh full-range read). Focused on the three areas you flagged. No blocking issues — the C ABI change is correct and thoroughly tested, and the helper extractions are clean.

C ABI lowering + COFF COMDAT preservation — ✅ correct

  • internal/cabi/cabi.go (transformFunc) and internal/abi/large.go now copy fn.Comdat() onto the rewritten function. This is a genuine COFF correctness fix: when a signature-rewriting transform recreates a linkonce_odr/weak function, dropping its COMDAT turns it into a weak-external on COFF that lld-link cannot coalesce across objects (duplicate/dropped symbols). Since nfn keeps the original name (fn.SetName("")AddFunction(..., fname, ...)), re-associating the same name-keyed COMDAT is the right move; when fn had no comdat, SetComdat of the nil comdat is a correct no-op, so the copy is safe to leave ungated on non-Windows.
  • The new setODRLinkage helper for freshly created callback wrappers is well-scoped: it only attaches an Any-selection COMDAT when prog.Target().GOOS == "windows", keyed on the wrapper's own name. The comment accurately explains the weak-vs-COMDAT COFF distinction.
  • Test coverage is strong and targets the exact condition: TestCABILoweringPreservesWindowsCOMDAT and TestWindowsCallbackWrapperUsesCOMDAT both drive a windows program target (independent of the triple string, matching how the code gates), assert AnyComdatSelectionKind survives, and run VerifyModule. internal/abi/large_test.go gained a matching linkonce_odr … comdat assertion.

Metadata-size & cache-lock error-path helpers — ✅ clean

  • validateMetaSize(size uint64) extracts the [headerSize, uint32 max] bounds check so the otherwise-hard-to-reach "file too large" branch is unit-testable. Open intentionally keeps its own inline check because it additionally needs the platform-int overflow guard before int(fi.Size()) for the mmap size — a justified superset, not a missed dedup.
  • acquireLockWith(lockPath, lock func(*os.File) error) and lockReleaseError(unlockErr, closeErr) are behavior-preserving extractions that let TestAcquireAndReleaseLockErrors deterministically exercise the acquire-failure and release-error paths (via an injected failing lock fn and errors.Is on the wrapped error) instead of depending on a real flock failure. releaseLock semantics are unchanged.

Test / golden updates — ✅

  • cl/_testdata/method/in.go and cl/_testrt/struct/in.go relax hardcoded i64 <len> FileCheck literals to {{[0-9]+}}. Those lengths encode the absolute source-path length, which differs across build hosts (notably Windows), so the wildcard is the correct portability fix rather than a weakened assertion.
  • cl/_testmeta/reflect_named/meta-expect.txt is a deterministic golden regeneration (reflect iface mangled-name hash changed with the rebase) — no behavior change.
  • ssa/ssa_test.go adds TestARMTargetSpec, which locks down the GOARM default→armv7 feature selection from the prior round.

Verification

  • go test ./internal/meta — pass (size + CSR validation suites).
  • go test ./internal/crosscompile (lock tests, incl. the new error-path coverage) — pass.
  • internal/cabi / internal/abi / ssa still can't be built in this sandbox (no LLVM headers for the xgo-dev/llvm cgo dep), so the COMDAT tests weren't run here — they're covered by your green 43-check matrix and the native Windows AMD64 (and your local Win11 ARM64) LLVM 19.1.7 validation.

The COMDAT preservation is the right fix in the right places with matching Windows-targeted tests, and the refactors improve testability without changing behavior. LGTM for the foundation scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants