windows: implement native fault and SEH execution semantics (R5, depends on #2404) - #2405
windows: implement native fault and SEH execution semantics (R5, depends on #2404)#2405cpunion wants to merge 48 commits into
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! |
d478989 to
cbc9399
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.
06c8540 to
54685c5
Compare
|
@fennoai please review the rebased R5 Windows fault/SEH implementation. All CI checks pass (42 successful; the release job is skipped as expected), the Codecov patch check passes, and all 40 R5 commits are patch-identical after rebasing onto #2404s consolidated history. |
|
FennoAI is processing your request, please wait a moment... |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Windows native fault & SEH execution semantics (R5)
This is a high-quality, carefully-reasoned change. Strong points worth calling out: the compile-time ABI assertions pinning CONTEXT size/offsets in fault_context_windows_{amd64,arm64}.go (excellent guard against silent ABI drift), copying the OS-owned exception CONTEXT before the SEH walk rather than mutating Windows' record, correct per-arch nil-call recovery (prepareNilCallUnwind), and consistent defensive frame-pointer guards (alignment, memReadable, stride/frame caps). Platform selection is clean: the store-site metadata and COFF machinery are correctly gated to GOOS == "windows" / siteObjectCOFF, consistent with the byte-identical Linux/macOS output claim.
I ran four review passes (quality, performance, security, docs). No critical or blocking issues. Two inline findings below; a few advisory notes follow.
Advisory (no reliable single inline line):
- Fragile fixed 3-frame unwind in
recoverFrameMarks—runtime/internal/lib/runtime/unwind_windows_seh_llgo.go: the loop unwinds exactly 3 frames on the assumptionrecoverFrameMarks -> recoverMark -> Recover -> deferred func. This holds today only becauserecoverFrameMarksis//go:noinline,recoverMarkis reached via an indirect function value, andRecoveris a real runtime call.recoverMarkitself is not marked//go:noinline; a future refactor that inlines any of these would silently point the mark at the wrong frame with no assertion to catch it. Consider//go:noinlineonrecoverMarkand/or a sanity check on the resulting symbol. - 386
fpCallersdereferences the initial FP before validating it —runtime/internal/lib/runtime/unwind_windows_386_llgo.go: the first iteration reads*(fp)/*(fp+wordSize)with the alignment check applied only afterward and nomemReadableguard, unlike the fault-pathwindowsFPWalkFrom. Not exploitable here (walks its own live stack from a trusted context), but aligning it with the guarded pattern would harden it. asmQuoteELFSymbolappears unused and its$escaping is inconsistent —internal/build/funcinfo_table.go: no production caller (only referenced from a test); its\$escape differs from the$$doubling used on the COFF push-section path. Confirm whether it should be wired in or removed.- Performance (cold path, informational):
RtlLookupFunctionEntryis called per frame with a NULL history table (Go threads anUNWIND_HISTORY_TABLEto amortize this), andmemReadableissues aVirtualQuerysyscall per probe. Both live only on the Windows fault/traceback path with hard frame-count bounds, so impact is bounded — noting only in case stack capture ever becomes hot on Windows. - Suggested test: the Go-vs-foreign-fault distinction relies on
currentGbeing zero on any thread that never entered Go. A regression test asserting that a fault on a pure-native thread continues the handler chain (rather than being converted to a Go panic) would lock in this load-bearing invariant.
|
Addressed the full review in three follow-up commits:
Local validation passed on Parallels Windows 11 ARM64 (native runtime smoke including both new regressions), Windows/386 package compilation, all three Windows MSVC C targets, and the affected compiler/metadata tests on macOS. I will request re-review after the new CI run is green. |
|
@fennoai please re-review the three follow-up commits described above. The complete rerun is now green across Windows, Linux, macOS, benchmarks, and Codecov; all review threads are resolved. |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Re-review: follow-up commits (delta 54685c5e..ad6ae693)
I re-reviewed the three follow-up commits stacked linearly on the previously-reviewed head (no rebase; 54685c5e is an ancestor of ad6ae693). Delta is 8 files, +101/-52. All three findings from my prior review are resolved correctly, and the follow-up also added the regression test I suggested. No new issues; nothing outstanding.
9535e8b1 runtime: complete Windows fault classification — resolves the P2 EXCEPTION_INT_OVERFLOW finding and the P3 contradictory-comment finding:
- The lossy C-side
signalmapping is removed; the raw exceptioncodeis now threaded end-to-end (llgo_fault_callback→onWindowsFault/panicWindowsException), so classification lives in Go where it belongs. 0xC0000095(INT_OVERFLOW) is now handled →PanicErrorString("integer overflow"), which yieldsruntime error: integer overflow(verified againsterrorString.Error()inz_error.go), matching gc'sruntimeErrorsemantics. Divide-by-zero → SIGFPE and access-violation/in-page → SIGSEGV are preserved.- The contradictory/stale comment block is replaced by a single accurate comment enumerating the three legitimate return paths (foreign thread, non-Go text, non-nil fault without
SetPanicOnFault).
62b7e632 runtime: harden Windows fault stack walking — resolves the 386 fpCallers and the fragile-3-frame-unwind findings:
fpCallersnow applies the alignment +memReadable(fp)+memReadable(fp+wordSize)guard at the top of every iteration (including the first), matching the fault-pathwindowsFPWalkFrompattern.- Both
recoverMarkfunctions (386 and SEH variants) are now//go:noinlinewith comments documenting that they must remain distinct frames — exactly the suggested hardening for the fixed 3-frame unwind assumption.
ad6ae693 build: remove unused ELF symbol quoting helper — resolves the dead-code finding: asmQuoteELFSymbol and its test are removed; grep confirms no remaining references.
Added test coverage (windowsruntime): checkIntegerOverflowFault asserts the recovered panic is runtime error: integer overflow, and checkForeignFaultOnNativeThread (new fault.c wrapper raising EXCEPTION_ACCESS_VIOLATION on a native CreateThread) locks in the Go-vs-foreign-fault invariant — the regression test I suggested.
I did not independently verify the CI/Codecov/benchmark status or the resolution of other reviewers' threads referenced in the description; this re-review covers the code delta.
1 participant
Refs #2325
Depends on #2404.
Summary
debug.SetPanicOnFaultbehavior while leaving foreign faults to the hostPlatform and size boundary
The fault handlers, unwinders, COFF carriers, wrapper markers, and pointer-store panic-site records are selected only for Windows. A Windows access violation exposes the faulting store PC, so that target needs a PC/line record to reproduce Go's recovered-panic source line without depending on PDB/debugger services.
Emitting one record for every potentially nil store on all native targets is a separate compatibility and representation question tracked in #2406. It is intentionally outside this Windows PR.
Using the same source and output paths for the #2404 parent and this head, all six macOS
cprintf,println, andfmtprintfnormal/full-LTO executable file sizes are byte-for-byte identical. Data and BSS are also identical;fmtprintftext is 4 B smaller in each mode. Linux is rechecked by the same-run PR benchmark job.Validation
go test ./ssa ./internal/build ./internal/build/funcinfo ./internal/debuginfo./clWindows metadata and store-site platform-selection testsgo test -vet=off ./test/go -run '^TestRuntimeStatementLineInfo$'windows_store_fault.go:167frame validationThe local host's full
./clLTO suite emits LLVM+zcm/+zczdiagnostics that also occur on the #2404 baseline; the affected focused tests and Windows native tests pass without those diagnostics.