fix: free owners at the branch exit even when an arm returns (I-182) - #147
Conversation
|
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 (6)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ownership pass now anchors conditional last-use frees at branch exits without excluding branches that may return. New fixtures and integration, ASan, and Valgrind tests cover returning-arm and fallthrough-arm cases. ChangesOwnership leak fix
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The conditional ownership cleanup change has regression and leak-detection coverage, and no actionable current-head risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
The conditional-last-use re-anchor refused branches that may return, keeping the in-arm anchor so the return path stayed covered. But the in-arm anchor never fires on not-taken paths: an owner whose last read sits inside a returning arm leaked its buffer on every other path (steady 33 MB RSS growth over 500k calls, now flat), whether that path fell through to the function end or took a non-returning sibling arm. The refusal predates the owner return epilogue, which covers the return paths: its covered-check excludes ancestor branches, so a branch-exit anchor does not suppress the epilogue Free. Drop the refusal for both Inst and Param owners — the Free anchors at the branch exit on every fall-through path, and the epilogue fires on every return path. Regression coverage: two behavioral tests plus Valgrind/ASan fixtures for both arm shapes (last use in the returning arm; last use in a fall-through arm with a returning sibling).
ffae476 to
619b9f7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Fixes I-182: an owner whose last read sits inside a conditional arm leaked its heap buffer on every not-taken path whenever the branch (or a sibling arm) could
return. The conditional-last-use re-anchor refused may-return branches and kept the in-arm anchor — which never fires on paths that skip the arm:returns → the fall-through path reached the function end with the owner live (64 B/call; 33 MB peak RSS over 500k calls, now flat at 1 MB).returning sibling → the implicit-else path reached the merge with the owner live (same leak).Fix: drop the may-return refusal in the re-anchor (both
InstandParamowners), so the Free always anchors at the branch exit — the earliest point where the value is dead on all fall-through paths. The refusal predated the owner return epilogue, which owns the return paths: its covered-check excludes ancestor branches, so a branch-exit anchor never suppresses the epilogue Free, and the epilogue never double-fires on paths the exit anchor covers. No codegen changes.Test plan
cargo test --workspace— green (incl. 2 new behavioral tests: both arm shapes, taken + not-taken + sibling paths)./scripts/run_linux_tests.sh(Docker) — ASan 36/36, Valgrind 41/41, including 2 new fixtures (last_use_in_returning_arm_fallthrough,last_use_in_fallthrough_arm_sibling_returns) and the pre-existinglast_use_in_if_fallthrough/ conditional-move fixtures the issue called outRUSTFLAGS=-Dwarnings cargo clippy --workspace --all-targets,cargo fmt --check— cleanSummary by CodeRabbit
Bug Fixes
Tests