cranelift: include every SSA value bound to a variable in stack maps#13449
Open
vouillon wants to merge 2 commits into
Open
cranelift: include every SSA value bound to a variable in stack maps#13449vouillon wants to merge 2 commits into
vouillon wants to merge 2 commits into
Conversation
Declares a variable that needs stack maps, defines it twice in the same block (`v1` then `v2`), reads both values, then calls a function as a safepoint before returning both. The expected CLIF spills both `v1` and `v2` to separate slots and lists both in the call's `stack_map=[…]`. Without the accompanying fix, `SSABuilder::variables` stores only the latest `Value` per `(Variable, Block)` pair, so the earlier definition is dropped from `values_for_var` and the safepoint pass omits it from the stack map. The test fails with only `v2` spilled and only one entry in `stack_map`.
`SSABuilder::variables` is a `SecondaryMap<Variable,
SecondaryMap<Block, PackedOption<Value>>>` that stores one `Value` per
`(Variable, Block)` pair, so each `def_var` overwrites whatever the
prior definition in that block was. `values_for_var` then iterates this
map and returns only the latest `Value` per block. That is the wrong
set for the safepoint pass: `FunctionBuilder::finalize` uses
`values_for_var` to propagate stack-map flags from a variable onto its
associated values, and any earlier SSA value bound to the variable in
the same block is silently dropped even if it is still live across a
later safepoint.
Concretely, wasm code that re-defines a stack-map variable while an
earlier SSA value bound to it is still live on the operand stack
across an intervening safepoint hits the pattern:
local.get N ;; pushes the SSA value of the first def
...
call $foo ;; safepoint
local.tee N ;; redefines the variable
...
array.new_fixed ;; another safepoint that may GC
The safepoint pass omits the first def's value from the stack map,
GC's trace doesn't mark it, sweep removes it from OASR, the slot is
freed, and the subsequent init-barrier in `array.new_fixed` inc-refs
a freed slot, corrupting the OASR list.
Add `all_var_values: SecondaryMap<Variable, EntitySet<Value>>` to
`SSABuilder`, and maintain the invariant that every `Value` written to
`variables[var][block]` is also in `all_var_values[var]`. The only
places that store a `Value` into `variables[var][block]` for the
first time are `def_var` and `find_var`'s block-param-creation tail,
so those are the two sites that need to insert. `use_var_nonlocal`'s
predecessor-copy loop only re-stores a `val` that `find_var` just
returned, so the value is already in `all_var_values`; a
`debug_assert!` documents and checks this. Finally switch
`values_for_var` to iterate the new set.
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.
SSABuilder::variablesis aSecondaryMap<Variable, SecondaryMap<Block, PackedOption<Value>>>that stores oneValueper(Variable, Block)pair, so eachdef_varoverwrites whatever the prior definition in that block was.values_for_varthen iterates this map and returns only the latestValueper block. That is the wrong set for the safepoint pass:FunctionBuilder::finalizeusesvalues_for_varto propagate stack-map flags from a variable onto its associated values, and any earlier SSA value bound to the variable in the same block is silently dropped even if it is still live across a later safepoint.Concretely, Wasm code that re-defines a stack-map variable while an earlier SSA value bound to it is still live on the operand stack across an intervening safepoint hits the pattern:
Without this fix the safepoint pass omits the first def's value from the stack map, GC's trace doesn't mark it, sweep removes it from OASR, the slot is freed, and the subsequent init-barrier in
array.new_fixedinc-refs a freed slot, corrupting the OASR list.Add
all_var_values: SecondaryMap<Variable, EntitySet<Value>>toSSABuilder, and maintain the invariant that everyValuewritten tovariables[var][block]is also inall_var_values[var]. The only places that store aValueintovariables[var][block]for the first time aredef_varandfind_var's block-param-creation tail, so those are the two sites that need to insert.use_var_nonlocal's predecessor-copy loop only re-stores avalthatfind_varjustreturned, so the value is already in
all_var_values; adebug_assert!documents and checks this. Finally switchvalues_for_varto iterate the new set.