Skip to content

Make property resolution direction-aware; amortize asymmetric visibility set checks into the runtime cache #22709

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.

Sign up for GitHub

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

Draft
hollyschilling wants to merge 1 commit into
base: master
Choose a base branch
from
+226 −65
Draft

Make property resolution direction-aware; amortize asymmetric visibility set checks into the runtime cache #22709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 25 additions & 3 deletions Zend/zend_execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,16 +1070,20 @@ ZEND_API bool zend_never_inline zend_verify_property_type(const zend_property_in
return i_zend_verify_property_type(info, property, strict);
}

static zend_never_inline zval* zend_assign_to_typed_prop(const zend_property_info *info, zval *property_val, zval *value, zend_refcounted **garbage_ptr EXECUTE_DATA_DC)
static zend_always_inline zval* zend_assign_to_typed_prop_ex(const zend_property_info *info, zval *property_val, zval *value, zend_refcounted **garbage_ptr, bool check_set_access EXECUTE_DATA_DC)
{
zval tmp;
const uint32_t guard_mask =
ZEND_ACC_READONLY | (check_set_access ? ZEND_ACC_PPP_SET_MASK : 0);

if (UNEXPECTED(info->flags & (ZEND_ACC_READONLY|ZEND_ACC_PPP_SET_MASK))) {
if (UNEXPECTED(info->flags & guard_mask)) {
if ((info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE)) {
zend_readonly_property_modification_error(info);
return &EG(uninitialized_zval);
}
if (info->flags & ZEND_ACC_PPP_SET_MASK && !zend_asymmetric_property_has_set_access(info)) {
if (check_set_access
&& (info->flags & ZEND_ACC_PPP_SET_MASK)
&& !zend_asymmetric_property_has_set_access(info)) {
zend_asymmetric_visibility_property_modification_error(info, "modify");
return &EG(uninitialized_zval);
}
Expand All @@ -1098,6 +1102,24 @@ static zend_never_inline zval* zend_assign_to_typed_prop(const zend_property_inf
return zend_assign_to_variable_ex(property_val, &tmp, IS_TMP_VAR, EX_USES_STRICT_TYPES(), garbage_ptr);
}

static zend_never_inline zval* zend_assign_to_typed_prop(const zend_property_info *info, zval *property_val, zval *value, zend_refcounted **garbage_ptr EXECUTE_DATA_DC)
{
return zend_assign_to_typed_prop_ex(info, property_val, value, garbage_ptr, true EXECUTE_DATA_CC);
}

/* For write sites whose runtime cache slot produced `info`: the slot is only
* populated when set access was verified at resolution time
* (zend_get_property_offset()), so the per-write asymmetric visibility check
* is redundant. */
static zend_never_inline zval* zend_assign_to_typed_prop_granted(const zend_property_info *info, zval *property_val, zval *value, zend_refcounted **garbage_ptr EXECUTE_DATA_DC)
{
ZEND_ASSERT(!(info->flags & ZEND_ACC_PPP_SET_MASK)
|| (info->flags & ZEND_ACC_PUBLIC_SET)
|| zend_asymmetric_property_has_set_access(info));

return zend_assign_to_typed_prop_ex(info, property_val, value, garbage_ptr, false EXECUTE_DATA_CC);
}

static zend_always_inline bool zend_value_instanceof_static(const zval *zv) {
if (Z_TYPE_P(zv) != IS_OBJECT) {
return 0;
Expand Down
43 changes: 36 additions & 7 deletions Zend/zend_object_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,15 @@ static zend_always_inline const zend_class_entry *get_fake_or_executed_scope(voi
}
}

static zend_always_inline uintptr_t zend_get_property_offset(zend_class_entry *ce, zend_string *member, int silent, void **cache_slot, const zend_property_info **info_ptr) /* {{{ */
/* Resolve a property for reading (write_access == false) or for
* writing/unsetting (write_access == true). Write-kind resolution verifies
* asymmetric set visibility at population time: when the running scope lacks
* set access, the result is still returned (the caller's state-dependent
* slow path decides between an error and the __set fallback), but the
* runtime cache is NOT populated. A populated write-site cache slot
* therefore guarantees set access, which lets the VM's cached direct-assign
* fast path skip the per-write asymmetric visibility check. */
static zend_always_inline uintptr_t zend_get_property_offset(zend_class_entry *ce, zend_string *member, int silent, void **cache_slot, const zend_property_info **info_ptr, bool write_access) /* {{{ */
{
zval *zv;
zend_property_info *property_info;
Expand Down Expand Up @@ -440,6 +448,27 @@ static zend_always_inline uintptr_t zend_get_property_offset(zend_class_entry *c
return ZEND_DYNAMIC_PROPERTY_OFFSET;
}

if (write_access
&& UNEXPECTED(flags & ZEND_ACC_PPP_SET_MASK)
&& !(flags & ZEND_ACC_PUBLIC_SET)) {
const zend_class_entry *scope = get_fake_or_executed_scope();

if (property_info->ce != scope
&& (!(flags & ZEND_ACC_PROTECTED_SET)
|| !is_protected_compatible_scope(property_info->prototype->ce, scope))) {
/* Set access denied for this scope: resolve as usual, but keep
* the cache slot empty so every such write stays on the slow
* path with its state-dependent handling. Invalidate the key
* too: downstream code (e.g. the hooked simple-write marking)
* assumes resolution populated the slot and would otherwise
* flag a stale polymorphic entry. */
if (cache_slot) {
CACHE_PTR_EX(cache_slot, NULL);
cache_slot = NULL;
}
}
}

if (property_info->hooks) {
*info_ptr = property_info;
if (cache_slot) {
Expand Down Expand Up @@ -468,7 +497,7 @@ static ZEND_COLD void zend_wrong_offset(zend_class_entry *ce, zend_string *membe
const zend_property_info *dummy;

/* Trigger the correct error */
zend_get_property_offset(ce, member, 0, NULL, &dummy);
zend_get_property_offset(ce, member, 0, NULL, &dummy, false);
}
/* }}} */

Expand Down Expand Up @@ -749,7 +778,7 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int
#endif

/* make zend_get_property_info silent if we have getter - we may want to use it */
property_offset = zend_get_property_offset(zobj->ce, name, (type == BP_VAR_IS) || (zobj->ce->__get != NULL), cache_slot, &prop_info);
property_offset = zend_get_property_offset(zobj->ce, name, (type == BP_VAR_IS) || (zobj->ce->__get != NULL), cache_slot, &prop_info, false);

if (EXPECTED(IS_VALID_PROPERTY_OFFSET(property_offset))) {
try_again:
Expand Down Expand Up @@ -1072,7 +1101,7 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva
uint32_t *guard = NULL;
ZEND_ASSERT(!Z_ISREF_P(value));

property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__set != NULL), cache_slot, &prop_info);
property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__set != NULL), cache_slot, &prop_info, true);

if (EXPECTED(IS_VALID_PROPERTY_OFFSET(property_offset))) {
try_again:
Expand Down Expand Up @@ -1434,7 +1463,7 @@ ZEND_API zval *zend_std_get_property_ptr_ptr(zend_object *zobj, zend_string *nam
fprintf(stderr, "Ptr object #%d property: %s\n", zobj->handle, ZSTR_VAL(name));
#endif

property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__get != NULL), cache_slot, &prop_info);
property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__get != NULL), cache_slot, &prop_info, true);

if (EXPECTED(IS_VALID_PROPERTY_OFFSET(property_offset))) {
try_again:
Expand Down Expand Up @@ -1562,7 +1591,7 @@ ZEND_API void zend_std_unset_property(zend_object *zobj, zend_string *name, void
const zend_property_info *prop_info = NULL;
uint32_t *guard = NULL;

property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__unset != NULL), cache_slot, &prop_info);
property_offset = zend_get_property_offset(zobj->ce, name, (zobj->ce->__unset != NULL), cache_slot, &prop_info, true);

if (EXPECTED(IS_VALID_PROPERTY_OFFSET(property_offset))) {
zval *slot = OBJ_PROP(zobj, property_offset);
Expand Down Expand Up @@ -2372,7 +2401,7 @@ ZEND_API int zend_std_has_property(zend_object *zobj, zend_string *name, int has
uintptr_t property_offset;
const zend_property_info *prop_info = NULL;

property_offset = zend_get_property_offset(zobj->ce, name, 1, cache_slot, &prop_info);
property_offset = zend_get_property_offset(zobj->ce, name, 1, cache_slot, &prop_info, false);

if (EXPECTED(IS_VALID_PROPERTY_OFFSET(property_offset))) {
try_again:
Expand Down
4 changes: 3 additions & 1 deletion Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2524,7 +2524,9 @@ ZEND_VM_C_LABEL(assign_obj_simple):
property_val = OBJ_PROP(zobj, prop_offset);
if (Z_TYPE_P(property_val) != IS_UNDEF) {
if (prop_info != NULL) {
value = zend_assign_to_typed_prop(prop_info, property_val, value, &garbage EXECUTE_DATA_CC);
/* Cache-hit path: set access was verified when the
* slot was populated. */
value = zend_assign_to_typed_prop_granted(prop_info, property_val, value, &garbage EXECUTE_DATA_CC);
ZEND_VM_C_GOTO(free_and_exit_assign_obj);
} else {
ZEND_VM_C_LABEL(fast_assign_obj):
Expand Down
Loading
Loading