Fix callbacks that cause goroutine stack to grow#59
Conversation
CallNFloat built the syscall argument and return block on the goroutine stack and handed its address to runtime_cgocall. syscallN runs on g0 and writes the C return values back into that block after the call returns. If that call runs a callback that re-enters Go and grows the goroutine stack, the stack moves (copystack) and syscallN's write lands at the stale, pre-move address, so the first such call silently loses its return value. Take the block from a sync.Pool, so it lives on the heap and never sits on a moving stack, and pin it with runtime.Pinner across the cgocall. Pooling keeps the move-safe path allocation-free in steady state; the pin keeps it correct should the heap ever become movable. Add TestCallbackGrowStack, which drives a callback through CallFunction on a fresh small-stack goroutine and forces a large copystack from inside the callback, reproducing the lost-return-value bug deterministically instead of relying on process-global warmup state. Also note runtime.Pinner in CallFunctionContext's safety documentation.
Add return_struct_24 (a 24-byte struct) and TestStructReturn24B, which exercise the sret ABI path the existing struct-return tests (all <= 16 bytes) never reach: the caller provides the rvalue buffer and the callee writes the struct straight into it through the hidden return pointer (RDI on amd64, X8 on arm64). Simplify the sret setup on both architectures to point that hidden return pointer directly at rvalue. CallFunction already documents that the return buffer must be valid and large enough for the result, so there is no need to allocate a scratch buffer or special-case a nil rvalue.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kolkov
left a comment
kolkov
left a comment
There was a problem hiding this comment.
Thanks for this — you found a real bug that we missed. Our v0.5.4 //go:noescape optimization was wrong: when a callback triggers copystack, syscallN on g0 writes return values to the old (freed) goroutine stack address. The runtime comment at cgocall.go:331 says it plainly: "When we call into Go, the stack is free to be moved."
We went through the runtime source in detail — exitsyscall() at line 338 of cgocallbackg clears both syscallsp and throwsplit, which removes the two guards that copystack checks (stack.go:900, stack.go:1030). After that, any stack growth in the callback moves the goroutine stack and the old pages go back to stackcache — not unmapped, just recycled. So it's silent corruption, not a segfault. Nasty.
The test is solid. growStack(512) with 4KB frames on a fresh goroutine makes it deterministic.
One concern with the current PR though: runtime.Pinner adds measurable overhead. Looking at the CI benchmarks:
main (v0.5.5): BenchmarkGoffiIntArgs 112.3 ns/op
PR #59: BenchmarkGoffiIntArgs 188.3 ns/op (+68%)
The Pinner path goes through span.speciallock (pinner.go:168) on every call — that's a real mutex acquisition, not just an atomic. We measured ~60 ns per Pin+Unpin cycle sequentially.
purego uses sync.Pool without Pinner (func.go:25, go_runtime.go:13 — no //go:noescape either) and has shipped that way for years. Go's GC is explicitly "non-generational and non-compacting" (runtime/mgc.go:9). If a moving GC ever ships, adding Pinner is a one-line change — but right now it costs us 68% regression for a hypothetical future scenario.
Would you be open to dropping the Pinner and going with Pool-only, matching purego's pattern? That should bring the regression down to roughly +12% instead of +68%. We can add a comment noting where to add Pinner if moving GC ever lands.
The sret cleanup and TestStructReturn24B look good independently — happy to take those as-is.
It is possible to be be made rudely aware if goffi is ever used with a newer version of Go that does happen to introduce a moving GC. https://github.com/go4org/unsafe-assume-no-moving-gc/blob/main/check.go (Of course should such a check ever be added to goffi it would be outside the scope of this PR.) |
|
Thanks for the pointer! We looked into it — interesting approach, but probably not for us. It's a runtime panic in |
Pool alone guarantees heap residence — sufficient for Go's non-compacting GC (confirmed through Go 1.27 including Green Tea GC). Pinner added ~60ns overhead per call due to span.speciallock mutex (measured: +68% regression). cgocheck does not apply to goffi (uses //go:linkname, not cmd/cgo). Also removes //go:noescape from hot-path runtime_cgocall declarations since args is now heap-allocated via Pool. If Go ever ships a compacting GC, add runtime.Pinner back (4 lines).
|
I pushed a commit dropping the Pinner while keeping the Pool fix intact. The reasoning: The Pool is the actual bug fix — it moves The Pinner was adding ~60 ns per call through Go's GC is non-compacting through 1.27 (Green Tea changed scanning locality, not object placement). If a compacting GC ever ships, re-adding Pinner is 4 lines — the comment in the code points to exactly where. I also removed The Let me know if you have concerns with this direction. |
- CHANGELOG: v0.5.6 entry with bug description and credit to @tie - README: remove incorrect '0 allocs (Unix)' claim, document Pool - ARCHITECTURE: replace noescape description with Pool explanation - PERFORMANCE: update allocation description
goffi v0.5.6 fixes callback stack-move corruption: callbacks that grow the goroutine stack could corrupt callbackArgs pointers because Go's moving GC relocates stacks. Fix uses sync.Pool + runtime.Pinner. PR go-webgpu/goffi#59, contributor: @tie.
3 participants
This change fixes callbacks that cause goroutine stacks to grow by using
callbackArgsfrom async.Pool,and ensures that the we do not assume non-moving GC on that path using
runtime.Pinner.See
TestCallbackGrowStackfor reproducer.Also adds a minor
srettest/cleanup.