Skip to content

Fix safepoint stack slot reuse #13469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
angelnereira wants to merge 1 commit into from
+67 −8
Closed

Fix safepoint stack slot reuse #13469

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 67 additions & 8 deletions cranelift/frontend/src/frontend/safepoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,22 +759,25 @@ impl SafepointSpiller {

let mut option_inst = func.layout.last_inst(block);
while let Some(inst) = option_inst {
// If this instruction is a safepoint, then we must add stack
// map entries for the needs-stack-map values that are live
// across it.
if self.liveness.safepoints.contains_key(&inst) {
self.rewrite_safepoint(func, inst);
}

// If this instruction defines a needs-stack-map value that is
// live across a safepoint, then spill the value to its stack
// slot.
// slot. Do this after rewriting the safepoint itself so that
// the stack map reserves slots for values live across this
// instruction before any slots for instruction results are
// freed for reuse.
let mut pos = FuncCursor::new(func).after_inst(inst);
vals.extend_from_slice(pos.func.dfg.inst_results(inst));
for val in vals.drain(..) {
self.rewrite_def(&mut pos, val);
}

// If this instruction is a safepoint, then we must add stack
// map entries for the needs-stack-map values that are live
// across it.
if self.liveness.safepoints.contains_key(&inst) {
self.rewrite_safepoint(func, inst);
}

// Replace all uses of needs-stack-map values with loads from
// the value's associated stack slot.
let mut pos = FuncCursor::new(func).at_inst(inst);
Expand Down Expand Up @@ -3106,4 +3109,60 @@ block0:
"#
);
}

#[test]
fn safepoint_reserves_live_slots_before_freeing_result_slots() {
let mut sig = Signature::new(CallConv::SystemV);
sig.returns.push(AbiParam::new(ir::types::I32));

let mut fn_ctx = FunctionBuilderContext::new();
let mut func = Function::with_name_signature(ir::UserFuncName::testcase("sample"), sig);
let mut builder = FunctionBuilder::new(&mut func, &mut fn_ctx);

let name = builder
.func
.declare_imported_user_function(ir::UserExternalName {
namespace: 0,
index: 0,
});
let mut sig = Signature::new(CallConv::SystemV);
sig.returns.push(AbiParam::new(ir::types::I32));
let signature = builder.func.import_signature(sig);
let func_ref = builder.import_function(ir::ExtFuncData {
name: ir::ExternalName::user(name),
signature,
colocated: true,
patchable: false,
});

let block0 = builder.create_block();
builder.switch_to_block(block0);

let live = builder.ins().iconst(ir::types::I32, 1);
let call = builder.ins().call(func_ref, &[]);
let result = builder.func.dfg.first_result(call);
builder.ins().return_(&[result]);

builder.seal_all_blocks();
builder.finalize();

let mut spiller = SafepointSpiller::default();
spiller.liveness.post_order.push(block0);
spiller.liveness.live_across_any_safepoint.insert(live);
spiller
.liveness
.safepoints
.insert(call, [live].into_iter().collect());

let result_slot = spiller
.stack_slots
.get_or_create_stack_slot(&mut func, result);
spiller.rewrite(&mut func);

let live_slot = spiller.stack_slots.get(live).unwrap();
assert_ne!(
result_slot, live_slot,
"the safepoint result slot must not be reused for a value live across that same safepoint"
);
Comment on lines +3149 to +3166
Copy link
Copy Markdown
Member

@fitzgen fitzgen May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a little too low-level to really be very useful, because the bug is not in the low-level get-or-create-stack-slot APIs, it is in the order that those APIs are called when rewriting the whole function based on the liveness analysis.

It should be possible to make a test at the whole CLIF function level which asserts the expected output CLIF after running the safepoint spiller via assert_eq_output!(...), similar to e.g. the needs_stack_map_and_loop test in this test module, but which exercises this bug and checks for regressions. If I understand correctly, what is needed is something like this:

block0(v0: i64):
    v1 = call f(v0)
    ;; v1 needs inclusion in stack maps
    v2 = call f(v1)
    ;; v2 needs inclusion in stack maps
    v3 = call f(v2)
    return v3

That is, we have two values that need inclusion in stack maps, have the same type and non-overlapping live ranges and therefore could possibly reuse the same stack slot, and the live range for one ends at a safepoint.

This might not be the exact shape necessary to trigger the bug. It might require another call or that the values have longer live ranges across additional safepoints. I'm not exactly sure, but you should be able to come up with something based off this initial starting point. Basically just look at the low-level API call sequence you're currently making and craft a CLIF function that will trigger that same low-level API call sequence.

Please make sure that the invalid stack slot reuse is present in this test without the fix, and then that the invalid stack slot reuse goes away after the fix is reapplied.

}
}
Loading