From 12c03756139dc6fcd9c892293ef28cde35cad1eb Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 7 Jan 2025 22:48:31 +0100 Subject: [PATCH 1/2] Fix JIT for overridden properties with added hooks Fixes GH-17376 --- Zend/tests/gh17376.phpt | 26 ++++++++++++++++++++++++++ Zend/zend_inheritance.c | 5 ----- ext/opcache/jit/zend_jit_ir.c | 25 ++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 6 deletions(-) create mode 100644 Zend/tests/gh17376.phpt diff --git a/Zend/tests/gh17376.phpt b/Zend/tests/gh17376.phpt new file mode 100644 index 000000000000..d9f4ab5fc03d --- /dev/null +++ b/Zend/tests/gh17376.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-17376: Child classes may add hooks to plain properties +--FILE-- + 2; + } +} + +function test(A $a) { + var_dump($a->prop); +} + +test(new A); +test(new B); + +?> +--EXPECT-- +int(1) +int(2) diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 8197e6162843..d2118cd4e7db 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1653,11 +1653,6 @@ void zend_build_properties_info_table(zend_class_entry *ce) table, parent_table, sizeof(zend_property_info *) * ce->parent->default_properties_count ); - - /* Child did not add any new properties, we are done */ - if (ce->default_properties_count == ce->parent->default_properties_count) { - return; - } } ZEND_HASH_MAP_FOREACH_PTR(&ce->properties_info, prop) { diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index e14345215f6f..beef821f698e 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -14291,6 +14291,29 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, } } } else { + /* Child classes may add hooks to property. */ + if (ce_is_instanceof && !(prop_info->flags & ZEND_ACC_FINAL)) { + int prop_info_offset = + (((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*)); + ir_ref prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce))); + prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(prop_info_ref, offsetof(zend_class_entry, properties_info_table))); + prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(prop_info_ref, prop_info_offset)); + ir_ref is_same_prop_info = ir_EQ(prop_info_ref, ir_CONST_ADDR(prop_info)); + if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) { + int32_t exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_TO_VM); + const void *exit_addr = zend_jit_trace_get_exit_addr(exit_point); + if (!exit_addr) { + return 0; + } + ir_GUARD(is_same_prop_info, ir_CONST_ADDR(exit_addr)); + } else { + ir_ref if_same_prop_info = ir_IF(is_same_prop_info); + ir_IF_FALSE_cold(if_same_prop_info); + ir_END_list(slow_inputs); + ir_IF_TRUE(if_same_prop_info); + } + } + prop_ref = ir_ADD_OFFSET(obj_ref, prop_info->offset); prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref); if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) { @@ -14435,7 +14458,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, SET_STACK_REG(JIT_G(current_frame)->stack, EX_VAR_TO_NUM(opline->op1.var), ZREG_NONE); } - if (JIT_G(trigger) != ZEND_JIT_ON_HOT_TRACE || !prop_info) { + if (slow_inputs) { ir_MERGE_list(slow_inputs); jit_SET_EX_OPLINE(jit, opline); From 65f9f9d4e04b84531e48a0c4d8b52e0b87ac9b68 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Fri, 10 Jan 2025 23:47:12 +0100 Subject: [PATCH 2/2] Switch to cheaper guard that doesn't require repetition --- Zend/Optimizer/zend_inference.h | 1 + Zend/tests/gh17376.phpt | 57 +++++++++++++++++++++-- ext/opcache/jit/zend_jit_ir.c | 81 ++++++++++++++++++++++++--------- 3 files changed, 115 insertions(+), 24 deletions(-) diff --git a/Zend/Optimizer/zend_inference.h b/Zend/Optimizer/zend_inference.h index 666abc586592..fd35e9c36dc0 100644 --- a/Zend/Optimizer/zend_inference.h +++ b/Zend/Optimizer/zend_inference.h @@ -31,6 +31,7 @@ #define MAY_BE_PACKED_GUARD (1<<27) /* needs packed array guard */ #define MAY_BE_CLASS_GUARD (1<<27) /* needs class guard */ #define MAY_BE_GUARD (1<<28) /* needs type guard */ +#define MAY_BE_NEW_HOOKS_GUARD (1<<29) /* object has new guards compared to comp-time class */ #define MAY_HAVE_DTOR \ (MAY_BE_OBJECT|MAY_BE_RESOURCE \ diff --git a/Zend/tests/gh17376.phpt b/Zend/tests/gh17376.phpt index d9f4ab5fc03d..1ba307611057 100644 --- a/Zend/tests/gh17376.phpt +++ b/Zend/tests/gh17376.phpt @@ -8,19 +8,70 @@ class A { } class B extends A { - public $prop { - get => 2; + public $prop = 1 { + get { + echo __METHOD__, "\n"; + return $this->prop; + } + set { + echo __METHOD__, "\n"; + $this->prop = $value; + } } } function test(A $a) { + echo "read\n"; var_dump($a->prop); + echo "write\n"; + $a->prop = 42; + echo "read-write\n"; + $a->prop += 43; + echo "pre-inc\n"; + ++$a->prop; + echo "pre-dec\n"; + --$a->prop; + echo "post-inc\n"; + $a->prop++; + echo "post-dec\n"; + $a->prop--; } +echo "A\n"; test(new A); +echo "\nB\n"; test(new B); ?> --EXPECT-- +A +read int(1) -int(2) +write +read-write +pre-inc +pre-dec +post-inc +post-dec + +B +read +B::$prop::get +int(1) +write +B::$prop::set +read-write +B::$prop::get +B::$prop::set +pre-inc +B::$prop::get +B::$prop::set +pre-dec +B::$prop::get +B::$prop::set +post-inc +B::$prop::get +B::$prop::set +post-dec +B::$prop::get +B::$prop::set diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index beef821f698e..264c2722567d 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -14024,6 +14024,52 @@ static int zend_jit_class_guard(zend_jit_ctx *jit, const zend_op *opline, ir_ref return 1; } +/* Child classes that add new hooks to existing properties will violate the + * assumption that properties may be accessed directly. This guard makes sure no + * new hooked properties have been created in the object, compared to the trace + * class. For function JIT, we just pick the slow path. */ +static bool zend_jit_new_hooks_guard( + zend_jit_ctx *jit, + const zend_op *opline, + zend_ssa *ssa, + const zend_ssa_op *ssa_op, + ir_ref obj_ref, + uint32_t op1_info, + zend_class_entry *ce, + bool ce_is_instanceof, + zend_property_info *prop_info, + ir_ref *slow_inputs +) { + if ((JIT_G(trigger) != ZEND_JIT_ON_HOT_TRACE || !(op1_info & MAY_BE_NEW_HOOKS_GUARD)) + && ce_is_instanceof + && !(prop_info->flags & ZEND_ACC_FINAL)) { + ir_ref num_hooked_props = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce))); + num_hooked_props = ir_LOAD_U32(ir_ADD_OFFSET(num_hooked_props, offsetof(zend_class_entry, num_hooked_props))); + ir_ref has_same_num_hooked_props = ir_EQ(num_hooked_props, ir_CONST_U32(ce->num_hooked_props)); + if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) { + /* We use EXIT_TO_VM to avoid creating new side-traces that would + * just immediately fail. To make side-traces work properly, the + * side-trace needs to create a class guard for the given subclass. */ + int32_t exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_TO_VM); + const void *exit_addr = zend_jit_trace_get_exit_addr(exit_point); + if (!exit_addr) { + return false; + } + ir_GUARD(has_same_num_hooked_props, ir_CONST_ADDR(exit_addr)); + if (ssa->var_info && ssa_op->op1_use >= 0) { + ssa->var_info[ssa_op->op1_use].type |= MAY_BE_NEW_HOOKS_GUARD; + } + } else { + ir_ref if_same_num_hooked_props = ir_IF(has_same_num_hooked_props); + ir_IF_FALSE_cold(if_same_num_hooked_props); + ir_END_list(*slow_inputs); + ir_IF_TRUE(if_same_num_hooked_props); + } + } + + return true; +} + static int zend_jit_fetch_obj(zend_jit_ctx *jit, const zend_op *opline, const zend_op_array *op_array, @@ -14291,27 +14337,8 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, } } } else { - /* Child classes may add hooks to property. */ - if (ce_is_instanceof && !(prop_info->flags & ZEND_ACC_FINAL)) { - int prop_info_offset = - (((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*)); - ir_ref prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce))); - prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(prop_info_ref, offsetof(zend_class_entry, properties_info_table))); - prop_info_ref = ir_LOAD_A(ir_ADD_OFFSET(prop_info_ref, prop_info_offset)); - ir_ref is_same_prop_info = ir_EQ(prop_info_ref, ir_CONST_ADDR(prop_info)); - if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) { - int32_t exit_point = zend_jit_trace_get_exit_point(opline, ZEND_JIT_EXIT_TO_VM); - const void *exit_addr = zend_jit_trace_get_exit_addr(exit_point); - if (!exit_addr) { - return 0; - } - ir_GUARD(is_same_prop_info, ir_CONST_ADDR(exit_addr)); - } else { - ir_ref if_same_prop_info = ir_IF(is_same_prop_info); - ir_IF_FALSE_cold(if_same_prop_info); - ir_END_list(slow_inputs); - ir_IF_TRUE(if_same_prop_info); - } + if (!zend_jit_new_hooks_guard(jit, opline, ssa, ssa_op, obj_ref, op1_info, ce, ce_is_instanceof, prop_info, &slow_inputs)) { + return 0; } prop_ref = ir_ADD_OFFSET(obj_ref, prop_info->offset); @@ -14771,6 +14798,10 @@ static int zend_jit_assign_obj(zend_jit_ctx *jit, ir_IF_FALSE(if_has_prop_info); } } else { + if (!zend_jit_new_hooks_guard(jit, opline, ssa, ssa_op, obj_ref, op1_info, ce, ce_is_instanceof, prop_info, &slow_inputs)) { + return 0; + } + prop_ref = ir_ADD_OFFSET(obj_ref, prop_info->offset); prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref); /* With the exception of __clone(), readonly assignment always happens on IS_UNDEF, doding @@ -15095,6 +15126,10 @@ static int zend_jit_assign_obj_op(zend_jit_ctx *jit, } prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref); } else { + if (!zend_jit_new_hooks_guard(jit, opline, ssa, ssa_op, obj_ref, op1_info, ce, ce_is_instanceof, prop_info, &slow_inputs)) { + return 0; + } + prop_ref = ir_ADD_OFFSET(obj_ref, prop_info->offset); prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref); @@ -15518,6 +15553,10 @@ static int zend_jit_incdec_obj(zend_jit_ctx *jit, } prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref); } else { + if (!zend_jit_new_hooks_guard(jit, opline, ssa, ssa_op, obj_ref, op1_info, ce, ce_is_instanceof, prop_info, &slow_inputs)) { + return 0; + } + prop_ref = ir_ADD_OFFSET(obj_ref, prop_info->offset); prop_addr = ZEND_ADDR_REF_ZVAL(prop_ref);