Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JIT for overridden properties with added hooks #17395

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Zend/Optimizer/zend_inference.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
77 changes: 77 additions & 0 deletions Zend/tests/gh17376.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
--TEST--
GH-17376: Child classes may add hooks to plain properties
--FILE--
<?php

class A {
public $prop = 1;
}

class B extends A {
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)
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
5 changes: 0 additions & 5 deletions Zend/zend_inheritance.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move this to a separate PR, but IMO this should still be fixed.

}

ZEND_HASH_MAP_FOREACH_PTR(&ce->properties_info, prop) {
Expand Down
64 changes: 63 additions & 1 deletion ext/opcache/jit/zend_jit_ir.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
nielsdos marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand Down Expand Up @@ -14291,6 +14337,10 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit,
}
}
} 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);
if (JIT_G(trigger) == ZEND_JIT_ON_HOT_TRACE) {
Expand Down Expand Up @@ -14435,7 +14485,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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could technically be reverted, but seems more logical.

ir_MERGE_list(slow_inputs);
jit_SET_EX_OPLINE(jit, opline);

Expand Down Expand Up @@ -14748,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
Expand Down Expand Up @@ -15072,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);

Expand Down Expand Up @@ -15495,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);

Expand Down
Loading