[depends on #2335] cabi: lower aggregates for Windows MSVC targets

[cpunion]

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

Depends on #2335.

Summary

• recognize canonical LLVM architecture spellings and select an explicit MSVC ABI implementation for Windows amd64, arm64, and 386 targets
• lower Microsoft x64 and x86 aggregate parameters and return values, while selecting the existing AAPCS64 classification explicitly for Windows arm64
• preserve the MSVC x86 four-byte byval alignment on definitions, calls, and callback wrappers without redirecting naturally aligned local accesses to a weaker incoming pointer
• exercise the lowering from the Windows workflow in addition to the platform-independent C ABI tests

Validation

• go test -count=1 -coverprofile=/tmp/llgo-windows-cabi-cover.out ./internal/cabi (97.7% statement coverage)
• go test -count=1 -run "^(TestTargetArchAndNewTransformerArchSelection|TestMSVC.*)$" ./internal/cabi
• go vet ./internal/cabi
• git diff --check and workflow YAML parsing
• compared every existing internal/cabi/_testdata/wrap C/Go signature against Clang 19 output for x86_64-pc-windows-msvc, aarch64-pc-windows-msvc, and i686-pc-windows-msvc

Scope

This provides the aggregate C ABI foundation for MSVC targets. Runtime threading, GC, panic/recover, debug information, import/export details, other calling conventions, and complete MinGW/Cygwin support remain tracked by #2325.

Native Windows ARM64 validation

Validated on Windows 11 ARM64 with Go 1.26.5 and native MSYS2 CLANGARM64 LLVM 19.1.7:

• the focused MSVC target/classification/call/callback suite passes for windows/amd64, windows/arm64, and windows/386;
• the complete internal/cabi package passes natively in 112.747 seconds on the rebased stack;
• the compiler and LLVM tools are native COFF-ARM64 binaries.
• the VM consumed the source directly from \\Mac\Home\source\goplus\llgo-wt-windows-cabi-20260815; caches and output remained on Windows.

GitHub CI covers Windows AMD64, Ubuntu, macOS, and Codecov on the pushed contribution head.

Final CI and coverage

• all 43 check runs completed without failure: 42 passed and 1 was conditionally skipped
• Linux and macOS full test/coverage jobs passed
• the LLVM 19 Windows AMD64 host-smoke job passed
• Codecov patch coverage is 98.89% (target 49.24%)

[github-actions]

LLGo baseline benchmarks

e4c6ea7c19cb | 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%	328.317 ms	-1.4% (better)	1.371 ms	+6.7% (worse)
Linux	fmtprintf	1880776 B	+0.0%	2.657 s	+0.0% (worse)	3.212 ms	-0.4% (better)
Linux	println	68880 B	+0.0%	336.147 ms	+0.0% (worse)	1.580 ms	-2.1% (better)
macOS	cprintf	84672 B	+0.0%	443.402 ms	+4.4% (worse)	3.667 ms	+39.5% (worse)
macOS	fmtprintf	1892208 B	+0.0%	2.732 s	-5.8% (better)	12.457 ms	-2.6% (better)
macOS	println	121376 B	+0.0%	479.476 ms	+30.2% (worse)	4.194 ms	+3.5% (worse)

Core language and compiler benchmarks

Platform	Benchmark	ns/op	vs base
Linux	BenchmarkLookupPCRandom	13.350 ns/op	+0.2% (worse)
Linux	BenchmarkMergeCompilerFlags	150.400 ns/op	+0.1% (worse)
Linux	BenchmarkMergeLinkerFlags	97.120 ns/op	+2.9% (worse)
Linux	BenchmarkChannelBuffered	33.730 ns/op	+0.2% (worse)
Linux	BenchmarkChannelHandoff	27580 ns/op	-1.5% (better)
Linux	BenchmarkDefer	44.570 ns/op	+1.2% (worse)
Linux	BenchmarkDirectCall	1.558 ns/op	+0.1% (worse)
Linux	BenchmarkGlobalRead	1.560 ns/op	+0.1% (worse)
Linux	BenchmarkGlobalWrite	2.508 ns/op	+1.1% (worse)
Linux	BenchmarkGoroutine	38248 ns/op	+12.8% (worse)
Linux	BenchmarkInterfaceCall	8.101 ns/op	+0.0%
Linux	BenchmarkRuntimeGetG	1.873 ns/op	+0.2% (worse)
macOS	BenchmarkLookupPCRandom	13.170 ns/op	+7.5% (worse)
macOS	BenchmarkMergeCompilerFlags	121.800 ns/op	+9.6% (worse)
macOS	BenchmarkMergeLinkerFlags	79.630 ns/op	-9.8% (better)
macOS	BenchmarkChannelBuffered	30.920 ns/op	-0.1% (better)
macOS	BenchmarkChannelHandoff	7126 ns/op	-0.7% (better)
macOS	BenchmarkDefer	34.640 ns/op	-29.0% (better)
macOS	BenchmarkDirectCall	1.162 ns/op	+1.4% (worse)
macOS	BenchmarkGlobalRead	1.262 ns/op	-9.4% (better)
macOS	BenchmarkGlobalWrite	1.371 ns/op	-7.1% (better)
macOS	BenchmarkGoroutine	30620 ns/op	-2.1% (better)
macOS	BenchmarkInterfaceCall	6.566 ns/op	-22.8% (better)
macOS	BenchmarkRuntimeGetG	2.381 ns/op	-32.8% (better)

Compared with 0f480253029a measured in the same runner job.

[codecov]

Codecov Report

❌ Patch coverage is 99.52756% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/crosscompile/crosscompile.go	96.93%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[cpunion]

@fennoai All GitHub checks pass, and the current head has also passed the focused and complete internal/cabi suites natively on Windows 11 ARM64. Could you please review this PR?

[fennoai]

Review: Windows MSVC C ABI support

Overall this is a high-quality, well-tested PR. The MSVC C ABI lowering (internal/cabi/arch_msvc.go), the per-index attribute-slice refactor, the platform-split filelock_*/mmap_* files, and the internal/meta bounds-validation hardening are clean, idiomatic, and thoroughly covered by tests (VerifyModule-backed lowering tests, GOARCH resolution, LTO mapping). No correctness bugs were found in the changed logic.

The most important findings are in the archive-extraction path that this PR's cross-compilation feature relies on. That code is pre-existing (not in this diff), but Windows toolchain support makes the ZIP path a first-class, reachable download surface — so it is worth addressing here. Inline comments cover the two in-diff refinements.

Security (pre-existing code, no diff line — expanded surface)

• Zip-Slip / path traversal in extractZip — internal/crosscompile/fetch.go:287. filepath.Join(dest, file.Name) is used with no containment check before os.Create (line 299). A malicious/tampered archive entry named ..\..\evil escapes dest and writes arbitrary files. The sibling extractTarGz (line 248) already performs the strings.HasPrefix(target, filepath.Clean(dest)+os.PathSeparator) guard — apply the same to extractZip, and skip/validate symlink entries. Since Windows toolchains ship as .zip, this path becomes reachable with this PR.
• No integrity verification of downloaded toolchains — internal/crosscompile/fetch.go:209 (downloadFile), URLs at crosscompile.go:69. Archives are fetched and extracted with no SHA-256 pinning or signature check; extracted contents are executed as part of the compiler/linker toolchain. A tampered release asset or a redirect/MITM on the download yields code execution at build time. Consider pinning per-URL checksums and rejecting non-HTTPS / cross-host redirects.
• Untrusted file-mode bits on extraction — internal/crosscompile/fetch.go:260. os.OpenFile(target, ..., os.FileMode(header.Mode)) applies the archive's mode verbatim, allowing setuid/setgid/world-writable files. Mask with header.Mode & 0o777 and strip setuid/setgid.

Minor (non-blocking)

• internal/meta/mmap_windows.go / mmap_unix.go: mapFile trusts the caller-supplied size for unsafe.Slice; the bounds check lives only in Open. Fine today (single caller), but a short "caller validates size" contract comment on mapFile would guard against future misuse.
• internal/crosscompile/fetch_test.go:148: active.Add(-1) runs before releaseLock, decrementing the critical-section counter while the lock is still held — weakens the overlap-detection invariant the test asserts. Test-only.

Positives worth calling out: the releaseLock fix (no longer deleting the lock file, with the explanatory comment on the lock-recycling hazard) is correct; the meta.go offset/order/alignment validation loop and idempotent Close() are real robustness wins; the map[int]llvm.Attribute → map[int][]llvm.Attribute generalization was applied consistently across all four consumers.

[fennoai]

Review complete

I reviewed the locked range 4a5bd76...1719198 and submitted a non-blocking PR review.

Summary: The x64 and Windows ARM64 aggregate lowering look correct, the map[int][]Attribute refactor is applied consistently, and the ByValAlign / replaceAllocaInstrs guard is sound. Supporting infra (crosscompile file locking, meta mmap, goarch, triple parsing, workflow) reads cleanly.

One finding (P1, Windows x86 / MSVC only): TypeInfoWindows386.GetTypeInfo omits Clang's Win32 over-alignment rule — structs whose alignment exceeds 4 bytes (containing i64/double) must be passed as a plain indirect pointer (non-byval, natural alignment) before the expand path. As written, {i64,double} is scalar-expanded and {i32,i64} gets byval align 4, both diverging from Clang. Details and a suggested fix are inline on internal/cabi/arch_msvc.go.

Note: LLVM headers aren't available in this sandbox, so I couldn't build or run the suite here — the finding is grounded in Clang's X86_32ABIInfo::classifyArgumentType (release/19.x) rather than a local test run.

[fennoai]

FennoAI Review — MSVC aggregate C ABI lowering

Thanks for the thorough write-up and the native Windows 11 ARM64 validation. I reviewed the locked range 4a5bd76...1719198 (28 files), concentrating on the ABI core (internal/cabi/arch_msvc.go, internal/cabi/cabi.go, tests). The supporting infrastructure (crosscompile file locking, meta mmap, goarch, target-triple parsing, workflow) reads cleanly.

The map[int]llvm.Attribute → map[int][]llvm.Attribute refactor is correct and is applied consistently across transformFunc, transformCallInstr, and transformCallbackFunc. The ByValAlign handling and the replaceAllocaInstrs guard (ti.ByValAlign == 0 || ti.ByValAlign >= ti.Align) are sound — they correctly avoid redirecting naturally-aligned local accesses onto a weaker 4-byte incoming pointer.

Verification note: LLVM headers are not available in this sandbox, so I could not compile or run the suite here. The findings below are grounded in Clang's authoritative X86_32ABIInfo::classifyArgumentType (release/19.x), not in local test runs.

I found one ABI-correctness issue scoped to Windows x86 (MSVC) only. The x64 and arm64 paths look correct (x64: 1/2/4/8-byte aggregates in one integer register, everything else indirect via sret/pointer, no register splitting; arm64: explicit AAPCS64 reuse).

What I could not fully verify

• Runtime/link-level behavior (no LLVM toolchain here). CI green + your native runs cover this; I'm relying on those for the paths I couldn't exercise.

[cpunion]

@fennoai Please re-review the current head now that the full CI matrix, native Windows host smoke, both coverage jobs, and Codecov have passed. The four prior review threads have been addressed or resolved with the Clang 19 x86 ABI evidence recorded in-thread.

[fennoai]

Re-review complete — LGTM

I re-reviewed head 2baf2d4 over the locked range 4a5bd76...2baf2d4 and submitted a non-blocking PR review.

Prior P1 finding — retracted; I was wrong. I re-read Clang 19's X86_32ABIInfo::classifyArgumentType: the Win32 over-alignment check is gated on the record's required alignment (getRequiredAlignment()/isAlignRequired() — i.e. explicit alignas/__declspec(align)), not the natural 8-byte alignment of an i64/double member. So naturally-aligned structs correctly fall through to expand ({i64,double}→AttrExtract) or the padded byval-align-4 fallback ({i32,i64}, {i64,i32}) — exactly the Clang 19.1.7 output you recorded in-thread. My proposed info.Align > 4 gate would have introduced a divergence. The strengthened TestMSVCAggregateClassification assertions now lock in the correct behavior. The two P2/P3 documentation threads are addressed.

Incremental changes are a net improvement: meta.go adds bounds/validation over mmap'd file-controlled bytes (good hardening), link_options.go preserves DWARF for lld-link, and the goarch goto→continue/break refactor is behavior-preserving.

No blocking or outstanding correctness concerns. (Caveat unchanged: no LLVM headers in this sandbox, so I verified against Clang source + your reproduced output rather than a local build; CI/native smoke/Codecov cover execution.)

[fennoai]

FennoAI Re-review — MSVC aggregate C ABI lowering (head 2baf2d4)

I re-reviewed the locked range 4a5bd76...2baf2d4, focusing on the incremental changes since my last pass (1719198) and on whether the four prior threads were addressed.

Prior P1 finding — I was wrong; retracting it ✅

My earlier P1 claimed that on i686-pc-windows-msvc, over-aligned structs (containing i64/double) must be passed as a plain indirect pointer before the expand/byval paths, and that {i64,double}→AttrExtract and {i32,i64}→byval align 4 diverged from Clang.

That finding was incorrect, and your response is right. I re-read Clang 19's X86_32ABIInfo::classifyArgumentType (release/19.x) line by line. The Win32 over-alignment check is:

if (IsWin32StructABI && State.Required.isRequiredArg(ArgIndex)) {
  // uses Layout.getRequiredAlignment() / TI.isAlignRequired()
  if (AlignInBits > 32)
    return getIndirectResult(Ty, /*ByVal=*/false, State);
}

The alignment it tests is the record's required alignment (getRequiredAlignment() / isAlignRequired()) — i.e. explicit alignas / __declspec(align) — not the natural 8-byte alignment of a long long/double member. So a naturally-aligned struct never trips that branch; it falls through to:

• expand when unpadded and ≤128 bits (canExpandIndirectArgument: Size == getTypeSize(Ty)) → matches AttrExtract, and
• byval align 4 fallback when padded → matches AttrPointer + ByValAlign=4.

That reproduces exactly the Clang 19.1.7 output you recorded in-thread — f(i64, double, ptr byval(B) align 4, ptr byval(C) align 4) for A={long long,double}, B={int,long long}, C={long long,int}. My proposed info.Align > 4 gate would have introduced a divergence from Clang, because it conflated LLVM datalayout alignment (i64:64) with the C AST required alignment Clang actually uses in this check. Thanks for pushing back with reproducible evidence — the current classification is correct, and the strengthened TestMSVCAggregateClassification assertions (ByValAlign == 0 for the expanded case, == 4 for the two padded cases) now lock in the right behavior.

The two P2/P3 documentation threads (isMSVCTarget default-to-MSVC comment; x64 large-aggregate byval-omission comment) are addressed as described.

Incremental changes since 1719198

• internal/meta/meta.go — the new validate() / validateCSRSection / name-range bounds checks over mmap'd, file-controlled bytes are a solid hardening improvement (untrusted metadata now can't drive out-of-range reads), and the uint32-range size guard before mmap is correct. Good addition.
• internal/build/link_options.go — debugInfoLinkerArgs now emits PreserveLinkFlags when DWARF is kept, which is the right fix for linkers like lld-link that drop DWARF unless preservation is explicit.
• internal/goarch/config.go — ParseARM64 goto → continue/break refactor is behavior-preserving and clearer.
• Remaining changes (crosscompile, ssa COMDAT/decl, workflow) are consistent supporting work.

Conclusion

No blocking or outstanding correctness concerns from me. The MSVC x64 / x86 / ARM64 aggregate lowering matches Clang 19, the prior threads are resolved, and the incremental hardening is a net improvement. LGTM.

Caveat unchanged from last time: LLVM headers aren't available in this sandbox, so I verified ABI behavior against Clang source + your reproduced output rather than a local build/test run; CI, native Windows smoke, and Codecov cover the executable paths.

[cpunion]

@fennoai, please re-review the current head e4c6ea7c19cb.

The three C ABI commits were rebased without conflict onto the fully reviewed #2335 head a8fb0fc9e58a. All 43 checks completed without failure (42 passed, 1 conditionally skipped), including LLVM 19 Windows AMD64 host smoke and Linux/macOS full tests; Codecov patch coverage is 98.89%. Native Windows 11 ARM64 validation also passed the complete internal/cabi suite and compiler build/run directly from the shared UNC source tree.

Please focus on MSVC target detection and amd64/arm64/386 aggregate classification, x86 byval align 4 preservation, multi-attribute propagation across definitions/calls/callback wrappers, and compatibility with the rebased COFF/architecture foundation.

[fennoai]

Re-review complete — LGTM

I re-reviewed rebased head e4c6ea7 over the locked range 0f48025...e4c6ea7, diffing the C ABI files against my last confirmed-good head 2baf2d4 to separate rebase churn from real change. Submitted a non-blocking PR review.

Focus areas unchanged & still correct: internal/cabi/arch_msvc.go is byte-identical to the state I confirmed against Clang 19 last round — MSVC target detection, amd64/arm64/386 classification, x86 byval align 4 (expand-vs-byval split), and multi-attribute propagation all carry over verbatim.

New since last review — COFF COMDAT propagation (correct): transformFunc now preserves the source COMDAT (SetComdat(fn.Comdat()), safe across the name-swap and for non-COMDAT fns); callback wrappers and ODR/instantiated definitions get comdat any only on Windows GOOS via the new setODRLinkage helpers (in both cabi.go and ssa/decl.go). This is the right fix — COFF needs ODR/linkonce defs to carry IMAGE_COMDAT_SELECT_ANY so lld-link can coalesce them. New tests assert both Windows-present and Unix-absent behavior and VerifyModule the output.

Minor (non-blocking): two near-identical setODRLinkage helpers (different receiver types) — mild duplication, fine as-is.

Remaining changes are the supporting infra reviewed before plus the mechanical goplus/llgo→xgo-dev/llgo import rename. No blocking or outstanding concerns.

Caveat unchanged: no LLVM headers in this sandbox, so I verified against the diff + Clang/LLVM COFF semantics + in-repo tests; the 42 passing checks, native Windows 11 ARM64 validation, and Codecov cover execution.

[fennoai]

FennoAI Re-review — rebased head e4c6ea7

I re-reviewed the locked range 0f48025...e4c6ea7 (rebased onto the reviewed #2335 head a8fb0fc). To separate rebase churn from real change, I diffed the C ABI files against my last confirmed-good head 2baf2d4.

Focus areas — aggregate classification & byval align 4: unchanged, still correct ✅

internal/cabi/arch_msvc.go is byte-identical to the state I confirmed correct against Clang 19 last round. The MSVC target detection (targetArch / isMSVCTarget, windows && (msvc || !gnu)), the amd64 / arm64 / 386 classification, the x86 byval align 4 preservation (including the expand-vs-byval split for unpadded vs padded i64/double structs), and the multi-attribute (byval + align) propagation all carry over verbatim. No re-litigation needed — the prior Clang-19-verified conclusion stands.

New since last review — COFF COMDAT propagation ✅

The substantive addition in this range is COMDAT handling for the COFF/architecture foundation:

• transformFunc now does nfn.SetComdat(fn.Comdat()), carrying the source function's COMDAT onto the lowered definition. The name-swap ordering is safe: the comdat table entry (keyed by the original name) survives the fn.SetName("") → AddFunction(fname) dance, and SetComdat(nil) is a no-op for non-COMDAT functions.
• transformCallbackFunc routes the linkonce wrapper through the new setODRLinkage, which attaches comdat any only when GOOS == windows. The wrapper is name-deduplicated (m.NamedFunction(wrapName) guard) before creation, and the name is set before setODRLinkage, so value.Name() keys the comdat correctly.
• ssa/decl.go adds a parallel Package.setODRLinkage for the module zerobase, instantiated functions, and ODR globals — same Windows-only comdat any rule.

This is the right fix: COFF requires ODR/linkonce definitions to carry a COMDAT (IMAGE_COMDAT_SELECT_ANY) so lld-link can coalesce otherwise-identical definitions across objects; plain LLVM weak/linkonce alone degrades to per-object weak externals that the linker won't merge. AnyComdatSelectionKind matches the linkonce/weak_odr coalescing semantics. The new tests (TestCABILoweringPreservesWindowsCOMDAT, TestWindowsCallbackWrapperUsesCOMDAT, ssa/coff_comdat_test.go) assert both the Windows-present and Unix-absent behavior and VerifyModule the result.

The remaining changes in the range are the same supporting infra I reviewed before (crosscompile, meta hardening, goarch, link options, workflow) plus the mechanical goplus/llgo → xgo-dev/llgo import rename from the rebase — all consistent.

Minor (non-blocking)

There are now two near-identical setODRLinkage helpers — Package.setODRLinkage (ssa/decl.go) and Transformer.setODRLinkage (internal/cabi/cabi.go) — with the same doc comment and logic on different receiver types. Fine as-is; if the COFF COMDAT convention grows, a single shared helper would avoid drift between the two.

Conclusion

No blocking or outstanding correctness concerns. The rebase is clean, the ABI core is unchanged from the confirmed-correct state, and the new COMDAT propagation is correct and well-tested. LGTM.

Caveat unchanged: LLVM headers aren't available in this sandbox, so I verified against the diff, Clang/LLVM COFF semantics, and the in-repo tests rather than a local build/run; the 42 passing checks + native Windows 11 ARM64 validation + Codecov cover execution.