[PFA 3/n] Support PFA in const exprs#22785
Conversation
arnaud-lb
changed the title
0ff8119 to
f5a4f79
Compare
arnaud-lb
changed the title
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
I reviewed the PR excluding the first two commits. The tests LGTM. For the implementation, I didn't see obvious issues (except formatting nits), but I'm not fully comfortable approving the C changes.
0a6d9cd to
f153d7a
Compare
|
This can / should be rebased now. |
f153d7a to
9391556
Compare
|
I was doing it :) |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Tests still LGTM. No longer seeing issues in the C implementation, but this is not my area of expertise.
… of the declaring opline itself zend_partial_create() takes a 'zend_op* declaring_opline' parameter for the purpose of building a cache key: the opline address is unique to this PFA as long as it's in SHM. However the declaring opline is not available when evaluating a PFA AST. Replace the parameter by 'uint32_t* declaring_lineno'. Callers can pass '&zend_op->lineno' or '&ast->lineno': Both are unique to the related PFA.
The declaring op_array is used for multiple purposes: * Generating a PFA name * Finding the filename * Finding whether op_array is cached, and therefore whether the PFA should be cached too However it's not available when evaluating a PFA AST. Remove the declaring_op_array argument, and pass the relevant information directly: * Generate the PFA name at compile time, pass it as argument to zend_partial_create() * Pass the filename and a cacheable flag as argument to zend_partial_create()
9391556 to
b0ddbb6
Compare
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
A few things from a read of the const-expr path (both correctness items are in paths no test covers, so CI stayed green).
|
|
||
| fail: | ||
| for (uint32_t i = 0, num_args = ZEND_CALL_NUM_ARGS(frame); i < num_args; i++) { | ||
| zval_ptr_dtor(ZEND_CALL_VAR_NUM(frame, i)); |
There was a problem hiding this comment.
This cleanup can double-free. A const-expr PFA with a failing named argument reaches here with the named slot uninitialized:
function target($a, $b) {}
function both(
$x = target(new stdClass, ?),
$y = target(a: MISSING_CONST + 1, b: ?),
) {}
both(); // zend_mm_heap corruptedzend_handle_named_arg bumps NUM_ARGS and hands back the target slot without initializing it (the UNDEF-fill loop stops before the target, and is skipped entirely when num_extra_args == 1). When the value evaluation fails without writing the slot (MISSING_CONST + 1 fails on its first operand), this loop zval_ptr_dtors the slot, which still aliases the new stdClass the first default left on the VM stack, so the object is freed while its PFA closure still owns it. The positional path only counts a slot after a successful evaluate; ZVAL_UNDEF-ing the named target before evaluating into it would line the two up.
|
|
||
| zval this_ptr; | ||
| ZVAL_UNDEF(&this_ptr); | ||
| Z_PTR(this_ptr) = NULL; |
There was a problem hiding this comment.
A static-method PFA in a const expr loses its scope here: this_ptr is forced to NULL and the frame is pushed with object_or_called_scope = NULL, even though the static method has common.scope != NULL.
class Foo { static function bar($x, $y) {} const C = self::bar(1, ?); }
Foo::C;Debug builds abort at zend_vm_init_call_frame (!func->common.scope || object_or_called_scope); release builds compile the forwarding call to a global bar() instead of Foo::bar, since called_scope comes back NULL. The runtime path is fine (INIT_STATIC_METHOD_CALL carries called_scope). No constexpr test exercises a static-method PFA.
| opcache.enable=1 | ||
| opcache.enable_cli=1 | ||
| opcache.optimization_level=-1 | ||
| opcache.preload={PWD}/constexpr_015.inc |
There was a problem hiding this comment.
This preload test needs a Windows skip. WINDOWS_X64_ZTS is red here with "Preloading is not supported on Windows". #22829 already adds the guard; this one needs it too, or land them together.
3 participants
See individual commits. First two are GH-22783 and GH-22784, which will be merged separately. (Edit: these were merged, and the branch has been rebased.)