windows: add the Win32 runtime and callback substrate (R4) - #2404
Conversation
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@fennoai please review this Windows runtime/callback substrate after the full CI and patch-coverage gates passed. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Windows Win32 runtime and callback substrate (R4)
Overall this is a high-quality, carefully engineered PR. Four review passes (quality, performance, security, docs) found no critical correctness bugs and no security vulnerabilities. The platform abstractions (filelock, mmap, tls/sync/thread backends) are cleanly split by build tag, the hand-written Windows syscall assembly faithfully adapts Go's own with correct SEH unwind data and stack alignment, argument-count bounds are enforced against the assembly frame sizes, HeapAlloc/HeapFree lifecycles are leak/double-free free, and response-file quoting correctly implements both GNU and CommandLineToArgvW escaping rules. Test coverage is strong.
Only a few low-severity items are worth noting; none are blocking. Two are inline. A few informational notes:
- FLS destructor immutability (
thread_windows.c,llgo_win_fls_set): on the update path (slot != 0, non-nil value)slot->valueis refreshed butslot->destructoris not. Benign today (destructor is fixed at key-create time), but a latent inconsistency — consider refreshing it or documenting immutability. - NULL-guard consistency (
_wrap/runtime_windows.c):llgo_load_library/llgo_get_proc_address/llgo_walltimedereference their error/output pointers without a NULL check, unlike sibling wrappers inthread_windows.c/sync_windows.c. Not exploitable (only reached from trusted runtime shims), but a defensive check would be consistent. FFI_WIN64value comment (ffi/abi_windows_amd64.go= 1,ffi/abi_windows_arm64.go= 2): both are correct per libffi's per-arch enum, but a side-by-side reader may suspect a copy-paste error. A one-line "value is architecture-specific" note (as done forWindowsStdcallABI) would help.ParseARMfloat-suffix parsing (internal/goarch/config.go): stripping,softfloatthen,hardfloatis order-sensitive for malformed inputs containing both suffixes. No real impact; rejecting when both are present would be cleaner.
Review completeI reviewed the full diff (178 files, ~8.3k additions) across four passes — code quality, performance, security, and documentation accuracy — and submitted findings as a PR review. Summary: A high-quality, carefully engineered PR. Arch-specific constants (libffi ABI enums, 5 non-blocking findings (see the review for inline detail):
|
|
Addressed the remaining informational review notes in 42624f8 where they improve correctness or clarity: FLS updates now keep the stored destructor synchronized, the architecture-specific libffi ABI values are documented, and malformed GOARM values containing both float suffixes are rejected. The |
There was a problem hiding this comment.
Review: Windows R4 runtime and callback substrate
Reviewed the full diff (178 files, ~8.3k additions) across four passes: code quality, performance, security, and documentation accuracy.
Overall this is a high-quality, carefully engineered PR. The architecture-specific constants are consistently correct across 386/amd64/arm64 and are backed by tests:
- libffi ABI enum values (386
STDCALL=2/MS_CDECL=5, amd64WIN64=1, arm64WIN64=2) - UCRT
jmp_bufsizes (64/256/192 bytes) - the variable-width
SyscallNassembly bridge correctly sizes stack for the 42-argument limit on all three arches, with proper Win64 shadow-space/xmm handling and the x86 dual-cleanup convention SyscallNbounds-checks argument count (n > len(args)andn > 42) before dereferencing- the
NewCallbackclosure cache uses check-lock-recheck, frees the loser's closure on a registration race, and keeps GC roots alive — matching upstream Go's never-freed-callback semantics - COFF COMDAT emission for ODR globals is the right fix for
lld-linkand is well-tested
Security pass: no memory-safety or trust-boundary findings. The [65]unsafe.Pointer callback frame buffer is correctly guarded with a heap fallback; setjmp/libuv struct sizes are correctly sized; C shims null-check handles and guard the FLS destructor against process-exit re-entry; the CI workflow uses contents: read on pull_request and interpolates no untrusted PR content. The reflect/value.go change (ret = &v → ret = v) is itself a genuine fix for a multi-word-return stack overflow.
The findings below are non-blocking: one performance improvement, one consistency question, and minor comment-accuracy nits.
42624f8 to
3392c92
Compare
cpunion
changed the title
3392c92 to
b605148
Compare
|
@fennoai please re-review the final Windows R4 runtime/callback substrate after the target-isolation cleanup. All CI checks now pass, patch coverage is 90.00%, the Windows native runtime/FFI/GC/wall-clock validation passes, and the same-runner Linux/macOS benchmarks show no binary-size growth. |
There was a problem hiding this comment.
Review summary
Large, well-structured port adding Windows runtime support (~4.4k lines across the compiler build path, the C-lite runtime, and the low-level runtime). Reviewed for code quality, performance, security, and documentation accuracy via four focused passes.
Overall: no correctness-blocking or security defects found. The work is unusually disciplined:
- Build-tag splits across the many
_windows.go/_default.go/_unix.gofiles are consistent and mutually exclusive; symbols are defined exactly once per target (verified themakefunc_callback_windows.govsmakefunc_impl_default.goDe Morgan complements and thenativeKeyDestructorfunc-vs-alias split). - ABI-sensitive pieces are handled carefully and backed by tests: syscall bridge frame sizes bound the arg count on the Go side before entering the
.Sstub (no overflow), the FFI callback path bounds frame size and falls back to a heap slice for >65 args, and setjmp buffer sizes/alignment are per-arch. - CI is not exposed to fork-PR secret theft:
windows.ymlusespull_request(notpull_request_target) withpermissions: contents: read, and the added PowerShell steps interpolate no attacker-controlledgithub.event.*fields. - The prior
panic("TODO: ticksPerSecond for windows")is genuinely resolved (trace clock normalized to ns), with no stale TODOs left behind.
The findings below are all minor / non-blocking (nits and one informational perf note).
Also noted (no inline): runtime/internal/clite/os/dir_unix.go now mixes a modern //go:build line with legacy // +build lines; stat.go in the same PR updates both forms consistently. Consider dropping the redundant // +build lines in dir_unix.go for consistency with the new files, since a drift between the two would trip gofmt/vet.
Re-review completeRe-reviewed the updated diff (120 files, ~4.4k additions; range All 5 findings from the prior review are resolved ✅
This revision is clean. No new issues from the target-isolation cleanup:
2 low-priority (P3) nits:
Design note (not a defect): Nothing blocking. Note: this was a review-only task; I did not run the build/test suite — you confirmed CI, 90% patch coverage, and the native/benchmark validation already pass. |
|
Review follow-up: the two inline documentation nits are fixed in 513804e and all three threads are resolved. I kept the legacy |
There was a problem hiding this comment.
Re-review: Windows R4 runtime and callback substrate (post target-isolation cleanup)
Re-reviewed the updated diff (120 files, ~4.4k additions; authoritative range 1f8cd64...656a9c9) across four passes — code quality, performance, security, documentation — with attention to what changed since the prior review.
All 5 findings from the previous review are resolved:
llgo_nanotimenow caches the QPC frequency viallgo_nanotime_initinstead of querying it per call.- The callback foreign-thread exit comment was rewritten to state the real rationale (normal returns call
ExitForeignThreadto keep the non-lifecycle fallback correct; Goexit leaves via the G lifecycle), andreflect/makefunc.gowas refactored so the two paths no longer diverge. syscall_syscalln'sr2doc now describes per-arch behavior precisely (FP on amd64, high word on 386, zero on arm64).- The
MutexAttrcomment now correctly attributes rejection toSetType/Mutex.Init. llgo_win_fls_setnow updatesslot->destructoron slot reuse, with a clarifying comment.
This revision is clean. The target-isolation cleanup (ssa/eh.go, abitype.go, the native_backend_*/sync_backend_* alias split, and the foreign_thread_gc_windows/foreign_thread_stub_windows partition) is well-structured:
- Security pass: no new memory-safety, trust-boundary, injection, or ABI/frame-sizing issues.
SyscallNbounds are enforced (n > len(args),n > 42); the.Sbridge stack frames match; setjmpjmp_bufsizes (64/256/192) and the callback frame buffer remain correct; the CI workflow changes pin dependency versions and pass no untrusted PR content into shell/PowerShell. - Performance pass: no meaningful concerns. Per-callback GC registration short-circuits on the
foreignThreadGCRegistrationOwnedTLS flag before touching the collector; the closure cache uses double-checked locking with entry construction outside the lock; the pre-Go1.26 syscall wrappers are//go:nosplitso their variadic backing array stays on the stack. - Build-tag partitioning across
_unix/_windows/_default/pre_go126variants is complete and non-overlapping (no missing platform, no duplicate symbols).
Two low-priority documentation/robustness nits remain, noted inline. Neither is blocking.
One design note (not a defect): windowsCallbackMaxFrame charges one full word per non-zero argument, a deliberately conservative approximation of Go's more tightly-packed callback frame. This can reject some dense sub-word signatures on windows/386 that upstream syscall.NewCallback would accept. The code documents this as intentional; worth keeping in mind as a compatibility gap rather than an exact port.
|
Addressed the remaining non-inline dynamic-import review note in e87c370. |
dea79a0 to
e87c370
Compare
Provide direct Unix and Win32 thread, TLS, synchronization, atomic, stdio, setjmp, libuv, and OS adapters. Keep target-specific libraries and layouts isolated so Windows support does not add code to existing Unix binaries.
Lower PE/COFF dynamic imports, align the UCRT setjmp ABI, terminate through runtime.exit, and keep LLGO_ROOT/module parsing portable across CRLF checkouts. Cover the compiler paths and malformed target flags.
Select the hosted thread, synchronization, and atomic backends; add Windows process, clock, syscall, GC, setjmp, and lifecycle adapters; and retain the existing Unix behavior behind target-specific files. Reuse the Go Windows syscall wrapper surface and isolate Win32 code from other targets.
Implement syscall.NewCallback with cached libffi closures, Go-compatible signature validation, closure-identity keys, and 386 calling conventions. Route both syscall and reflect callbacks through the retained foreign-thread GC lifecycle so repeated callbacks avoid collector registration churn.
Run native PE/COFF runtime, FFI, GC, callback, thread, clock, import, and compiler-path smoke tests under the shared dependency setup. Keep the empty-program import audit alongside the feature tests so unintended runtime dependencies stay visible.
8e4f3c1 to
7c94433
Compare
There was a problem hiding this comment.
Review: Windows R4 runtime support 
Large, well-structured PR bringing up the Windows runtime (threading, TLS/FLS, SRW-lock sync, syscall/callback bridges, setjmp ABI, foreign-thread GC registration). Build-tag partitioning (default/unix/windows + arch splits) is consistent, C/asm shims clean up handles and heap allocations on both success and failure paths, and test coverage (FFI, GC, threads, setjmp sizes) is thorough. No blocking correctness defect found in the concurrency, atomics, or syscall-lowering paths.
The notes below are non-blocking. Inline comments cover the specific locations; a few cross-cutting observations follow.
Cross-cutting / no reliable single line:
-
Performance —
sync.Once/Mutex/TLS route through non-inlinable C shims. On Windows,(*Once).Docalls intollgo_win_once→InitOnceExecuteOnceon every invocation (not just first), andMutex.Lock/UnlockwrapAcquireSRWLockExclusive/Releasevia out-of-line C calls. These primitives sit undergetSemaState(sema_llgo.go), which backs all Go channels/mutex/WaitGroup blocking, so the per-op Go→C crossing is on a genuinely hot path. Consider a Go-side atomic fast-path inOnce.Doand//go:linknamedirectly to the SRW ops (as the pthread path does) to drop one indirection per lock/unlock.llgo_win_fls_setalso does aHeapAllocper new TLS slot with an extra pointer chase on everyGet; worth confirming that indirection is off thegetgfast path. -
Unverified struct sizes. The hard-coded libuv handle sizes in
sizes_windows.go/sizes_windows_386.go(uvHandleSize=96,uvAsyncSize=224,uvTimerSize=160,uvSignalSize=264, and 386 variants) are not covered by any test in this PR, unlike the setjmp/atomic sizes whichtypesize_test.govalidates on the native host. If a value is wrong the runtime silently corrupts memory. Consider a compile-time size assertion or a note pinning them to the packaged libuv version (1.52.1). -
Supply chain (informational).
.github/actions/setup-deps/action.ymlinstalls MSYS2 packages viapacman -U <url>fromrepo.msys2.orgwith pinned versions but no repo-side sha256 pin (trust rests on the MSYS2 keyring). Consistent with the existing pattern; flagged for awareness. -
arm64 syscall bridge unwind (low-confidence). In
_wrap/syscall_windows.S, the arm64.seh_startepilogue/.seh_endepilogueblock appears to repeat prologue-style save directives (.seh_save_regp,.seh_save_fplr). CI only checks the symbol name, not unwind-table validity, so a malformed epilogue record could pass CI yet break stack unwinding through this frame during a Windows exception. Worth diffing the generated.xdata/.pdataagainst a known-good MSVC/Go epilogue.
| value = strings.TrimSuffix(value, softFloat) | ||
| } | ||
| if strings.HasSuffix(value, hardFloat) { | ||
| } else if strings.HasSuffix(value, hardFloat) { |
There was a problem hiding this comment.
Behavior change to non-Windows code path, buried in a Windows PR. Switching from if to else if changes ParseARM semantics: "7,hardfloat,softfloat" previously parsed successfully (both suffixes stripped) and now returns an error — config_test.go was updated to expect wantErr: true for exactly that case. This is arguably a correctness improvement (rejecting contradictory float suffixes), but it changes an existing ARM-parsing path unrelated to Windows and is easy to miss in a 6600-line diff. Please confirm it's intentional and consider calling it out in the PR description.
|
|
||
| import _ "unsafe" | ||
|
|
||
| const windowsSyscallMaxArgs = 42 |
There was a problem hiding this comment.
windowsSyscallMaxArgs = 42 is coupled by prose only to three separate hand-written assembly frame sizes in _wrap/syscall_windows.S (amd64 subq $360, arm64 sub sp, #304 / 34 stack args, 386 dynamic). If this constant is bumped without adjusting the .S frames, the bridge would silently overflow the outgoing-argument area (stack corruption) instead of panicking. Consider a comment on each .S frame stating the derived max-arg count, or a CI/compile-time assertion tying the constant to the frame sizes.
| @@ -1,4 +1,5 @@ | |||
| //go:build !(darwin && amd64) | |||
| //go:build !windows && !(darwin && amd64) | |||
There was a problem hiding this comment.
Mixed build-tag syntax: the //go:build !windows && !(darwin && amd64) line is correct, but the two legacy // +build lines below are ANDed to !windows && (!darwin || !amd64). They happen to be equivalent here, but the dual-syntax is easy to get wrong (stat.go in this same PR was updated cleanly). Consider dropping the legacy // +build lines or aligning them, since they only matter for pre-1.17 toolchains.
2 participants
Summary
Implements the R4 Win32 runtime and callback substrate from #2325:
CreateThread, FLS, SRW lock, condition-variable, andINIT_ONCEbackends;QueryPerformanceFrequencyonce at runtime startup so the monotonic-clock hot path performs onlyQueryPerformanceCounter, and derive the CPU count from the process affinity mask like Go's Windows runtime;syscall.SyscallNbridges for Windows 386/amd64/arm64 and reuse the Go 1.26 Windowssyscallwrappers;//go:cgo_import_dynamicpointer declarations to COFFdllimportaddresses, so the upstream syscall package initializes without LLGo-specific replacements;syscall.NewCallback/NewCallbackCDeclwith closure-identity caching, native calling conventions, argument/result validation, GC-root retention, panic/recover, defer, andruntime.Goexitbehavior;runtime.exit/ExitProcessaftermainreturns, matching Go even when another goroutine remains blocked;R5 remains responsible for Windows hardware-fault recovery and SEH-backed traceback unwinding. R6 will add the broader Windows OS/files/process/network standard-library surface; neither concern is hidden or skipped here.
Compatibility and non-Windows impact
CLOCK_REALTIMEadaptation are selected by build tags instead of adding helper calls or retained metadata to Unix programs.cprintfandprintlnfile/text sizes exact with and without LTO.fmtprintfis 16 B smaller as a non-LTO file and unchanged with LTO; removing artificial//lineoverrides changes the reported text metric by only +16 B/+32 B, without increasing either executable file.cprintf,println, andfmtprintffile/text measurements exact with and without LTO.kernel32, andlibgc/libffiwhen used), notmsys-2.0.dll,cygwin1.dll,libwinpthread,libgcc_s, orlibstdc++.Validation
runtime/internal/clite/time,runtime/internal/lib/reflect, andruntime/internal/lib/runtime;TestRecoverDeferredReflectMakeFuncandTestReflectMakeFuncGoroutineStartup;TestReflectTypeMetadataMakeFuncProbeacceptance test;cprintf,println, andfmtprintf, with and without LTO.internal/buildWindows dynamic-import/main-exit pipeline (including alternate patched syntax) and every supported Windows setjmp/sigjmp ABI path inssa; Codecov reports 92.59% patch coverage forcgo_pragmas.goand 96.49% forssa/eh.go.