[depends on #2338] runtime: support native Windows system callbacks - #2352
[depends on #2338] runtime: support native Windows system callbacks#2352cpunion wants to merge 92 commits into
Conversation
There was a problem hiding this comment.
Review: Windows support
Reviewed the Windows support additions (~7.7k LOC): FFI/threading/syscall/callback bridges, SEH-based unwinding, fault handling, cross-compile toolchain fetch, and the meta parser. Overall this is high-quality systems code: C shims are well-commented, CONTEXT/ABI records carry compile-time size/offset assertions to guard against ABI drift, syscall thunks bound argument counts against exact stack-frame sizes, and the fault/unwind paths guard every raw pointer read with alignment + readability checks. No blocking correctness or memory-safety defects were found in the new code.
Findings below are surfaced as an FYI (non-blocking). Two are inline; the rest are here because they touch pre-existing or cross-file context.
Pre-existing gap now more reachable — zip-slip in extractZip (internal/crosscompile/fetch.go, ~line 288)
extractZip does path := filepath.Join(dest, file.Name) with no traversal guard, whereas extractTarGz in the same file correctly rejects entries escaping dest (strings.HasPrefix(target, filepath.Clean(dest)+sep)). A malicious/compromised .zip with ../ entries could write outside the extraction dir. This path is reachable (e.g. picolibc is fetched as .zip). Not introduced by this PR, but since it broadens the Windows toolchain-fetch surface, consider mirroring the extractTarGz guard in extractZip.
Contract robustness (confirm, low priority)
thread_windows.cllgo_win_fls_set: replacingslot->valueneither runs the previous value's destructor nor updatesslot->destructor. Matches pthreadsetspecificsemantics and today's Go caller always passes the same destructor, so it's latent — worth a one-line comment noting the intent.sync_windowsOnce.Do:llgo_win_oncecan returnGetLastError()on init failure, but the callback always returns success and the returnc.Intis easy to ignore at call sites. Confirm callers need not check it, or document so.- Magic literal
87(ERROR_INVALID_PARAMETER) appears bare in three thread files while the sync shim uses a named22(POSIXEINVAL); naming the87constant would prevent misreads across the two error domains.
Docs (minor)
os_windows.gopackage doc ("exposes the hosted runtime's operating-system C API") is copied from the fuller Unix variants but this file only declaresGetenv; scope is overstated.
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 current head Please focus on:
All 42 applicable GitHub checks pass (one normal release skip), including native Windows AMD64 and |
Review — Windows callback stage (#2352)Reviewed head Verdict: LGTM. No correctness or memory-safety issues found. One strictly-conservative behavioral divergence and a couple of minor notes below — none blocking. Focus-area findings1. Go 1.26
2. libffi closure bridge, foreign-thread registration, GC liveness, panic/recover — sound
3. Cache identity for shared code, distinct env — correct
4. Runtime-local ABI type lowering — clean separation
5. Concurrent cache safety — correct
Minor / non-blocking
Notes
This is well-structured, well-tested work that faithfully mirrors Go 1.26's contract while reusing LLGo's libffi/foreign-thread facilities. Recommend merge after the dependency (#2338) lands. |
|
@fennoai Please do a focused final review of the follow-up delta The two P2 findings are addressed:
The two focused-review P3 readability points are documented in Please focus on GC registration/unregistration ordering and ownership (including runtime-created threads and failure cleanup), plus the x86 stack restoration, callee-saved registers, LastError/result handling, and C symbol spelling. Current head passes all 42 applicable GitHub checks (one normal release skip), native Windows AMD64, |
Focused follow-up review — delta
|
|
@fennoai I checked the The registration is not orphaned after Running the real counterexample on native Windows 11 ARM64 did expose a different problem: the callback wrapper's deferred |
Re-examining the Goexit finding — delta
|
Depends on #2338.
Part of #2325.
Summary
syscall.NewCallbackandsyscall.NewCallbackCDeclfor native Windows through libffi closuresSyscallNstack bridge needed by the same callback/DLL runtime surface, derived from Go's x86 implementationruntime.Goexitterminate a foreign callback thread without longjmp unwinding across the libffi frame; the retained registration is released by the Windows G/FLS orGC_ExitThreadlifecycleruntime/abidependency into the low-level FFI packageScope
This PR is the callback stage of the Windows proposal. It depends only on the native runtime core in #2338; Windows process, network, and broader standard-library integration remain separate follow-up work. The small 386
SyscallNaddition closes the architecture build blocker found while reviewing the callback's stdcall/cdecl path; broader 386 qualification remains in the proposal's dedicated phase.The implementation follows the public contract and validation semantics of Go 1.26's Windows callback runtime while using LLGo's existing libffi and foreign-thread facilities instead of copying the upstream compiler-specific callback assembly.
Validation
On macOS ARM64 with Go 1.26.5 and LLVM 19:
go test -count=1 ./internal/ffi ./internal/lib/runtimepasses from the runtime modulego test -count=1 ./internal/env ./internal/crosscompilepassesgo test -count=1 ./internal/cabipasses (99.354 seconds under concurrent local load)go test -count=1 ./ssapasses (128.619 seconds under concurrent local load)SyscallNbridge fori686,x86_64, andaarch64MSVC triples with the expected COFF symbol spelling; the Windows CI job repeats this checkOn a native Windows 11 ARM64 VM with Go 1.26.5, MSVC Build Tools, and LLVM 19.1.7, building directly from the macOS worktree through the Parallels share:
internal/cabi: 105.971 seconds)llgo (devel) windows/arm64runtime.Goexit, runs its deferred function, and terminates through the GC-aware Windows thread-exit pathGoexitregression, passes 20/20 consecutive runsGitHub CI completed all 42 applicable checks successfully on
d67c9f7eb(with the normal release job skipped), including native Windows AMD64, Ubuntu, macOS, andcodecov/patchat 98.20%.