test(gc): a zero-slot test fixture has room for the named-store floor (fixes #10941) - #10949
proggeramlug wants to merge 1 commit into
Conversation
…fixes #10941) `alloc_{nursery,old}_test_object(0)` allocated exactly an `ObjectHeader` and left the receiver unstamped, on the reasoning recorded above it that "a zero-slot fixture needs no descriptor at all - the derived bound is 0 either way". A named-property write does not respect that bound. The inline/overflow boundary is `max(object_live_slot_count(obj), INLINE_SLOT_FLOOR)` and the floor is 2, so the first two keys written to a zero-slot fixture store into inline slots 0 and 1 of an object that has none. Those two words are the next cell. Every caller before PR #10938 only ever set a `[[Prototype]]` on one, so nothing had written a named property and the hazard was invisible; it presents as a wrong read now and a SIGSEGV somewhere unrelated later. Both fixtures now allocate `max(field_count, INLINE_SLOT_FLOOR)` slots while PUBLISHING the bound as `field_count`, so the collector still traces exactly `field_count` slots and the descriptor-count accounting the original comment protects is unchanged. Witness: `gc::tests::zero_slot_fixture` asserts the ALLOCATION, for both the nursery and the old-generation fixture, and reddens by name when the change is reverted. An end-to-end pin - six named writes, read back - was written and deliberately dropped: without the fix it does not fail, it dumps core, which under `--test-threads=1` takes the other ~4,200 results with it. That is recorded in the module doc. Suite: 4218 passed / 0 failed / 6 ignored, `--test-threads=1`, both arms.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe GC test allocators now reserve two physical inline slots for zero-slot objects. New nursery and old-generation tests verify the required payload size while preserving the published zero-slot bound. ChangesZero-slot GC fixture safety
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This test-only change reserves inline storage for zero-slot GC fixtures while preserving their published bounds; no merge-blocking production impact is evident. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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. Comment |
|
Landed on main in merge train 255 (#10950, v0.5.1636), main The train carried this PR at head |
2 participants
Fixes #10941.
The bug
alloc_{nursery,old}_test_object(0)(gc/tests/support.rs) allocated exactly anObjectHeaderand left the receiver unstamped, on the reasoning recorded in the comment above it:A named-property write does not respect that bound. The inline/overflow boundary is
max(object_live_slot_count(obj), INLINE_SLOT_FLOOR)and the floor is 2, so the first two keys written to a zero-slot fixture store into inline slots 0 and 1 of an object that has none. Those two words are the next cell.Every caller before PR #10938 only ever set a
[[Prototype]]on one, so nothing had written a named property and the hazard was invisible. It presents as a wrong read now and a SIGSEGV somewhere unrelated later.The change
Option 1 from the issue. Both fixtures allocate
max(field_count, INLINE_SLOT_FLOOR)slots while publishing the bound asfield_count, so the collector still traces exactlyfield_countslots and the descriptor-count accounting the original comment protects is unchanged. The zero-fill covers the allocation rather than the published bound.Witness, and it can fail
gc::tests::zero_slot_fixtureasserts the allocation — for the nursery fixture and its old-generation twin — and reverting the change reddens both by name:An end-to-end pin — six named writes to a zero-slot fixture, read back — was written and then deliberately dropped, and the module doc says so: without the fix it does not fail, it dumps core, which under
--test-threads=1takes the other ~4,200 results in the process with it. The structural assertions redden with a message and for the same reason.Suite
cargo test --release -p perry-runtime --lib -- --test-threads=1, both arms:4218 passed / 0 failed / 6 ignored.
Base:
0fa391529(train 253, v0.5.1633). Independent of #10948 (#10939) — either can land first.Summary by CodeRabbit
Bug Fixes
Tests