fix: stop batch commands from dangling on return - #134
Merged
Merged
Conversation
`Cmd.batch` holds a slice that the runtime reads after `update` has
returned. `&.{ ... }` only lands in static memory when every element is
comptime-known; one runtime value and the array is a stack temporary
that is gone by the time the runtime walks it. Reading it back gives an
invalid union tag and aborts.
`examples/hello_world.zig` did exactly this: the 'c' key path built a
batch containing `layout.size_cells` and friends. It now keeps the two
commands in a field on the model, which outlives the call.
Adds `Cmd.batchAlloc`/`Cmd.sequenceAlloc`, which copy into an allocator
-- the frame allocator being the right one, since commands are processed
during the tick that produced them. The lifetime rule is documented on
the union fields and in the reference.
Found while writing the model test harness: a batch built the obvious
way came back as the wrong command entirely.
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.
Found while writing the model test harness (#133): a batch built the obvious way came back as a completely different command.
The bug
Cmd.batchholds a[]const Cmd(Msg)that the runtime reads afterupdatehas returned. A&.{ ... }literal only lands in static memory when every element is comptime-known. One runtime value in there and the array becomes a stack temporary, gone by the time the runtime walks it.Minimal reproduction:
There is no diagnostic — the slice just points at reused stack memory. Depending on what lands there, a batch silently executes the wrong commands or aborts on an invalid union tag.
examples/hello_world.zighit this exactly: the'c'key path built a batch containinglayout.size_cells,layout.rowandlayout.col, all runtime values.The fix
The example now keeps its two commands in a field on the model, which outlives the call:
For cases where model-owned storage does not fit,
Cmd.batchAlloc/Cmd.sequenceAlloccopy into an allocator:The frame allocator is the right one: commands are processed during the tick that produced them, and it is reset on the next.
The lifetime rule is now documented on the
batchandsequenceunion fields and in the reference, since the type signature cannot express it.Tests
tests/command_tests.zigbuilds each shape inside a function, returns it, deliberately overwrites 8 KB of stack, and then reads the batch back. ThebatchAlloc,sequenceAlloc, model-owned and all-comptime forms survive; the raw runtime literal is what does not, and is the case being steered away from.zig build testandzig buildclean on 0.16.0.