Fix static variable behavior with inheritance #6705
Conversation
|
Everything except of closure related changes looks clear. |
To make it clearer that these arrays cannot be shared and cannot be immutable.
We should only ever set this to a non-null value if there are static_variables. Make sure this is the case in lazy class loading as well.
Zend/zend_closures.c
Outdated
| @@ -38,6 +38,7 @@ typedef struct _zend_closure { | |||
| zend_function func; | |||
| zval this_ptr; | |||
| zend_class_entry *called_scope; | |||
| HashTable *static_variables; | |||
There was a problem hiding this comment.
I don't understand way we need this new "static_variables" and can't reuse "func.static_variables" and "static_variables_ptr".
There was a problem hiding this comment.
Yes, the changes in this file turned out to be unnecessary. I've reverted them.
| @@ -7021,9 +7025,8 @@ void zend_compile_func_decl(znode *result, zend_ast *ast, bool toplevel) /* {{{ | |||
| if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) { | |||
| op_array->fn_flags |= ZEND_ACC_PRELOADED; | |||
| ZEND_MAP_PTR_NEW(op_array->run_time_cache); | |||
There was a problem hiding this comment.
We probably, won't need special map_ptr handling for preloading now. I'll check this, after you merge the PR.
Zend/zend_inheritance.c
Outdated
|
|
||
| op_array = Z_PTR(p->val); | ||
| ZEND_ASSERT(op_array->type == ZEND_USER_FUNCTION); | ||
| ZEND_ASSERT(op_array->scope == pce); | ||
| ZEND_ASSERT(op_array->prototype == NULL); | ||
| new_op_array = zend_arena_alloc(&CG(arena), sizeof(zend_op_array) + sizeof(void*)); | ||
| new_op_array = zend_arena_alloc(&CG(arena), sizeof(zend_op_array) + 2 * sizeof(void*)); |
There was a problem hiding this comment.
We don't need to allocate additional word if op_array->static_variables == NULL.
ext/opcache/ZendAccelerator.c
Outdated
| !(GC_FLAGS(op_array->static_variables) & IS_ARRAY_IMMUTABLE)) { | ||
| GC_ADDREF(op_array->static_variables); | ||
| if (op_array->static_variables) { | ||
| GC_TRY_ADDREF(op_array->static_variables); |
There was a problem hiding this comment.
Why do we need this increment? I guess, we did this to trigger CoW and keep original values.
There was a problem hiding this comment.
Yes, this addref is not needed anymore. I've dropped it.
The initial static_variables values are also never shared (only the whole op_array containing them is shared).
When a method is inherited, the static variables will now always use the initial values, rather than the values at the time of inheritance. As such, behavior no longer depends on whether inheritance happens before or after a method has been called.
This is implemented by no always keeping static_variables as the original values, and static_variables_ptr as the modified copy.