fix: give a match on an rc-payload optional exact ownership - #809
Merged
Conversation
A match on an optional whose payload is rc-counted leaked the payload (and everything it held) once per match — the live residual behind task #214. Filed as "the return-widened box carries no destructor", but attaching one there would double-free: optional extraction transfers the shell's payload count out by convention, so shells on that path must stay destructor-less. The fix works within the name-keyed payload-release design instead, three coordinated pieces: - the optional-subject arm binding retains its own count (mirroring the enum payload binding dance: release the previous value on a loop rebind, retain the new one), and the prepass tracks the name so exit cleanup releases it or a return transfers it — under the same name-conflict poisoning as every other owned local. - a match on an optional local is now a whitelisted use in the payload-release cascade: the subject keeps the count its shell arrived with, so the cleanup path stays its last owner. a RESULT match still disqualifies (its binding path takes no count). - a tail match's expression arm returns through the full return contract (ir_emit_return_value, split out of ir_emit_return_stmt) instead of a bare `ret` — the bare ret skipped defers, the tracked local cleanup, the borrow retain, and the optional/result wrap, so every count an arm binding retained leaked per call (including the pre-existing enum-binding tail-return leak), and a borrowed return could hand the caller a count nobody owned. docs/ownership.md moves match onto the safe-use list with the why. ## what was tested - probes at 300k rounds, before -> after (peak rss): same-function match 34.8mb -> 2.1mb; match in a callee on the parameter 35.1mb -> 2.1mb; fresh optional matched from argument position 53.6mb -> 2.3mb; local bind + probes, unwrap_or through a call stay flat; every probe's stdout identical before and after - new tests/cases/test_optional_match_ownership.pith golden covers same-function match, callee-parameter match, a tail arm returning the binding's string field, and an arm returning the binding WHOLE (the transfer exclusion) - new tests/leaks/leak_optional_match_payload gate case: flat at 200k vs 800k rounds (2716 vs 2596 kb peak) - make bootstrap-verify: bootstrap ir fixed point verified (includes self regressions + std tests) - 8 consecutive make sitegen-check runs clean (the over-release gate) - make check-invalid-only: 51 passed, 0 failed; fmt + lint clean; bootstrap seed regenerated
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
matchon an optional whose payload is rc-counted (a struct here) leaked the payload once per match — ~110 B/round same-function, ~170 B/round when the fresh optional is matched from argument position. This is the live residual behind task #214.why not the filed fix
#214 was filed as "the return-widened box carries no payload destructor — attach one". That would double-free: optional extraction transfers the shell's payload count out by convention (
ir_tuple_call_arg_is_borrow's comment names it), so shells on that path must stay destructor-less. The fix works within the name-keyed payload-release design instead.what changed
ir_emit_match_bindings), mirroring the enum payload binding dance — release the previous value on a loop rebind, retain the new one — and the prepass tracks the name so exit cleanup releases it or a return transfers it, under the same name-conflict poisoning as every other owned local.ir_tuple_uses_all_safe): the subject keeps the count its shell arrived with, so the cleanup path stays its last owner. A RESULT match still disqualifies — its binding path takes no count.ir_emit_return_valueis split out ofir_emit_return_stmtand passed intoir_emit_match_returnas a callback (no import cycle). The bareretit used to emit skipped defers, the tracked-local cleanup, the borrow retain, and the optional/result wrap — so every count an arm binding retained leaked per call (including the pre-existing enum-binding tail-return leak), and a borrowed return could hand the caller a count nobody owned.docs/ownership.md moves match onto the safe-use list with the reasoning.
Still open on this branch's task list, deliberately separate PRs: the tuple-literal argument box (
f((a,b)), ~64 B/call) and the nestedf([[]])untagged inner list (~30 B/round) — different mechanisms, each measured live.what was tested
unwrap_orthrough a call stay flat; every probe's stdout identical before and aftertests/cases/test_optional_match_ownership.pithgolden: same-function match, callee-parameter match, a tail arm returning the binding's string field, and an arm returning the binding whole (the transfer exclusion)tests/leaks/leak_optional_match_payloadgate case, registered in tooling/leak_check.sh and measured with the gate's own sequence: flat at 200k vs 800k rounds (2716 vs 2596 kb peak)make bootstrap-verify: bootstrap ir fixed point verified (includes self regressions + std tests)make sitegen-checkruns clean — the golden-output gate that catches over-releases memcheck can'tmake check-invalid-only: 51 passed, 0 failed; fmt + lint clean; bootstrap seed regenerated