From fdcfc0a30cc9ef527c7559e988bcdf3e24ffab8c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 23 Feb 2025 22:36:17 +0100 Subject: [PATCH 1/5] Fix GH-15834: Segfault with hook "simple get" cache slot and minimal JIT The FETCH_OBJ_R VM handler has an optimization that directly enters into a hook if it is a simpler getter hook. This is not compatible with the minimal JIT because the minimal JIT will try to continue executing the opcodes after the FETCH_OBJ_R. To solve this, we check whether the opcode is still the expected one after the execution of the VM handler. If it is not, we know that we are going to execute a simple hook. In that case, exit to the VM. --- ext/opcache/jit/zend_jit.c | 17 +++++++++++++++++ ext/opcache/tests/jit/gh15834.phpt | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 ext/opcache/tests/jit/gh15834.phpt diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 77694e07d132e..56e73b2401357 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -2709,6 +2709,23 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op /* We skip over the DO_FCALL, so decrement call_level ourselves. */ call_level--; } + break; + case ZEND_FETCH_OBJ_R: + if (!zend_jit_handler(&ctx, opline, + zend_may_throw(opline, ssa_op, op_array, ssa))) { + goto jit_failure; + } + + /* If a simple hook is called, exit to the VM. */ + ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); + ir_IF_FALSE(if_hook_enter); + if (GCC_GLOBAL_REGS || zend_jit_vm_kind == ZEND_VM_KIND_HYBRID) { + ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); + } else { + ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ + } + ir_IF_TRUE(if_hook_enter); + break; default: if (!zend_jit_handler(&ctx, opline, diff --git a/ext/opcache/tests/jit/gh15834.phpt b/ext/opcache/tests/jit/gh15834.phpt new file mode 100644 index 0000000000000..a8cc6ce7a15d6 --- /dev/null +++ b/ext/opcache/tests/jit/gh15834.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-15834 (Segfault with hook "simple get" cache slot and minimal JIT) +--EXTENSIONS-- +opcache +--INI-- +opcache.jit=1111 +--FILE-- + $this->_prop; + } +} + +$a = new A; +for ($i=0;$i<5;$i++) { + echo $a->prop; + $a->_prop++; +} +?> +--EXPECT-- +12345 From a590953301d119ccfbfab952699b6ed381c91972 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 23 Feb 2025 23:24:38 +0100 Subject: [PATCH 2/5] Simplify check --- ext/opcache/jit/zend_jit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 56e73b2401357..9da2e2add76ba 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -2719,7 +2719,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op /* If a simple hook is called, exit to the VM. */ ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); ir_IF_FALSE(if_hook_enter); - if (GCC_GLOBAL_REGS || zend_jit_vm_kind == ZEND_VM_KIND_HYBRID) { + if (GCC_GLOBAL_REGS) { ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); } else { ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ From d66f9f57010590e86330240ea66521466af0f230 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 27 Feb 2025 22:32:52 +0100 Subject: [PATCH 3/5] Small optimization --- ext/opcache/jit/zend_jit.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 9da2e2add76ba..07b77377fcd9d 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -2335,11 +2335,6 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op case ZEND_FETCH_OBJ_R: case ZEND_FETCH_OBJ_IS: case ZEND_FETCH_OBJ_W: - if (opline->op2_type != IS_CONST - || Z_TYPE_P(RT_CONSTANT(opline, opline->op2)) != IS_STRING - || Z_STRVAL_P(RT_CONSTANT(opline, opline->op2))[0] == '\0') { - break; - } ce = NULL; ce_is_instanceof = 0; on_this = 0; @@ -2369,6 +2364,11 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op } } } + if (opline->op2_type != IS_CONST + || Z_TYPE_P(RT_CONSTANT(opline, opline->op2)) != IS_STRING + || Z_STRVAL_P(RT_CONSTANT(opline, opline->op2))[0] == '\0') { + break; + } if (!zend_jit_fetch_obj(&ctx, opline, op_array, ssa, ssa_op, op1_info, op1_addr, 0, ce, ce_is_instanceof, on_this, 0, 0, NULL, RES_REG_ADDR(), IS_UNKNOWN, @@ -2716,15 +2716,25 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op goto jit_failure; } - /* If a simple hook is called, exit to the VM. */ - ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); - ir_IF_FALSE(if_hook_enter); - if (GCC_GLOBAL_REGS) { - ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); - } else { - ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ + if (JIT_G(opt_level) < ZEND_JIT_LEVEL_INLINE) { + if (opline->op1_type == IS_UNUSED) { + ce = op_array->scope; + } else { + ce = NULL; + } + } + + if (!ce || !(ce->ce_flags & ZEND_ACC_FINAL) || ce->num_hooked_props > 0) { + /* If a simple hook is called, exit to the VM. */ + ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); + ir_IF_FALSE(if_hook_enter); + if (GCC_GLOBAL_REGS) { + ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); + } else { + ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ + } + ir_IF_TRUE(if_hook_enter); } - ir_IF_TRUE(if_hook_enter); break; default: From 065961bc33aad312dee2e8c47115dbec491eecc8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 3 Mar 2025 08:25:13 +0100 Subject: [PATCH 4/5] Fix false positive build warning --- ext/opcache/jit/zend_jit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 07b77377fcd9d..de9a4b89c51c5 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -1316,7 +1316,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op uint32_t target_label, target_label2; uint32_t op1_info, op1_def_info, op2_info, res_info, res_use_info, op1_mem_info; zend_jit_addr op1_addr, op1_def_addr, op2_addr, op2_def_addr, res_addr; - zend_class_entry *ce; + zend_class_entry *ce = NULL; bool ce_is_instanceof; bool on_this; From 19f7441150b4265a5eb34ca185057dd7d10f6706 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 4 Mar 2025 23:20:25 +0100 Subject: [PATCH 5/5] Optimize --- ext/opcache/jit/zend_jit.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index de9a4b89c51c5..2d4ef0bf4fc38 100644 --- a/ext/opcache/jit/zend_jit.c +++ b/ext/opcache/jit/zend_jit.c @@ -2716,24 +2716,27 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op goto jit_failure; } - if (JIT_G(opt_level) < ZEND_JIT_LEVEL_INLINE) { - if (opline->op1_type == IS_UNUSED) { - ce = op_array->scope; - } else { - ce = NULL; + /* Cache slot is only used for IS_CONST op2, so only that can result in hook fast path. */ + if (opline->op2_type == IS_CONST) { + if (JIT_G(opt_level) < ZEND_JIT_LEVEL_INLINE) { + if (opline->op1_type == IS_UNUSED) { + ce = op_array->scope; + } else { + ce = NULL; + } } - } - if (!ce || !(ce->ce_flags & ZEND_ACC_FINAL) || ce->num_hooked_props > 0) { - /* If a simple hook is called, exit to the VM. */ - ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); - ir_IF_FALSE(if_hook_enter); - if (GCC_GLOBAL_REGS) { - ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); - } else { - ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ + if (!ce || !(ce->ce_flags & ZEND_ACC_FINAL) || ce->num_hooked_props > 0) { + /* If a simple hook is called, exit to the VM. */ + ir_ref if_hook_enter = ir_IF(jit_CMP_IP(jit, IR_EQ, opline + 1)); + ir_IF_FALSE(if_hook_enter); + if (GCC_GLOBAL_REGS) { + ir_TAILCALL(IR_VOID, ir_LOAD_A(jit_IP(jit))); + } else { + ir_RETURN(ir_CONST_I32(1)); /* ZEND_VM_ENTER */ + } + ir_IF_TRUE(if_hook_enter); } - ir_IF_TRUE(if_hook_enter); } break;