Skip to content

[depends on #2338] runtime: support native Windows system callbacks - #2352

Closed
cpunion wants to merge 92 commits into
xgo-dev:mainfrom
cpunion:codex/windows-callbacks-20260817
Closed

[depends on #2338] runtime: support native Windows system callbacks#2352
cpunion wants to merge 92 commits into
xgo-dev:mainfrom
cpunion:codex/windows-callbacks-20260817

Conversation

@cpunion

@cpunion cpunion commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Depends on #2338.

Part of #2325.

Summary

  • implement syscall.NewCallback and syscall.NewCallbackCDecl for native Windows through libffi closures
  • select the Go-compatible stdcall ABI on windows/386 while retaining the single native convention on amd64 and arm64
  • complete the raw windows/386 SyscallN stack bridge needed by the same callback/DLL runtime surface, derived from Go's x86 implementation
  • validate callback arguments, results, and the 64-word frame limit with Go runtime-compatible errors and 386 return-slot accounting
  • key cached callbacks by code, closure environment, and calling convention, with synchronized concurrent registration
  • retain LLGo-owned foreign-thread GC registration until FLS teardown so repeated callbacks avoid collector-lock registration churn while preserving GC roots, defer, panic, and recover
  • let runtime.Goexit terminate a foreign callback thread without longjmp unwinding across the libffi frame; the retained registration is released by the Windows G/FLS or GC_ExitThread lifecycle
  • share Go ABI-to-libffi type lowering inside the runtime without introducing a runtime/abi dependency into the low-level FFI package
  • cover cdecl/stdcall selection, pointer-sized aggregates, zero-sized parameters, concurrent registration, and closures that share code but capture distinct environments

Scope

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 SyscallN addition 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/runtime passes from the runtime module
  • go test -count=1 ./internal/env ./internal/crosscompile passes
  • go test -count=1 ./internal/cabi passes (99.354 seconds under concurrent local load)
  • go test -count=1 ./ssa passes (128.619 seconds under concurrent local load)
  • LLVM 19 assembles the raw SyscallN bridge for i686, x86_64, and aarch64 MSVC triples with the expected COFF symbol spelling; the Windows CI job repeats this check

On 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:

  • metadata, architecture, LLVM, build, cross-compilation, SSA, and complete C ABI tests pass (internal/cabi: 105.971 seconds)
  • the native ARM64 LLGo host compiler builds and reports llgo (devel) windows/arm64
  • runtime, standard-library syscall, fault traceback, and reflection/libffi smoke programs pass
  • system callbacks execute from Windows-created foreign threads with GC/finalizer roots, defer, panic/recover, cdecl/stdcall behavior, aggregate and zero-sized arguments, concurrent registration, and distinct captured closure environments
  • one Windows-created thread performs 64 consecutive callbacks with a collection every eighth call, then exits through FLS cleanup; this exercises retained collector registration and final teardown
  • a foreign-thread callback calls runtime.Goexit, runs its deferred function, and terminates through the GC-aware Windows thread-exit path
  • the complete Windows FFI/callback smoke, including the Goexit regression, passes 20/20 consecutive runs

GitHub CI completed all 42 applicable checks successfully on d67c9f7eb (with the normal release job skipped), including native Windows AMD64, Ubuntu, macOS, and codecov/patch at 98.20%.

cpunion and others added 30 commits August 17, 2026 21:31

@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 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.c llgo_win_fls_set: replacing slot->value neither runs the previous value's destructor nor updates slot->destructor. Matches pthread setspecific semantics and today's Go caller always passes the same destructor, so it's latent — worth a one-line comment noting the intent.
  • sync_windows Once.Do: llgo_win_once can return GetLastError() on init failure, but the callback always returns success and the return c.Int is 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 named 22 (POSIX EINVAL); naming the 87 constant would prevent misreads across the two error domains.

Docs (minor)

  • os_windows.go package doc ("exposes the hosted runtime's operating-system C API") is copied from the fuller Unix variants but this file only declares Getenv; scope is overstated.

Comment thread runtime/internal/lib/runtime/syscall_windows_callback_llgo.go
Comment thread runtime/internal/lib/runtime/runtime_windows.go
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

LLGo baseline benchmarks

d67c9f7eb49a | workflow run | long-term charts

Program measurements

Platform Workload File size vs base Build vs base Run vs base
Linux cprintf 19480 B +0.5% (worse) 335.488 ms +1.0% (worse) 1.351 ms -1.5% (better)
Linux fmtprintf 1884048 B +0.2% (worse) 2.596 s -0.1% (better) 3.615 ms -0.3% (better)
Linux println 69640 B +1.1% (worse) 345.195 ms +2.6% (worse) 1.747 ms +2.5% (worse)
macOS cprintf 84672 B +0.0% 598.621 ms +68.0% (worse) 3.708 ms +26.1% (worse)
macOS fmtprintf 1892912 B +0.0% (worse) 3.104 s +27.0% (worse) 14.007 ms -20.6% (better)
macOS println 121856 B +0.4% (worse) 595.527 ms +42.1% (worse) 4.973 ms +11.7% (worse)
Core language and compiler benchmarks
Platform Benchmark ns/op vs base
Linux BenchmarkLookupPCRandom 12.290 ns/op +0.2% (worse)
Linux BenchmarkMergeCompilerFlags 143.800 ns/op -0.7% (better)
Linux BenchmarkMergeLinkerFlags 93.950 ns/op -0.2% (better)
Linux BenchmarkChannelBuffered 36.590 ns/op +0.9% (worse)
Linux BenchmarkChannelHandoff 24669 ns/op +3.9% (worse)
Linux BenchmarkDefer 45.210 ns/op +4.2% (worse)
Linux BenchmarkDirectCall 1.772 ns/op +0.9% (worse)
Linux BenchmarkGlobalRead 1.758 ns/op +0.1% (worse)
Linux BenchmarkGlobalWrite 2.817 ns/op +0.3% (worse)
Linux BenchmarkGoroutine 35740 ns/op +14.6% (worse)
Linux BenchmarkInterfaceCall 8.472 ns/op -7.4% (better)
Linux BenchmarkRuntimeGetG 1.761 ns/op +0.1% (worse)
macOS BenchmarkLookupPCRandom 11.910 ns/op -12.8% (better)
macOS BenchmarkMergeCompilerFlags 119.400 ns/op -0.7% (better)
macOS BenchmarkMergeLinkerFlags 76.940 ns/op +7.8% (worse)
macOS BenchmarkChannelBuffered 32.610 ns/op -3.3% (better)
macOS BenchmarkChannelHandoff 8519 ns/op -30.5% (better)
macOS BenchmarkDefer 31.830 ns/op -33.3% (better)
macOS BenchmarkDirectCall 1.157 ns/op -5.0% (better)
macOS BenchmarkGlobalRead 1.200 ns/op +8.4% (worse)
macOS BenchmarkGlobalWrite 1.326 ns/op +16.6% (worse)
macOS BenchmarkGoroutine 49152 ns/op -15.9% (better)
macOS BenchmarkInterfaceCall 6.853 ns/op -1.8% (better)
macOS BenchmarkRuntimeGetG 2.240 ns/op -21.5% (better)

Compared with 0f480253029a measured in the same runner job.

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.19928% with 15 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/build/cgo_pragmas.go 84.31% 4 Missing and 4 partials ⚠️
internal/crosscompile/crosscompile.go 96.93% 2 Missing and 1 partial ⚠️
internal/build/build.go 96.82% 1 Missing and 1 partial ⚠️
ssa/eh.go 96.49% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@cpunion

cpunion commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

@fennoai Please review current head 287c98829 for the Windows callback stage of #2325.

Please focus on:

  • compatibility with Go 1.26's syscall.NewCallback / NewCallbackCDecl validation and 386 frame/calling-convention semantics
  • the libffi closure bridge, foreign-thread registration, GC liveness, panic/recover behavior, and concurrent cache safety
  • cache identity for closures sharing one code entry but carrying distinct environments
  • keeping Go ABI type lowering runtime-local so the low-level FFI package does not depend on runtime/abi
  • code clarity, memory safety, and callback-path performance

All 42 applicable GitHub checks pass (one normal release skip), including native Windows AMD64 and codecov/patch at 98.20%. The same head passes the complete native Windows 11 ARM64/MSVC + LLVM 19 validation and the FFI/callback smoke 20/20 consecutive runs. The PR is based directly on the fully gated #2338 head and does not depend on later Windows stages.

@fennoai

fennoai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Review — Windows callback stage (#2352)

Reviewed head 287c98829 against base 0f4802530, focused on the callback path (runtime/internal/lib/runtime/syscall_windows_callback_llgo.go and its FFI/thread/type-lowering dependencies). I cross-checked the validation semantics against Go 1.26.0 upstream (src/runtime/syscall_windows.go) and traced the closure/env, foreign-thread, GC, and cache paths end to end.

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 findings

1. Go 1.26 NewCallback/NewCallbackCDecl validation + 386 semantics — matches

  • Panic strings match upstream exactly, including the shared "compileCallback: expected function with one uintptr-sized result" for all three of not-a-function / wrong-result-count / non-uintptr-result, plus "argument size is larger than uintptr", "float arguments not supported", "float results not supported", "function argument frame too large", and the "type <T> is currently not supported for use in system callbacks" format.
  • cleanstack correctly selects stdcall only on 386; force-cleared to false on amd64/arm64 (syscall_compileCallback L230-232), so the single native convention holds there. The cache key includes cleanstack, so 386 gets distinct stdcall/cdecl trampolines while other arches collapse to one — matching upstream's winCallbackKey.
  • 64-word frame limit (windowsCallbackMaxFrame) with the return slot added only on 386 (intArgRegs == 0 in upstream) is reproduced (L148-156). Verified by callbackWith64Args rejecting on 386 (65 > 64) and accepting on amd64/arm64 (64 == 64).
  • int64/uint64 on 386 correctly rejected via the Size_ > uintptr guard; zero-sized args consume neither a Go arg slot nor a C ABI word (cArgIndex[i] = -1), matching upstream's align-and-early-return.

2. libffi closure bridge, foreign-thread registration, GC liveness, panic/recover — sound

  • callWindowsCallback registers the foreign thread (EnterForeignThread) as its first statement, before any Go-heap allocation or the deferred cleanup record, and deregisters via defer only when this call owned the registration (bdwgc Duplicate/Success accounting). Correct ordering.
  • Closure return buffer: cSig return type is TypeUintptr and the closure writes *(*uintptr)(ret). On every Windows target sizeof(ffi_arg) == sizeof(uintptr) (Windows long is 32-bit on both x86/x64), so no under-fill on 386.
  • GC roots / defer / panic-recover across the boundary are exercised by the smoke test (_test/windowsffi/main.go): live-root retention through runtime.GC(), stack-root integrity, and defer+recover state, run 4× on foreign threads.

3. Cache identity for shared code, distinct env — correct

  • Key is {code, env, cleanstack}, so closures sharing one code entry but capturing different environments get distinct trampolines. testDistinctClosureCallbacks confirms two MakeFunc/closure values with the same code pointer but different captured base produce different callback addresses and each preserves its own environment. Concurrent registration is covered by the 8-goroutine race in registerStdcallCallback.

4. Runtime-local ABI type lowering — clean separation

  • goABIFFIType lives in runtime/internal/lib/runtime/mfinal_ffi.go (which may import runtime/abi). Both runtime/internal/ffi (wrapper) and runtime/internal/clite/ffi (low-level) are free of any runtime/abi import — verified by grep. The callback path lowers abi.Type*ffi.Type in the runtime and hands only *ffi.Type to the FFI layer, exactly as the PR intends.

5. Concurrent cache safety — correct

  • Double-checked locking over psync.Once (Windows INIT_ONCE) + psync.Mutex (SRWLOCK, zero-value ready): check under lock → build entry outside lock → re-check under lock → on the losing race, entry.closure.Free() releases the C-allocated libffi closure and the returned Fn is always the winner's (permanently reachable via the map). No dangling Fn, no closure leak; Bind-failure path also frees.

Minor / non-blocking

  • [P3] Frame accounting is stricter than upstream for sub-word args. newWindowsCallbackEntry charges a full uintptr word per accepted arg (L140), whereas upstream packs sub-word scalars tightly into dstStackSize/dstSpill. A pathological function (e.g. 65 uint8 params) that upstream would accept could be rejected here as "frame too large." This is strictly more conservative (never accepts what upstream rejects) and irrelevant for real Windows callbacks, which use word-sized params. Fine to leave; a one-line comment noting the intentional word-granular approximation would help future readers.
  • [P3] windowsCallbackFFIType recurses for validation side-effects only (L105-113 discard the returned *ffi.Type for array/struct members and re-lower the whole type via goABIFFIType at L117). Correct, but the discarded recursive return reads as dead code at a glance — a short comment ("recurse to validate members; final lowering below") would clarify intent.

Notes

  • Validation reported in the PR (42 CI checks incl. native Windows AMD64, codecov/patch 98.20%, and the 20/20 native ARM64 smoke) is consistent with what the code and tests here support; I did not re-run the Windows suite (no Windows host in this environment) and relied on static review plus the checked-in tests.

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.

@cpunion

cpunion commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator Author

@fennoai Please do a focused final review of the follow-up delta 287c98829..f358827aa after your reviews.

The two P2 findings are addressed:

  • d424e8b5a retains an LLGo-owned manual BDWGC registration across repeated callbacks and releases it as the final operation of the existing G lifecycle FLS destructor; a new native smoke performs 64 callbacks on one foreign thread, forces GC every eighth call, waits for FLS/thread teardown, and collects again
  • f358827aa adds the Go-compatible windows/386 raw SyscallN stack bridge and makes Windows CI assemble/check its COFF symbol together with amd64 and arm64

The two focused-review P3 readability points are documented in 5dcc2e6d1; the conservative word-granular frame policy is unchanged.

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, codecov/patch at 98.20%, native Windows 11 ARM64 runtime/FFI validation, and the enhanced callback smoke 20/20.

@fennoai

fennoai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Focused follow-up review — delta 287c98829..f358827aa

Reviewed the three follow-up commits (d424e8b5 retained registration, 5dcc2e6d docs, f358827a 386 SyscallN bridge) against locked head f358827aa. Focus: GC registration ordering/ownership, the x86 stack bridge, and C symbol spelling. I traced the ownership state machine across all thread-lifecycle transitions and checked the i386 assembly against Go's upstream asm_windows_386.s.

Verdict: the two P2 items are correctly addressed and the 386 bridge is sound. One reachable edge-case in the new retention model is worth a look before merge (P2, non-blocking for the happy paths); details below.

x86 SyscallN bridge (f358827a) — correct

Verified line-by-line against upstream asm_windows_386.s:

  • Struct offsets 0/4/8/12/16/20(%ebx) = fn/n/args/r1/r2/err match windowsSyscall on 386; *windowsSyscall correctly read at 8(%ebp).
  • Callee-saved set (ebx, esi, edi, ebp) is fully preserved/restored in balanced LIFO; only eax/ecx/edx used as scratch.
  • Dual-convention stack restore: leal -12(%ebp), %esp recomputes esp absolutely from ebp, so it is immune to whether the callee cleaned its args (stdcall ret N) or not (cdecl) — the exact C-frame analog of upstream's MOVL BP, SP. Correct.
  • SetLastError(0) before / GetLastError after via %fs:0x34 (correct x86 TEB offset), ordering matches upstream; r1/r2 read from eax/edx.
  • rep movsl with cld, esi=args, edi=esp places arg0 at the top of stack — correct outgoing order; n==0 fast path mirrors upstream.
  • Symbol spelling: _llgo_windows_syscall (leading underscore) is correct for i386 MSVC/COFF; amd64/arm64 use the undecorated llgo_windows_syscall. The CI matrix in windows.yml checks each triple against the right spelling via llvm-nm --defined-only. Absence of .seh_* on i386 is correct (x86 uses FS-chain SEH, not table-based unwind).

GC registration retention (d424e8b5) — mostly sound, one reachable orphan

The retain-until-FLS-destructor model is correct on the paths the new smoke exercises. Verified:

  • Repeated callbacks on one foreign thread — register once (flag + lazy getg lifecycle), subsequent entries early-return, single UnregisterMyThread from destroyG at thread exit. Exactly-once. ✓
  • getg/setAutoG failure in EnterForeignThread — the defer{ if !ready release } guard releases exactly once; destroyG in the failure path sees currentGHasLifecycle==false (set only on setAutoG success) so it does not double-release. ✓
  • Runtime-created (GC_CreateThread) threads — the currentG != 0 && !currentGHasLifecycle guard returns false without registering; nothing to leak. ✓
  • Per-thread isolationforeignThreadGCRegistrationOwned, currentG, currentGHasLifecycle are all //llgo:tls; no cross-thread interference. ✓
  • ExitForeignThread fallback — unreachable on the happy path (a registered==true entry always installs the lifecycle) but harmless and idempotent. ✓

[P2] runtime.Goexit() from inside a callback orphans the retained registration. If Go code running inside a NewCallback/NewCallbackCDecl function on a foreign thread calls runtime.Goexit() (legal user code), the defer unwind reaches exitCurrentM → mexit(...) (os_windows.go:41, proc.go:150) → setg(nil). In setg (g_tls.go:72-89) the FLS key is cleared (:75) and currentGHasLifecycle is set false (:79) before destroyG(old) runs (:85). destroyG's new release is guarded by if currentGHasLifecycle (g_tls.go:121), which is now false, so releaseForeignThreadRegistration() is skipped — and the FLS destructor that would otherwise catch it was just torn down. Result: UnregisterMyThread is never called (leaked bdwgc thread record), and thread.Exit()/GC_ExitThread (os_windows.go:44) then runs on a still-registered foreign thread that GC_CreateThread did not create.

  • Reachable with legal user code, though an unusual pattern; the new 64-callback smoke returns normally and never Goexits, so it doesn't cover this.
  • Root cause is the ordering in setg: the lifecycle flag is cleared before destroyG consults it. Options: release the foreign registration in setg's orphan branch before flipping currentGHasLifecycle=false; or thread the ownership into destroyG independent of the just-cleared flag; or route a lifecycle-owned foreign G's mexit teardown through the same release the FLS destructor uses.
  • Worth confirming whether mexit-from-callback is considered in-scope for this stage. If callbacks are contractually expected to return normally (never Goexit / os.Exit-style unwind), a documented precondition plus an assertion would also close it.

Docs (5dcc2e6d) — good

The two readability points are addressed: the conservative word-granular frame policy and the validate-then-lower recursion in windowsCallbackFFIType now carry clarifying comments. Policy unchanged, as intended.

Notes

I did not re-run the Windows suite (no Windows host here); this is static review against the code plus the checked-in tests and Go upstream. CI status reported (42 checks, native AMD64, codecov/patch 98.20%, ARM64 validation, 20/20 smoke) is consistent with the happy-path behavior I verified.

Net: the delta is solid work — the 386 bridge is a faithful upstream adaptation and the registration retention removes the per-callback collector-lock churn correctly for normal callback flow. Recommend addressing or explicitly scoping-out the Goexit-from-callback orphan; everything else is merge-ready (pending #2338).

@cpunion

cpunion commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

@fennoai I checked the runtime.Goexit path against the actual Windows BDWGC teardown and added the counterexample as a regression in d67c9f7.

The registration is not orphaned after setg(nil): exitCurrentM immediately calls thread.Exit, and the GC-enabled Windows backend dispatches that to GC_ExitThread. BDWGC implements GC_ExitThread as GC_unregister_my_thread(); ExitThread(...) (upstream source). Releasing again from destroyG would therefore double-unregister this path.

Running the real counterexample on native Windows 11 ARM64 did expose a different problem: the callback wrapper's deferred ExitForeignThread forced Goexit to Windows-longjmp across the libffi callback frame and terminated with STATUS_BAD_FUNCTION_TABLE. Since retained registrations are now lifecycle-owned, the cleanup does not need a defer. The wrapper now calls ExitForeignThread only on normal return; Goexit uses the G/FLS plus GC_ExitThread teardown. The new test verifies that Goexit from a Windows-created foreign thread runs its defer and terminates the thread. Runtime/fault/stdlib/FFI smoke passes, and the complete FFI smoke passes 20/20 on native Windows ARM64.

@fennoai

fennoai Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Re-examining the Goexit finding — delta f358827aa..d67c9f7eb

You're right, and my earlier P2 conclusion was wrong. I verified your rebuttal against the BDWGC source and traced the new teardown path end to end. The registration is not orphaned, and the fix in d67c9f7eb is correct.

Where my prior finding went wrong

My mechanism trace was accurate — Goexit → exitCurrentM → mexit → setg(nil) does clear currentGHasLifecycle before destroyG(old) runs, so destroyG does not release the registration. But I stopped there and wrongly concluded "leak." I missed the very next step: exitCurrentM (os_windows.go:41-45) calls thread.Exit() immediately after mexit, and on the GC-enabled Windows backend thread.Exit() dispatches to GC_ExitThread (thread_windows.c:104).

Confirmed against bdwgc v8.2.12 win32_threads.c:2743-2747:

GC_API DECLSPEC_NORETURN void WINAPI GC_ExitThread(DWORD dwExitCode) {
  GC_unregister_my_thread();
  ExitThread(dwExitCode);
}

So the retained registration is released — by GC_ExitThread, not by destroyG. The exactly-once invariant holds on the Goexit path; it's just discharged through the OS-thread-exit route rather than the FLS destructor.

And my suggested "fix" would have introduced a real bug

Had destroyG/ExitForeignThread also released on this path, that would be a second GC_unregister_my_thread() on a native-Win32 thread. bdwgc's non-pthreads GC_unregister_my_thread → GC_delete_thread walks the GC_threads hash chain assuming the entry is present (win32_threads.c: "We assume it is there. The code intentionally traps if it wasn't.") — the second call would walk off the chain and crash, by design. So keeping the release off destroyG for the lifecycle-owned path is exactly correct, not a leak. Good catch rejecting it.

The fix itself (d67c9f7eb) — correct and minimal

  • Moving ExitForeignThread(registered) off the defer chain to trailing straight-line code is the right call. On runtime.Goexit, the defer unwind must not run ExitForeignThread, because forcing that unwind's Windows longjmp across the libffi callback C frame (no unwind table) is what produced STATUS_BAD_FUNCTION_TABLE. As plain trailing code it runs only on normal return; Goexit unwinds through Go frames and hands teardown to GC_ExitThread.
  • Path accounting after the change:
    • Normal returnExitForeignThread(true) sees currentGUsesLifecycle()==true → retains; released later by the FLS destroyG at thread exit. One unregister. ✓
    • Goexit → line 224 not reached; mexit→setg(nil) deliberately does not release; GC_ExitThread unregisters and terminates. One unregister. ✓
    • Either way, exactly one UnregisterMyThread/GC_unregister_my_thread, no double-free.
  • The residual foreignThreadGCRegistrationOwned==true on the terminating thread is harmless (per-thread FLS/TLS on a dying thread, never consulted again).

Regression test — good coverage

testForeignThreadCallbackGoexit exercises the exact counterexample: a cdecl callback that defers a flag-set then calls runtime.Goexit(), run on a real CreateThread foreign thread that the harness joins with WaitForSingleObject. It asserts the defer ran (deferred==true) and the join succeeded (errno==0), i.e. GC_ExitThread cleanly terminated the thread with exit code 0 rather than crashing. A runtime.GC() afterward would surface a corrupted collector thread table if the unregister had gone wrong. This directly covers what my earlier analysis speculated about.

Conclusion

No outstanding correctness concerns on the callback stage. To recap the full review arc: Go 1.26 validation/386 semantics ✓, libffi closure + env + foreign-thread/GC/panic-recover ✓, cache identity + concurrency ✓, runtime-local ABI lowering ✓, the 386 SyscallN bridge ✓, and the registration-retention model — including the Goexit teardown — now verified correct against the actual BDWGC Windows behavior. LGTM; recommend merge once the #2338 dependency lands.

Validation reported (native Windows ARM64 runtime/fault/stdlib/FFI smoke, FFI 20/20) is consistent with the code; I did not re-run the Windows suite here.

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