s2: Fix amd64 stack frame corruption#1145
Conversation
Fixes crash when pre-empted. Fixes #1097 - see for analysis.
📝 WalkthroughWalkthroughAdjusted the frame size in the AMD64 assembly code for the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
s2/decode_amd64.s (1)
54-569: 🏗️ Heavy liftAdd a regression/stress test to prevent this asm+async-preemption crash from resurfacing.
Because the reported failure is specifically an async preemption SIGSEGV during
s2.Decodeon amd64, it would be good to add a targeted stress test that:
- runs many concurrent decodes,
- encourages async preemption (e.g., frequent scheduling/yield points and long-running loops),
- and repeats enough times to catch rare stack-corruption failures.
Even a best-effort test (may be flaky unless tuned) would materially reduce risk for future asm changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@s2/decode_amd64.s` around lines 54 - 569, Add a targeted stress test that repeatedly runs concurrent s2.Decode calls to reproduce async-preemption stack corruption; create a test (e.g., TestDecodeAsyncPreemptStress) that spawns many goroutines (N=hundreds), sets runtime.GOMAXPROCS>1, and in each goroutine loops many times (thousands) calling s2.Decode on specially crafted inputs that exercise long-running asm paths (long literal and copy sequences), interleaving runtime.Gosched() or time.Sleep(0) to encourage async preemption and verifying every Decode returns the expected result (no panic and correct error code); run the test with short timeouts and allow tuning of goroutine/iteration counts so CI can run a smaller default but maintain a heavy local/regression mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@s2/decode_amd64.s`:
- Around line 54-569: Add a targeted stress test that repeatedly runs concurrent
s2.Decode calls to reproduce async-preemption stack corruption; create a test
(e.g., TestDecodeAsyncPreemptStress) that spawns many goroutines (N=hundreds),
sets runtime.GOMAXPROCS>1, and in each goroutine loops many times (thousands)
calling s2.Decode on specially crafted inputs that exercise long-running asm
paths (long literal and copy sequences), interleaving runtime.Gosched() or
time.Sleep(0) to encourage async preemption and verifying every Decode returns
the expected result (no panic and correct error code); run the test with short
timeouts and allow tuning of goroutine/iteration counts so CI can run a smaller
default but maintain a heavy local/regression mode.
Fixes crash when pre-empted.
Fixes #1097 - see for analysis.
Summary by CodeRabbit