From 25dd245c22ea4f0d757b819f7fa71dbc61c8ba1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Vouillon?= Date: Sat, 23 May 2026 19:09:22 +0200 Subject: [PATCH 1/3] Track stack-map variable bindings eagerly inside SSABuilder Move `stack_map_vars` and `stack_map_values` from `FunctionBuilderContext` into `SSABuilder` itself so that bindings can be recorded as they happen, in the two places SSA values get bound to a variable: * `SSABuilder::def_var`, when the user supplies a new definition; and * `SSABuilder::find_var`, when SSA construction synthesises a block parameter as the definition. --- cranelift/frontend/src/frontend.rs | 24 +++----------- cranelift/frontend/src/ssa.rs | 51 ++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 36 deletions(-) diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index ab85e91d2ac3..385dbe23f2d2 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -31,8 +31,6 @@ pub struct FunctionBuilderContext { ssa: SSABuilder, status: SecondaryMap, variables: PrimaryMap, - stack_map_vars: EntitySet, - stack_map_values: EntitySet, safepoints: safepoints::SafepointSpiller, } @@ -72,15 +70,11 @@ impl FunctionBuilderContext { ssa, status, variables, - stack_map_vars, - stack_map_values, safepoints, } = self; ssa.clear(); status.clear(); variables.clear(); - stack_map_values.clear(); - stack_map_vars.clear(); safepoints.clear(); } @@ -441,7 +435,7 @@ impl<'a> FunctionBuilder<'a> { let ty = self.func_ctx.variables[var]; assert!(ty != types::INVALID); assert!(ty.bytes() <= 16); - self.func_ctx.stack_map_vars.insert(var); + self.func_ctx.ssa.stack_map_vars_mut().insert(var); } /// Returns the Cranelift IR necessary to use a previously defined user @@ -559,7 +553,7 @@ impl<'a> FunctionBuilder<'a> { assert!(size <= 16); assert!(size.is_power_of_two()); - self.func_ctx.stack_map_values.insert(val); + self.func_ctx.ssa.stack_map_values_mut().insert(val); } /// Creates a jump table in the function, to be used by [`br_table`](InstBuilder::br_table) instructions. @@ -709,23 +703,13 @@ impl<'a> FunctionBuilder<'a> { } } - // Propagate the needs-stack-map bit from variables to each of their - // associated values. - for var in self.func_ctx.stack_map_vars.iter() { - for val in self.func_ctx.ssa.values_for_var(var) { - log::trace!("propagating needs-stack-map from {var:?} to {val:?}"); - debug_assert_eq!(self.func.dfg.value_type(val), self.func_ctx.variables[var]); - self.func_ctx.stack_map_values.insert(val); - } - } - // If we have any values that need inclusion in stack maps, then we need // to run our pass to spill those values to the stack at safepoints and // generate stack maps. - if !self.func_ctx.stack_map_values.is_empty() { + if !self.func_ctx.ssa.stack_map_values().is_empty() { self.func_ctx .safepoints - .run(&mut self.func, &self.func_ctx.stack_map_values); + .run(&mut self.func, self.func_ctx.ssa.stack_map_values()); } // Clear the state (but preserve the allocated buffers) in preparation diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index 1381c05421dd..3d1bfcf5f8bf 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -37,11 +37,11 @@ pub struct SSABuilder { /// the variable in the block. variables: SecondaryMap>>, - /// Records every SSA `Value` ever associated with a variable, including - /// values that `variables` has since overwritten within the same block. - /// Used by `values_for_var` to feed `FunctionBuilder::finalize`'s - /// stack-map propagation. - all_var_values: SecondaryMap>, + /// Variables whose bindings must be tracked for stack maps. + stack_map_vars: EntitySet, + + /// SSA values that must be included in stack maps. + stack_map_values: EntitySet, /// Records the position of the basic blocks and the list of values used but not defined in the /// block. @@ -116,7 +116,8 @@ impl SSABuilder { /// deallocating memory. pub fn clear(&mut self) { self.variables.clear(); - self.all_var_values.clear(); + self.stack_map_vars.clear(); + self.stack_map_values.clear(); self.ssa_blocks.clear(); self.variable_pool.clear(); self.inst_pool.clear(); @@ -128,7 +129,8 @@ impl SSABuilder { /// Tests whether an `SSABuilder` is in a cleared state. pub fn is_empty(&self) -> bool { self.variables.is_empty() - && self.all_var_values.is_empty() + && self.stack_map_vars.is_empty() + && self.stack_map_values.is_empty() && self.ssa_blocks.is_empty() && self.calls.is_empty() && self.results.is_empty() @@ -209,18 +211,35 @@ fn emit_zero(ty: Type, mut cur: FuncCursor) -> Value { /// Phi functions. /// impl SSABuilder { - /// Get all of the values associated with the given variable that we have - /// inserted in the function thus far. - pub fn values_for_var(&self, var: Variable) -> impl Iterator + '_ { - self.all_var_values[var].iter() - } - /// Declares a new definition of a variable in a given basic block. /// The SSA value is passed as an argument because it should be created with /// `ir::DataFlowGraph::append_result`. pub fn def_var(&mut self, var: Variable, val: Value, block: Block) { self.variables[var][block] = PackedOption::from(val); - self.all_var_values[var].insert(val); + self.record_stack_map_binding(var, val); + } + + /// Mutable view of the variables whose bindings must be tracked for stack maps. + pub fn stack_map_vars_mut(&mut self) -> &mut EntitySet { + &mut self.stack_map_vars + } + + /// SSA values that must be included in stack maps. + pub fn stack_map_values(&self) -> &EntitySet { + &self.stack_map_values + } + + /// Mutable view of [`Self::stack_map_values`]. + pub fn stack_map_values_mut(&mut self) -> &mut EntitySet { + &mut self.stack_map_values + } + + /// Propagate the needs-stack-map bit from `var` to `val`. + fn record_stack_map_binding(&mut self, var: Variable, val: Value) { + if self.stack_map_vars.contains(var) { + log::trace!("recording stack-map binding {var:?} -> {val:?}"); + self.stack_map_values.insert(val); + } } /// Declares a use of a variable in a given basic block. Returns the SSA value corresponding @@ -283,7 +302,7 @@ impl SSABuilder { // any of the blocks before `from`. // // So in either case there is no definition in these blocks yet and we can blindly set one. - debug_assert!(self.all_var_values[var].contains(val)); + debug_assert!(!self.stack_map_vars.contains(var) || self.stack_map_values.contains(val)); let var_defs = &mut self.variables[var]; while block != from { debug_assert!(var_defs[block].is_none()); @@ -340,7 +359,7 @@ impl SSABuilder { // find a usable definition. So create one. let val = func.dfg.append_block_param(block, ty); var_defs[block] = PackedOption::from(val); - self.all_var_values[var].insert(val); + self.record_stack_map_binding(var, val); // Now every predecessor needs to pass its definition of this variable to the newly added // block parameter. To do that we have to "recursively" call `use_var`, but there are two From 87f6f1f226ae489dff6e86c2e1c9e36663e6e75d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Vouillon?= Date: Wed, 27 May 2026 10:38:31 +0200 Subject: [PATCH 2/3] Document and enforce stack-map declaration ordering Marking a variable as needing a stack map now tracks bindings eagerly, so `declare_var_needs_stack_map` must be called before any `def_var` of the variable; earlier definitions would be silently omitted from stack maps. * Document the ordering constraint on `declare_var_needs_stack_map`. * Assert in `SSABuilder::mark_var_needs_stack_map` that the variable has no recorded definitions yet, so misuse panics rather than silently dropping a GC root from stack maps. * Fix the fuzzer's variable pool, which declared the stack-map need after the initial `def_var`, dropping that definition from stack maps. --- cranelift/frontend/src/frontend.rs | 8 ++++---- cranelift/frontend/src/ssa.rs | 12 +++++++++--- cranelift/fuzzgen/src/function_generator.rs | 3 ++- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index 385dbe23f2d2..83dbd19b3fe6 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -424,18 +424,18 @@ impl<'a> FunctionBuilder<'a> { /// the stack and it being reloading again, the stack can be updated to /// facilitate moving GCs. /// - /// This does not affect any pre-existing uses of the variable. + /// This must be called before any definition of the variable. /// /// # Panics /// - /// Panics if the variable's type is larger than 16 bytes or if this - /// variable has not been declared yet. + /// Panics if the variable's type is larger than 16 bytes, if this + /// variable has not been declared yet, or if it has already been defined. pub fn declare_var_needs_stack_map(&mut self, var: Variable) { log::trace!("declare_var_needs_stack_map({var:?})"); let ty = self.func_ctx.variables[var]; assert!(ty != types::INVALID); assert!(ty.bytes() <= 16); - self.func_ctx.ssa.stack_map_vars_mut().insert(var); + self.func_ctx.ssa.mark_var_needs_stack_map(var); } /// Returns the Cranelift IR necessary to use a previously defined user diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index 3d1bfcf5f8bf..8ea3bbc05726 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -219,9 +219,15 @@ impl SSABuilder { self.record_stack_map_binding(var, val); } - /// Mutable view of the variables whose bindings must be tracked for stack maps. - pub fn stack_map_vars_mut(&mut self) -> &mut EntitySet { - &mut self.stack_map_vars + /// Mark `var` as needing its bindings tracked for stack maps. + pub fn mark_var_needs_stack_map(&mut self, var: Variable) { + assert!( + self.variables[var].values().all(|v| v.is_none()), + "declare_var_needs_stack_map({var:?}) must be called before any \ + definition of the variable; otherwise earlier definitions are \ + silently omitted from stack maps" + ); + self.stack_map_vars.insert(var); } /// SSA values that must be included in stack maps. diff --git a/cranelift/fuzzgen/src/function_generator.rs b/cranelift/fuzzgen/src/function_generator.rs index 312a898e25c5..0f965fa03cb8 100644 --- a/cranelift/fuzzgen/src/function_generator.rs +++ b/cranelift/fuzzgen/src/function_generator.rs @@ -1920,7 +1920,6 @@ where for (ty, value) in vars.into_iter() { let var = builder.declare_var(ty); - builder.def_var(var, value); // Randomly declare variables as needing a stack map. // We limit these to only types that have fewer than 16 bytes @@ -1929,6 +1928,8 @@ where builder.declare_var_needs_stack_map(var); } + builder.def_var(var, value); + self.resources .vars .entry(ty) From c8e8af0916bb7d7a83b913d7ba1d767bd8f4757e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Vouillon?= Date: Wed, 27 May 2026 20:53:45 +0200 Subject: [PATCH 3/3] Use a debug assertion for stack-map declaration ordering The check that `mark_var_needs_stack_map` runs before any definition of the variable scans the variable's per-block definitions. Demote it from `assert!` to `debug_assert!` so it does not run in release builds. --- cranelift/frontend/src/frontend.rs | 5 +++-- cranelift/frontend/src/ssa.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index 83dbd19b3fe6..ecf354623f07 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -428,8 +428,9 @@ impl<'a> FunctionBuilder<'a> { /// /// # Panics /// - /// Panics if the variable's type is larger than 16 bytes, if this - /// variable has not been declared yet, or if it has already been defined. + /// Panics if the variable's type is larger than 16 bytes or if this + /// variable has not been declared yet. In debug builds, also panics if + /// the variable has already been defined. pub fn declare_var_needs_stack_map(&mut self, var: Variable) { log::trace!("declare_var_needs_stack_map({var:?})"); let ty = self.func_ctx.variables[var]; diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs index 8ea3bbc05726..42bd4c6d0957 100644 --- a/cranelift/frontend/src/ssa.rs +++ b/cranelift/frontend/src/ssa.rs @@ -221,7 +221,7 @@ impl SSABuilder { /// Mark `var` as needing its bindings tracked for stack maps. pub fn mark_var_needs_stack_map(&mut self, var: Variable) { - assert!( + debug_assert!( self.variables[var].values().all(|v| v.is_none()), "declare_var_needs_stack_map({var:?}) must be called before any \ definition of the variable; otherwise earlier definitions are \