Fix GH-21267: JIT infinite loop on FETCH_OBJ_R with IS_UNDEF property#21368
Conversation
|
I see the problem, but the fix looks too pessimistic. It adds a guard check that is going to be almost always useless. For now I simplified the test case . The first one fails on PHP-8.4 and above the second also fails on PHP-8.3 --TEST--
GH-21267 (JIT infinite loop on FETCH_OBJ_R with IS_UNDEF property in polymorphic context)
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=tracing
opcache.jit_buffer_size=64M
opcache.jit_hot_loop=0
opcache.jit_hot_func=2
opcache.jit_hot_return=0
opcache.jit_hot_side_exit=1
--FILE--
class C {
public $x = true;
public function __get($name) { return null; }
public function getX() { return $this->x; }
}
$o1 = new C;
$o2 = new C;
$o2->x = false;
$o3 = new C;
unset($o3->x);
$a = [$o1, $o2, $o3];
for ($i = 0; $i < 8; $i++) {
$m = $a[$i % 3];
$m->getX();
$m->getX();
}
?>
OK
--EXPECT--
OK--TEST--
GH-21267 (JIT infinite loop on FETCH_OBJ_R with IS_UNDEF property in polymorphic context)
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit=tracing
opcache.jit_buffer_size=64M
opcache.jit_hot_loop=0
opcache.jit_hot_func=2
opcache.jit_hot_return=0
opcache.jit_hot_side_exit=1
--FILE--
class C {
public $x = true;
public function __get($name) { return null; }
public function getX() { return $this->x; }
}
$o1 = new C;
unset($o1->x);
$o2 = new C;
$a = [$o1, $o2, $o2];
for ($i = 0; $i < 8; $i++) {
$m = $a[$i % 3];
$m->getX();
$m->getX();
}
?>
OK
--EXPECT--
OK |
|
Following is a better fix: diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 19b3ce12573..899188d5423 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -7972,7 +7972,19 @@ static int zend_jit_escape_if_undef(zend_jit_ctx *jit, int var, uint32_t flags,
}
jit_LOAD_IP_ADDR(jit, opline - 1);
- ir_IJMP(jit_STUB_ADDR(jit, jit_stub_trace_escape));
+
+ /* We can't use trace_escape() because opcode handler may be overriden by JIT */
+ zend_jit_op_array_trace_extension *jit_extension =
+ (zend_jit_op_array_trace_extension*)ZEND_FUNC_INFO(jit->current_op_array);
+ size_t offset = jit_extension->offset;
+ zend_vm_opcode_handler_func_t orig_handler =
+ ZEND_OP_TRACE_INFO((opline - 1), offset)->orig_handler;
+ ir_ref ref = ir_CONST_ADDR(orig_handler);
+ if (GCC_GLOBAL_REGS || ZEND_VM_KIND == ZEND_VM_KIND_TAILCALL) {
+ zend_jit_tailcall_handler(jit, ref);
+ } else {
+ zend_jit_vm_enter(jit, ref);
+ }
ir_IF_TRUE(if_def);
This should be applied to PHP-8.4 and above. |
When the JIT defers the IS_UNDEF check for FETCH_OBJ_R to the result type guard, the deoptimization escape path dispatches to opline->handler via the trace_escape stub. If opline->handler has been overwritten with JIT code (e.g. a function entry trace), this creates an infinite loop. Fix by dispatching to the original VM handler (orig_handler from the trace extension) instead of going through the trace_escape stub. This avoids the extra IS_UNDEF guard on every property read while correctly handling the rare IS_UNDEF case during deoptimization. Also set current_op_array in zend_jit_trace_exit_to_vm so that the blacklisted exit deoptimizer can resolve orig_handler, covering the case where side trace compilation is exhausted. Closes phpGH-21368.
28e99da to
1d9aba6
Compare
|
@dstogov Thanks for the review and the better fix. Updated the PR based on your approach — One addition: I also set |
| ir_ref ref = ir_CONST_ADDR(ZEND_OP_TRACE_INFO((opline - 1), offset)->orig_handler); | ||
| if (GCC_GLOBAL_REGS) { | ||
| ir_TAILCALL(IR_VOID, ref); | ||
| } else { | ||
| #if defined(IR_TARGET_X86) | ||
| ref = ir_CAST_FC_FUNC(ref); | ||
| #endif | ||
| ref = ir_CALL_1(IR_I32, ref, jit_FP(jit)); | ||
| ir_GUARD(ir_GE(ref, ir_CONST_I32(0)), jit_STUB_ADDR(jit, jit_stub_trace_halt)); | ||
| ir_RETURN(ir_CONST_I32(1)); // ZEND_VM_ENTER | ||
| } |
There was a problem hiding this comment.
I see, my patch didn't handle all VM kinds right.
Anyway, I'm not completely sure if the "else" part of your patch is going to work for all VM kinds.
There was a problem hiding this comment.
The else branch matches the dispatch pattern used by zend_jit_trace_exit_stub (line 2527) and zend_jit_trace_return (line 17148) on PHP-8.4: ir_CALL_1(IR_I32, ...) + ir_GUARD + ir_RETURN(ZEND_VM_ENTER). On 8.4, JIT only runs with HYBRID VM kind — GCC_GLOBAL_REGS vs CALL convention covers both paths.
For a master forward-port this would need zend_jit_tailcall_handler/zend_jit_vm_enter to also handle ZEND_VM_KIND_TAILCALL.
There was a problem hiding this comment.
It seems we can use TAILCALL for the else part.
ext/opcache/jit/zend_jit_trace.c
Outdated
| /* Set current_op_array so that zend_jit_escape_if_undef() can | ||
| * resolve orig_handler to avoid dispatching to JIT-overwritten | ||
| * opline->handler (which would cause an infinite loop). */ | ||
| ctx.current_op_array = zend_jit_traces[zend_jit_traces[trace_num].root].op_array; | ||
|
|
There was a problem hiding this comment.
if ctx.current_op_array may be uninitialized, better pass op_array into zend_jit_escape_if_undef() as a parameter.
There was a problem hiding this comment.
Done — zend_jit_escape_if_undef now takes op_array as an explicit parameter. The call site passes jit->current_op_array, which is set by zend_jit_trace_start in the side-trace path and by the new initialization in zend_jit_trace_exit_to_vm for the blacklist path.
There was a problem hiding this comment.
I meant not to set ctx.current_op_array, but pass the value directly into zend_jit_escape_if_undef() completely removing this chunk.
When the JIT defers the IS_UNDEF check for FETCH_OBJ_R to the result type guard, the deoptimization escape path dispatches to opline->handler via the trace_escape stub. If opline->handler has been overwritten with JIT code (e.g. a function entry trace), this creates an infinite loop. Fix by dispatching to the original VM handler (orig_handler from the trace extension) instead of going through the trace_escape stub. This avoids the extra IS_UNDEF guard on every property read while correctly handling the rare IS_UNDEF case during deoptimization. Also set current_op_array in zend_jit_trace_exit_to_vm so that the blacklisted exit deoptimizer can resolve orig_handler, covering the case where side trace compilation is exhausted. Closes phpGH-21368.
1d9aba6 to
c95f761
Compare
dstogov
left a comment
There was a problem hiding this comment.
I reduced the patch to
diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index 3ddaa027088..f5e821c2c66 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -8066,7 +8066,7 @@ static int zend_jit_defined(zend_jit_ctx *jit, const zend_op *opline, uint8_t sm
return 1;
}
-static int zend_jit_escape_if_undef(zend_jit_ctx *jit, int var, uint32_t flags, const zend_op *opline, int8_t reg)
+static int zend_jit_escape_if_undef(zend_jit_ctx *jit, int var, uint32_t flags, const zend_op *opline, const zend_op_array *op_array, int8_t reg)
{
zend_jit_addr reg_addr = ZEND_ADDR_REF_ZVAL(zend_jit_deopt_rload(jit, IR_ADDR, reg));
ir_ref if_def = ir_IF(jit_Z_TYPE(jit, reg_addr));
@@ -8089,7 +8089,21 @@ static int zend_jit_escape_if_undef(zend_jit_ctx *jit, int var, uint32_t flags,
}
jit_LOAD_IP_ADDR(jit, opline - 1);
- ir_IJMP(jit_STUB_ADDR(jit, jit_stub_trace_escape));
+
+ /* We can't use trace_escape() because opcode handler may be overridden by JIT */
+ zend_jit_op_array_trace_extension *jit_extension =
+ (zend_jit_op_array_trace_extension*)ZEND_FUNC_INFO(op_array);
+ size_t offset = jit_extension->offset;
+ if (GCC_GLOBAL_REGS) {
+ ir_ref ref = ir_CONST_ADDR(ZEND_OP_TRACE_INFO((opline - 1), offset)->orig_handler);
+ ir_TAILCALL(IR_VOID, ref);
+ } else {
+ ir_ref ref = ir_CONST_ADDR(ZEND_OP_TRACE_INFO((opline - 1), offset)->call_handler);
+#if defined(IR_TARGET_X86)
+ ref = ir_CAST_FC_FUNC(ref);
+#endif
+ ir_TAILCALL_1(IR_I32, ref, jit_FP(jit));
+ }
ir_IF_TRUE(if_def);
diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c
index 03b59eea615..b5d980ca5af 100644
--- a/ext/opcache/jit/zend_jit_trace.c
+++ b/ext/opcache/jit/zend_jit_trace.c
@@ -3603,7 +3603,7 @@ static int zend_jit_trace_deoptimization(
ZEND_ASSERT(STACK_FLAGS(parent_stack, check2) == ZREG_ZVAL_COPY);
ZEND_ASSERT(reg != ZREG_NONE);
- if (!zend_jit_escape_if_undef(jit, check2, flags, opline, reg)) {
+ if (!zend_jit_escape_if_undef(jit, check2, flags, opline, exit_info->op_array, reg)) {
return 0;
}
if (!zend_jit_restore_zval(jit, EX_NUM_TO_VAR(check2), reg)) {Do you see any problems?
Merge to master is going to be not trivial because of tail-call VM...
ext/opcache/jit/zend_jit_trace.c
Outdated
| /* Set current_op_array so that zend_jit_escape_if_undef() can | ||
| * resolve orig_handler to avoid dispatching to JIT-overwritten | ||
| * opline->handler (which would cause an infinite loop). */ | ||
| ctx.current_op_array = zend_jit_traces[zend_jit_traces[trace_num].root].op_array; | ||
|
|
There was a problem hiding this comment.
I meant not to set ctx.current_op_array, but pass the value directly into zend_jit_escape_if_undef() completely removing this chunk.
| ir_ref ref = ir_CONST_ADDR(ZEND_OP_TRACE_INFO((opline - 1), offset)->orig_handler); | ||
| if (GCC_GLOBAL_REGS) { | ||
| ir_TAILCALL(IR_VOID, ref); | ||
| } else { | ||
| #if defined(IR_TARGET_X86) | ||
| ref = ir_CAST_FC_FUNC(ref); | ||
| #endif | ||
| ref = ir_CALL_1(IR_I32, ref, jit_FP(jit)); | ||
| ir_GUARD(ir_GE(ref, ir_CONST_I32(0)), jit_STUB_ADDR(jit, jit_stub_trace_halt)); | ||
| ir_RETURN(ir_CONST_I32(1)); // ZEND_VM_ENTER | ||
| } |
There was a problem hiding this comment.
It seems we can use TAILCALL for the else part.
It seems we may always use |
When the JIT defers the IS_UNDEF check for FETCH_OBJ_R to the result type guard, the deoptimization escape path dispatches to opline->handler via the trace_escape stub. If opline->handler has been overwritten with JIT code (e.g. a function entry trace), this creates an infinite loop. Fix by dispatching to the original VM handler (orig_handler from the trace extension) instead of going through the trace_escape stub. This avoids the extra IS_UNDEF guard on every property read while correctly handling the rare IS_UNDEF case during deoptimization. Also set current_op_array in zend_jit_trace_exit_to_vm so that the blacklisted exit deoptimizer can resolve orig_handler, covering the case where side trace compilation is exhausted. Closes phpGH-21368.
|
Updated — adopted all suggestions:
|
|
It's something wrong with github. |
c95f761 to
4cebe12
Compare
There was wrong branch set for diff, should be ok now. |
|
I don't see problems. |
|
Cool, thanks your help :) |
Fixes #21267
When the JIT defers the
IS_UNDEFcheck forFETCH_OBJ_Rto the result type guard (theMAY_BE_GUARD+current_framepath), the deoptimization escape dispatches toopline->handlervia thetrace_escapestub. Ifopline->handlerhas been overwritten with JIT code (e.g. a function entry trace for a zero-param method whereFETCH_OBJ_Ris the first opline), this creates an infinite loop since no exit counters are bumped and no blacklisting occurs.The fix changes
zend_jit_escape_if_undef()to dispatch directly toorig_handler(the original VM handler stored in the trace extension) instead of going throughtrace_escape. This preserves the optimization of deferring theIS_UNDEFcheck to the result type guard while correctly handling the rare IS_UNDEF case during deoptimization.Additionally, sets
current_op_arrayinzend_jit_trace_exit_to_vm()so that the blacklisted exit deoptimizer can also resolveorig_handler, covering the case where side trace compilation is exhausted (jit_max_side_tracesreached).Test cases cover both the side trace path and the blacklisted exit path.