From 9ddc25afe3b8c67338c06a80985bfdeb2d2bd52b Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 6 Mar 2025 19:33:41 +0100 Subject: [PATCH 1/6] Simplify array_any(), array_all(), array_find(), array_find_key() (#17978) By returning something more semantically meaningful that SUCCESS/FAILURE we can avoid refcounting for array_all() and array_any(). Also we can avoid resetting the input values to UNDEF. --- ext/standard/array.c | 76 ++++++++++++++------------------------------ 1 file changed, 23 insertions(+), 53 deletions(-) diff --git a/ext/standard/array.c b/ext/standard/array.c index a1890b05b57d3..8462492651310 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -6591,7 +6591,13 @@ PHP_FUNCTION(array_filter) /* }}} */ /* {{{ Internal function to find an array element for a user closure. */ -static zend_result php_array_find(const HashTable *array, zend_fcall_info fci, zend_fcall_info_cache fci_cache, zval *result_key, zval *result_value, bool negate_condition) +enum php_array_find_result { + PHP_ARRAY_FIND_EXCEPTION = -1, + PHP_ARRAY_FIND_NONE = 0, + PHP_ARRAY_FIND_SOME = 1, +}; + +static enum php_array_find_result php_array_find(const HashTable *array, zend_fcall_info fci, zend_fcall_info_cache fci_cache, zval *result_key, zval *result_value, bool negate_condition) { zend_ulong num_key; zend_string *str_key; @@ -6599,16 +6605,8 @@ static zend_result php_array_find(const HashTable *array, zend_fcall_info fci, z zval args[2]; zval *operand; - if (result_value != NULL) { - ZVAL_UNDEF(result_value); - } - - if (result_key != NULL) { - ZVAL_UNDEF(result_key); - } - if (zend_hash_num_elements(array) == 0) { - return SUCCESS; + return PHP_ARRAY_FIND_NONE; } ZEND_ASSERT(ZEND_FCI_INITIALIZED(fci)); @@ -6631,7 +6629,7 @@ static zend_result php_array_find(const HashTable *array, zend_fcall_info fci, z ZEND_ASSERT(result == SUCCESS); if (UNEXPECTED(Z_ISUNDEF(retval))) { - return FAILURE; + return PHP_ARRAY_FIND_EXCEPTION; } bool retval_true = zend_is_true(&retval); @@ -6649,103 +6647,75 @@ static zend_result php_array_find(const HashTable *array, zend_fcall_info fci, z ZVAL_COPY(result_key, &args[1]); } - break; + return PHP_ARRAY_FIND_SOME; } } ZEND_HASH_FOREACH_END(); - return SUCCESS; + return PHP_ARRAY_FIND_NONE; } /* }}} */ /* {{{ Search within an array and returns the first found element value. */ PHP_FUNCTION(array_find) { - zval *array = NULL; + HashTable *array; zend_fcall_info fci; zend_fcall_info_cache fci_cache = empty_fcall_info_cache; ZEND_PARSE_PARAMETERS_START(2, 2) - Z_PARAM_ARRAY(array) + Z_PARAM_ARRAY_HT(array) Z_PARAM_FUNC(fci, fci_cache) ZEND_PARSE_PARAMETERS_END(); - if (php_array_find(Z_ARR_P(array), fci, fci_cache, NULL, return_value, false) != SUCCESS) { - RETURN_THROWS(); - } - - if (Z_TYPE_P(return_value) == IS_UNDEF) { - RETURN_NULL(); - } + php_array_find(array, fci, fci_cache, NULL, return_value, false); } /* }}} */ /* {{{ Search within an array and returns the first found element key. */ PHP_FUNCTION(array_find_key) { - zval *array = NULL; + HashTable *array; zend_fcall_info fci; zend_fcall_info_cache fci_cache = empty_fcall_info_cache; ZEND_PARSE_PARAMETERS_START(2, 2) - Z_PARAM_ARRAY(array) + Z_PARAM_ARRAY_HT(array) Z_PARAM_FUNC(fci, fci_cache) ZEND_PARSE_PARAMETERS_END(); - if (php_array_find(Z_ARR_P(array), fci, fci_cache, return_value, NULL, false) != SUCCESS) { - RETURN_THROWS(); - } - - if (Z_TYPE_P(return_value) == IS_UNDEF) { - RETURN_NULL(); - } + php_array_find(array, fci, fci_cache, return_value, NULL, false); } /* }}} */ /* {{{ Checks if at least one array element satisfies a callback function. */ PHP_FUNCTION(array_any) { - zval *array = NULL; + HashTable *array; zend_fcall_info fci; zend_fcall_info_cache fci_cache = empty_fcall_info_cache; ZEND_PARSE_PARAMETERS_START(2, 2) - Z_PARAM_ARRAY(array) + Z_PARAM_ARRAY_HT(array) Z_PARAM_FUNC(fci, fci_cache) ZEND_PARSE_PARAMETERS_END(); - if (php_array_find(Z_ARR_P(array), fci, fci_cache, return_value, NULL, false) != SUCCESS) { - RETURN_THROWS(); - } - - bool retval = !Z_ISUNDEF_P(return_value); - if (Z_TYPE_P(return_value) == IS_STRING) { - zval_ptr_dtor_str(return_value); - } - RETURN_BOOL(retval); + RETURN_BOOL(php_array_find(array, fci, fci_cache, NULL, NULL, false) == PHP_ARRAY_FIND_SOME); } /* }}} */ /* {{{ Checks if all array elements satisfy a callback function. */ PHP_FUNCTION(array_all) { - zval *array = NULL; + HashTable *array; zend_fcall_info fci; zend_fcall_info_cache fci_cache = empty_fcall_info_cache; ZEND_PARSE_PARAMETERS_START(2, 2) - Z_PARAM_ARRAY(array) + Z_PARAM_ARRAY_HT(array) Z_PARAM_FUNC(fci, fci_cache) ZEND_PARSE_PARAMETERS_END(); - if (php_array_find(Z_ARR_P(array), fci, fci_cache, return_value, NULL, true) != SUCCESS) { - RETURN_THROWS(); - } - - bool retval = Z_ISUNDEF_P(return_value); - if (Z_TYPE_P(return_value) == IS_STRING) { - zval_ptr_dtor_str(return_value); - } - RETURN_BOOL(retval); + RETURN_BOOL(php_array_find(array, fci, fci_cache, NULL, NULL, true) == PHP_ARRAY_FIND_NONE); } /* }}} */ From f6c2e40a11c9aba3ed9e9512c66de4e0b1f50343 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 2/6] 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. Closes GH-17909. --- NEWS | 4 +++ ext/opcache/jit/zend_jit.c | 42 +++++++++++++++++++++++++----- ext/opcache/tests/jit/gh15834.phpt | 23 ++++++++++++++++ 3 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 ext/opcache/tests/jit/gh15834.phpt diff --git a/NEWS b/NEWS index 1f2eea0253065..fa4018bf8ed96 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,10 @@ PHP NEWS . Fixed bug GH-17913 (ReflectionFunction::isDeprecated() returns incorrect results for closures created from magic __call()). (timwolla) +- Opcache: + . Fixed bug GH-15834 (Segfault with hook "simple get" cache slot and minimal + JIT). (nielsdos) + - Standard: . Fix memory leaks in array_any() / array_all(). (nielsdos) diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c index 77694e07d132e..2d4ef0bf4fc38 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; @@ -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, @@ -2709,6 +2709,36 @@ 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; + } + + /* 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 */ + } + 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 d9e39f5c6f70a4de3c201b436f447dc069d49e0e Mon Sep 17 00:00:00 2001 From: Dmitry Stogov Date: Thu, 6 Mar 2025 21:53:45 +0300 Subject: [PATCH 3/6] Fix JIT for INIT_STATIC_METHOD_CALL in a closure --- ext/opcache/jit/zend_jit_ir.c | 2 +- .../jit/init_static_method_call_001.phpt | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 ext/opcache/tests/jit/init_static_method_call_001.phpt diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index 4ef32415abe76..a3aae9b921f25 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -9353,7 +9353,7 @@ static int zend_jit_init_static_method_call(zend_jit_ctx *jit, if (opline->op1_type == IS_UNUSED && ((opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_PARENT || (opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF)) { - if (op_array->fn_flags & ZEND_ACC_STATIC) { + if (!op_array->scope || (op_array->fn_flags & ZEND_ACC_STATIC)) { scope_ref = ir_LOAD_A(jit_EX(This.value.ref)); } else { scope_ref = ir_LOAD_A(ir_ADD_OFFSET(ir_LOAD_A(jit_EX(This.value.ref)), offsetof(zend_object, ce))); diff --git a/ext/opcache/tests/jit/init_static_method_call_001.phpt b/ext/opcache/tests/jit/init_static_method_call_001.phpt new file mode 100644 index 0000000000000..b6f1b95cc1e23 --- /dev/null +++ b/ext/opcache/tests/jit/init_static_method_call_001.phpt @@ -0,0 +1,24 @@ +--TEST-- +JIT INIT_STATIC_METHOD_CALL: 001 Invalid scope +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +--EXTENSIONS-- +opcache +--FILE-- + +--EXPECT-- +int(15) From cc70838dc923f2f5896d2faa1240fb2e5100d24f Mon Sep 17 00:00:00 2001 From: Dmitry Stogov Date: Thu, 6 Mar 2025 23:00:53 +0300 Subject: [PATCH 4/6] Merge IR IR commit: 0441281e95ce9736131eddc71ce666389dcccd4b --- ext/opcache/jit/ir/ir_gcm.c | 7 ++++--- ext/opcache/jit/ir/ir_sccp.c | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ext/opcache/jit/ir/ir_gcm.c b/ext/opcache/jit/ir/ir_gcm.c index be8744ef198fd..396ba2d7f7c7b 100644 --- a/ext/opcache/jit/ir/ir_gcm.c +++ b/ext/opcache/jit/ir/ir_gcm.c @@ -401,9 +401,10 @@ static bool ir_split_partially_dead_node(ir_ctx *ctx, ir_ref ref, uint32_t b) for (i = 1; i < clones_count; i++) { clones[i].ref = clone = ir_emit(ctx, insn->optx, insn->op1, insn->op2, insn->op3); insn = &ctx->ir_base[ref]; - if (insn->op1 > 0) ir_use_list_add(ctx, insn->op1, clone); - if (insn->op2 > 0) ir_use_list_add(ctx, insn->op2, clone); - if (insn->op3 > 0) ir_use_list_add(ctx, insn->op3, clone); + /* Depending on the flags in IR_OPS, these can be references or data. */ + if (insn->op1 > 0 && insn->inputs_count >= 1) ir_use_list_add(ctx, insn->op1, clone); + if (insn->op2 > 0 && insn->inputs_count >= 2) ir_use_list_add(ctx, insn->op2, clone); + if (insn->op3 > 0 && insn->inputs_count >= 3) ir_use_list_add(ctx, insn->op3, clone); } /* Reconstruct IR: Update DEF->USE lists, CFG mapping and etc */ diff --git a/ext/opcache/jit/ir/ir_sccp.c b/ext/opcache/jit/ir/ir_sccp.c index 4f364849390a2..b4f2257744145 100644 --- a/ext/opcache/jit/ir/ir_sccp.c +++ b/ext/opcache/jit/ir/ir_sccp.c @@ -3067,7 +3067,7 @@ static void ir_iter_optimize_if(ir_ctx *ctx, ir_ref ref, ir_insn *insn, ir_bitqu static void ir_iter_optimize_guard(ir_ctx *ctx, ir_ref ref, ir_insn *insn, ir_bitqueue *worklist) { - bool swap; + bool swap = 0; ir_ref condition = ir_iter_optimize_condition(ctx, insn->op1, insn->op2, &swap); if (swap) { From bac1ed6579cf7431d2c46957b69d7d757d406d82 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 6 Mar 2025 21:54:30 +0100 Subject: [PATCH 5/6] Add test for GH-17966 This was fixed via https://github.com/dstogov/ir/pull/109 which was merged in cc70838dc923f2f5896d2faa1240fb2e5100d24f. --- NEWS | 1 + ext/opcache/tests/jit/gh17966.phpt | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 ext/opcache/tests/jit/gh17966.phpt diff --git a/NEWS b/NEWS index fa4018bf8ed96..404290dcf615b 100644 --- a/NEWS +++ b/NEWS @@ -15,6 +15,7 @@ PHP NEWS - Opcache: . Fixed bug GH-15834 (Segfault with hook "simple get" cache slot and minimal JIT). (nielsdos) + . Fixed bug GH-17966 (Symfony JIT 1205 assertion failure). (nielsdos) - Standard: . Fix memory leaks in array_any() / array_all(). (nielsdos) diff --git a/ext/opcache/tests/jit/gh17966.phpt b/ext/opcache/tests/jit/gh17966.phpt new file mode 100644 index 0000000000000..f9983ad4c2b36 --- /dev/null +++ b/ext/opcache/tests/jit/gh17966.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-17966 (Symfony JIT 1205 assertion failure) +--EXTENSIONS-- +opcache +--INI-- +opcache.jit=1205 +--FILE-- + +--EXPECT-- +float(2.5) +float(1.25) From 972d6b6664ec19b1f8ae2a87e2c9a9fedf80e80d Mon Sep 17 00:00:00 2001 From: Dmitry Stogov Date: Fri, 7 Mar 2025 01:09:28 +0300 Subject: [PATCH 6/6] Fix JIT for INIT_STATIC_METHOD_CALL in a closure --- ext/opcache/jit/zend_jit_ir.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index a3aae9b921f25..ca3ac174e5db0 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -9353,8 +9353,18 @@ static int zend_jit_init_static_method_call(zend_jit_ctx *jit, if (opline->op1_type == IS_UNUSED && ((opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_PARENT || (opline->op1.num & ZEND_FETCH_CLASS_MASK) == ZEND_FETCH_CLASS_SELF)) { - if (!op_array->scope || (op_array->fn_flags & ZEND_ACC_STATIC)) { + if (op_array->fn_flags & ZEND_ACC_STATIC) { scope_ref = ir_LOAD_A(jit_EX(This.value.ref)); + } else if (op_array->fn_flags & ZEND_ACC_CLOSURE) { + ir_ref if_object, values = IR_UNUSED; + + if_object = ir_IF(ir_EQ(jit_Z_TYPE_ref(jit, jit_EX(This)), ir_CONST_U8(IS_OBJECT))); + ir_IF_TRUE(if_object); + ir_END_PHI_list(values, + ir_LOAD_A(ir_ADD_OFFSET(ir_LOAD_A(jit_EX(This.value.ref)), offsetof(zend_object, ce)))); + ir_IF_FALSE(if_object); + ir_END_PHI_list(values, ir_LOAD_A(jit_EX(This.value.ref))); + ir_PHI_list(values); } else { scope_ref = ir_LOAD_A(ir_ADD_OFFSET(ir_LOAD_A(jit_EX(This.value.ref)), offsetof(zend_object, ce))); }