docs(plan): retract the unsound typed-shape inlining remedy (#7578) - #7588
Conversation
#7581 recommended emitting the shape-install hit path inline at the new site, by analogy with #7566. #7586 tested both halves of that reasoning and both are wrong, so the plan was pointing the next reader at a use-after-free. 1. Outlining/inlining the frame is a REGRESSION, not a lever: push_cls 0.72 -> 0.75s, churn_alloc 0.72 -> 0.79s. The function is bound by instruction count, not frame size. 2. Having codegen OR GC_OBJ_TYPED_LAYOUT_INTACT into the inline new's header word breaks the unwritten 'intact => descriptor reachable' invariant. layout_set_typed_unknown, the only thing that clears the bit, is reachable only from the Some(verdict) arm (gc/layout.rs:782-788), so an intact-but-descriptor-less object never recovers -- SIDE_MASK to the collector, intact to the inline guard, raw double written over a pointer slot with no barrier. Records #7586's actual result (push_cls 1.091x at +0 bytes) and the real cause: ~30 of ~70 hit-path instructions re-derived compile-time class constants the FFI boundary had made opaque.
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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 |
1 participant
Docs only, but time-sensitive: the plan currently tells the next reader to build a use-after-free, and I put it there.
In #7581 I wrote that the typed-shape install's hit path should be emitted inline at the
newsite, reasoning that every argument but the object pointer is a compile-time constant and that this is exactly the shape #7566 won 1.81× on. #7586 tested both halves of that reasoning. Both are wrong.1. The frame is not the lever — inlining it is a regression
The prologue really does look like #7566's shape:
sub sp, sp, #0x150, sixstppairs — a 336-byte frame and twelve callee-saved spills per construction, sized by LLVM for a descriptor build that runs once per shape. Outlining it behind#[cold] #[inline(never)]cut the frame to 80 bytes and the spills to zero, and made it slower:push_clschurn_allocThose spills are cheap dual-issued stores off the critical path; keeping six arguments live to forward to the outlined call costs more in register moves than the prologue saves. The function is bound by instruction count, not frame size — which is why the shipped fix attacks instruction count instead and gets 1.091×.
2. ⛔ The codegen half is a use-after-free factory
This is the part worth reading, because it is genuinely free and therefore very tempting.
declare-path classes must have an empty pointer mask, so since #7566 the inlinenewalready writes itsGcHeaderas a single i64 constant. OR-ingGC_OBJ_TYPED_LAYOUT_INTACTinto it costs +0 instructions and +0 bytes.It breaks an unwritten invariant that
layout_note_slotdepends on: intact ⟹ a descriptor is reachable.I verified the mechanism directly. On a contradicting store to an object that is intact but descriptor-less, the probe resolves
None— andlayout_set_typed_unknown, the only thing that clears the intact bit, is reachable only from theSome(verdict)arm (gc/layout.rs:782–788). Control falls through to the pointer-mask path and the bit is never cleared. The object is thenSIDE_MASKto the collector and intact to the class-field inline guard, which consults no map by design. The raw-store fast path writes a double over a pointer slot with no barrier and no layout note, and the next collection walks it as a heap pointer.Worth flagging for whoever reads that code next: the comment at
layout.rs:742says aNoneverdict "can only cost an extra fall-through, never mis-track a slot". That is true only while the invariant holds — it is a consequence of the invariant, not an independent guarantee. It reads as reassurance precisely to the person about to break it.Both are now written into the plan as ⛔ entries with the measurements, so they are not rediscovered by someone re-deriving the same "obvious" idea.
check_file_size.shclean. No code touched.