fix: spill closure captures past the 16 inline slots instead of dropping them - #805
Merged
Conversation
…ing them a closure capturing 17 variables computed a wrong answer with no diagnostic anywhere: the runtime's environment is a fixed 16-slot array, pith_closure_set_env bounds-checked and silently RETURNED for slot 16+, and pith_closure_get_env answered 0. summing seventeen captured ints printed 136 — the sum of the first sixteen — and nothing at any layer said a capture had been discarded. the limitations doc even called the cap 'not a correctness issue'. slots past the inline block now spill to a heap extension allocated on the first spill store and grown to the highest slot written. the common closure pays nothing: the pointer stays null and every inline read is untouched. the release path, the cycle collector's child walk, and the collector's teardown all cover the spill region, so a heap capture past slot 16 is retained, traced, and released exactly like an inline one; the closure's Drop frees the storage itself at both Box::from_raw sites. the field is appended after cycle_flags under the same contract: nothing outside the runtime reads past env_tags, so the offsets the codegen could bake stay put. no emitter or checker change — the compiler never had a cap, only the runtime did — so the bootstrap seed is untouched. ## what was tested - new tests/cases/test_closure_many_captures.pith: 17 int captures sum to 153 (used to print 136), and an 18-string closure whose last two captures land in the spill region round-trips — output matches its golden and runs clean under valgrind (the spill retain/release balances) - runtime unit tests 195/195: the old test that pinned the silent-drop contract now pins the spill contract, and a new test covers spill round-trip, the zero gap between written spill slots, and reads past everything written - run-regressions-only 366/366 - negative slots still answer 0 and ignore stores, unchanged
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.
what this fixes
A closure capturing 17 variables computed a wrong answer with no diagnostic anywhere. The runtime's closure environment is a fixed 16-slot array:
pith_closure_set_envbounds-checked and silently returned for slot 16+, andpith_closure_get_envanswered 0. Summing seventeen captured ints printed 136 — the sum of the first sixteen. The limitations doc even called the cap "not a correctness issue".The drop was purely runtime-side — the emitter never had a cap and happily emitted stores for slot 16+ — which is why nothing at any layer complained.
the change
Slots past the inline block spill to a heap extension (
ClosureOverflow, Vec-backed), allocated on the first spill store and grown to the highest slot written. The common closure pays nothing: the pointer stays null and every inline read is untouched. The release path, the cycle collector's child walk, and the collector's teardown all cover the spill region, so a heap capture past slot 16 is retained, traced, and released exactly like an inline one; the closure'sDropfrees the storage itself at bothBox::from_rawsites.The field is appended after
cycle_flagsunder the same layout contract: nothing outside the runtime reads pastenv_tags, so any offsets the codegen could bake stay put. No emitter or checker change, so the bootstrap seed is untouched.The capture limit is simply gone — 17 or 40 captures compute the same answer as 3. docs/limitations.md rewritten accordingly.
what was tested
tests/cases/test_closure_many_captures.pith: 17 int captures sum to 153 (used to print 136), and an 18-string closure whose last two captures land in the spill region round-trips — output matches its golden and runs clean under valgrind (the spill retain/release balances)run-regressions-only366/366