Skip to content

Commit 937c795

Browse files
committed
ZJIT: For JIT-to-JIT send, avoid loading uninitialized local through EP
JIT-to-JIT sends don't blit locals to nil in the callee's EP memory region because HIR is aware of this initial state and memory ops are only done when necessary. Previously, we read from this initialized memory by emitting `GetLocal` in e.g. BBs that are immediate successor to an entrypoint. The entry points sets up the frame state properly and we also reload locals if necessary after an operation that potentially makes the environment escape. So, listen to the frame state when it's supposed to be up-to-date (`!local_inval`).
1 parent ba47c2f commit 937c795

3 files changed

Lines changed: 66 additions & 10 deletions

File tree

zjit/src/hir.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5122,7 +5122,12 @@ pub fn iseq_to_hir(iseq: *const rb_iseq_t) -> Result<Function, ParseError> {
51225122
}
51235123
YARVINSN_getlocal_WC_0 => {
51245124
let ep_offset = get_arg(pc, 0).as_u32();
5125-
if ep_escaped || has_blockiseq { // TODO: figure out how to drop has_blockiseq here
5125+
if !local_inval {
5126+
// The FrameState is the source of truth for locals until invalidated.
5127+
// In case of JIT-to-JIT send locals might never end up in EP memory.
5128+
let val = state.getlocal(ep_offset);
5129+
state.stack_push(val);
5130+
} else if ep_escaped || has_blockiseq { // TODO: figure out how to drop has_blockiseq here
51265131
// Read the local using EP
51275132
let val = fun.push_insn(block, Insn::GetLocal { ep_offset, level: 0, use_sp: false, rest_param: false });
51285133
state.setlocal(ep_offset, val); // remember the result to spill on side-exits

zjit/src/hir/opt_tests.rs

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -780,14 +780,13 @@ mod hir_opt_tests {
780780
EntryPoint JIT(0)
781781
Jump bb2(v5, v6)
782782
bb2(v8:BasicObject, v9:BasicObject):
783-
v13:BasicObject = GetLocal l0, EP@3
784783
PatchPoint MethodRedefined(C@0x1000, fun_new_map@0x1008, cme:0x1010)
785784
PatchPoint NoSingletonClass(C@0x1000)
786-
v24:ArraySubclass[class_exact:C] = GuardType v13, ArraySubclass[class_exact:C]
787-
v25:BasicObject = CCallWithFrame C#fun_new_map@0x1038, v24, block=0x1040
788-
v16:BasicObject = GetLocal l0, EP@3
785+
v23:ArraySubclass[class_exact:C] = GuardType v9, ArraySubclass[class_exact:C]
786+
v24:BasicObject = CCallWithFrame C#fun_new_map@0x1038, v23, block=0x1040
787+
v15:BasicObject = GetLocal l0, EP@3
789788
CheckInterrupts
790-
Return v25
789+
Return v24
791790
");
792791
}
793792

@@ -7927,4 +7926,57 @@ mod hir_opt_tests {
79277926
Return v32
79287927
");
79297928
}
7929+
7930+
#[test]
7931+
fn no_load_from_ep_right_after_entrypoint() {
7932+
let formatted = eval("
7933+
def read_nil_local(a, _b, _c)
7934+
formatted ||= a
7935+
@formatted = formatted
7936+
-> { formatted } # the environment escapes
7937+
end
7938+
7939+
def call
7940+
puts [], [], [], [] # fill VM stack with junk
7941+
read_nil_local(true, 1, 1) # expected direct send
7942+
end
7943+
7944+
call # profile
7945+
call # compile
7946+
@formatted
7947+
");
7948+
assert_eq!(Qtrue, formatted, "{}", formatted.obj_info());
7949+
assert_snapshot!(hir_string("read_nil_local"), @r"
7950+
fn read_nil_local@<compiled>:3:
7951+
bb0():
7952+
EntryPoint interpreter
7953+
v1:BasicObject = LoadSelf
7954+
v2:BasicObject = GetLocal l0, SP@7
7955+
v3:BasicObject = GetLocal l0, SP@6
7956+
v4:BasicObject = GetLocal l0, SP@5
7957+
v5:NilClass = Const Value(nil)
7958+
Jump bb2(v1, v2, v3, v4, v5)
7959+
bb1(v8:BasicObject, v9:BasicObject, v10:BasicObject, v11:BasicObject):
7960+
EntryPoint JIT(0)
7961+
v12:NilClass = Const Value(nil)
7962+
Jump bb2(v8, v9, v10, v11, v12)
7963+
bb2(v14:BasicObject, v15:BasicObject, v16:BasicObject, v17:BasicObject, v18:NilClass):
7964+
CheckInterrupts
7965+
v27:BasicObject = GetLocal l0, EP@6
7966+
SetLocal l0, EP@3, v27
7967+
v39:BasicObject = GetLocal l0, EP@3
7968+
PatchPoint SingleRactorMode
7969+
SetIvar v14, :@formatted, v39
7970+
v45:Class[VMFrozenCore] = Const Value(VALUE(0x1000))
7971+
PatchPoint MethodRedefined(Class@0x1008, lambda@0x1010, cme:0x1018)
7972+
PatchPoint NoSingletonClass(Class@0x1008)
7973+
v59:BasicObject = CCallWithFrame RubyVM::FrozenCore.lambda@0x1040, v45, block=0x1048
7974+
v48:BasicObject = GetLocal l0, EP@6
7975+
v49:BasicObject = GetLocal l0, EP@5
7976+
v50:BasicObject = GetLocal l0, EP@4
7977+
v51:BasicObject = GetLocal l0, EP@3
7978+
CheckInterrupts
7979+
Return v59
7980+
");
7981+
}
79307982
}

zjit/src/hir/tests.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1500,11 +1500,10 @@ pub mod hir_build_tests {
15001500
EntryPoint JIT(0)
15011501
Jump bb2(v5, v6)
15021502
bb2(v8:BasicObject, v9:BasicObject):
1503-
v13:BasicObject = GetLocal l0, EP@3
1504-
v15:BasicObject = Send v13, 0x1000, :each
1505-
v16:BasicObject = GetLocal l0, EP@3
1503+
v14:BasicObject = Send v9, 0x1000, :each
1504+
v15:BasicObject = GetLocal l0, EP@3
15061505
CheckInterrupts
1507-
Return v15
1506+
Return v14
15081507
");
15091508
}
15101509

0 commit comments

Comments
 (0)