Skip to content

Fix GH-16186: Ignore scope when compiling closure in non-tracing jit #16200

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

Closed
wants to merge 1 commit into from
Closed

Fix GH-16186: Ignore scope when compiling closure in non-tracing jit #16200

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
17 changes: 17 additions & 0 deletions ext/opcache/jit/zend_jit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,8 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
bool ce_is_instanceof;
bool on_this;

ZEND_ASSERT(!(op_array->fn_flags & ZEND_ACC_CLOSURE) || !(op_array->scope));

if (JIT_G(bisect_limit)) {
jit_bisect_pos++;
if (jit_bisect_pos >= JIT_G(bisect_limit)) {
Expand Down Expand Up @@ -2818,6 +2820,18 @@ static int zend_real_jit_func(zend_op_array *op_array, zend_script *script, cons
/* Build SSA */
memset(&ssa, 0, sizeof(zend_ssa));

if (op_array->fn_flags & ZEND_ACC_CLOSURE) {
if (JIT_G(trigger) == ZEND_JIT_ON_FIRST_EXEC) {
zend_jit_op_array_extension *jit_extension = (zend_jit_op_array_extension*)ZEND_FUNC_INFO(op_array);
op_array = (zend_op_array*) jit_extension->op_array;
} else if (JIT_G(trigger) == ZEND_JIT_ON_HOT_COUNTERS) {
zend_jit_op_array_hot_extension *jit_extension = (zend_jit_op_array_hot_extension*)ZEND_FUNC_INFO(op_array);
op_array = (zend_op_array*) jit_extension->op_array;
} else {
ZEND_ASSERT(!op_array->scope);
Comment on lines +2824 to +2831
Copy link
Member

Choose a reason for hiding this comment

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

What about ZEND_FUNC_JIT_ON_PROF_REQUEST?

Copy link
Member Author

@arnaud-lb arnaud-lb Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added it here initially, but I then I figured that it was not necessary because we never compile closures in ZEND_FUNC_JIT_ON_PROF_REQUEST mode (according to my understanding of zend_jit_check_funcs()). In theory we could compile closures in zend_jit_check_funcs() by also iterating over op_array->dynamic_func_defs, but in this case we would get the right op_array, so fetching it from jit_extension would not be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

OK.

}
}

if (zend_jit_op_array_analyze1(op_array, script, &ssa) != SUCCESS) {
goto jit_failure;
}
Expand Down Expand Up @@ -3035,6 +3049,7 @@ static int zend_jit_setup_hot_counters(zend_op_array *op_array)
}
memset(&jit_extension->func_info, 0, sizeof(zend_func_info));
jit_extension->func_info.flags = ZEND_FUNC_JIT_ON_HOT_COUNTERS;
jit_extension->op_array = op_array;
jit_extension->counter = &zend_jit_hot_counters[zend_jit_op_array_hash(op_array) & (ZEND_HOT_COUNTERS_COUNT - 1)];
for (i = 0; i < op_array->last; i++) {
jit_extension->orig_handlers[i] = op_array->opcodes[i].handler;
Expand Down Expand Up @@ -3079,6 +3094,7 @@ int zend_jit_op_array(zend_op_array *op_array, zend_script *script)
}
memset(&jit_extension->func_info, 0, sizeof(zend_func_info));
jit_extension->func_info.flags = ZEND_FUNC_JIT_ON_FIRST_EXEC;
jit_extension->op_array = op_array;
jit_extension->orig_handler = (void*)opline->handler;
ZEND_SET_FUNC_INFO(op_array, (void*)jit_extension);
opline->handler = (const void*)zend_jit_runtime_jit_handler;
Expand Down Expand Up @@ -3108,6 +3124,7 @@ int zend_jit_op_array(zend_op_array *op_array, zend_script *script)
}
memset(&jit_extension->func_info, 0, sizeof(zend_func_info));
jit_extension->func_info.flags = ZEND_FUNC_JIT_ON_PROF_REQUEST;
jit_extension->op_array = op_array;
jit_extension->orig_handler = (void*)opline->handler;
ZEND_SET_FUNC_INFO(op_array, (void*)jit_extension);
opline->handler = (const void*)zend_jit_profile_jit_handler;
Expand Down
2 changes: 2 additions & 0 deletions ext/opcache/jit/zend_jit_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ static zend_always_inline bool zend_jit_same_addr(zend_jit_addr addr1, zend_jit_

typedef struct _zend_jit_op_array_extension {
zend_func_info func_info;
const zend_op_array *op_array;
const void *orig_handler;
} zend_jit_op_array_extension;

Expand Down Expand Up @@ -160,6 +161,7 @@ void ZEND_FASTCALL zend_jit_hot_func(zend_execute_data *execute_data, const zend

typedef struct _zend_jit_op_array_hot_extension {
zend_func_info func_info;
const zend_op_array *op_array;
int16_t *counter;
const void *orig_handlers[1];
} zend_jit_op_array_hot_extension;
Expand Down
38 changes: 38 additions & 0 deletions ext/opcache/tests/gh16186_001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
--TEST--
GH-16186 001 (Non-tracing JIT uses Closure scope)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=64M
opcache.jit=0234
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

class A {
private $x;
public function getIncrementor() {
return function() { $this->x++; };
}
}
$a = new A();
$f = $a->getIncrementor();
$c = new stdClass();
$f();
$f2 = Closure::bind($f, $c);
$f2();
$f2();

var_dump($c);

?>
--EXPECTF--
Warning: Undefined property: stdClass::$x in %s on line %d
object(stdClass)#%d (1) {
["x"]=>
int(2)
}
38 changes: 38 additions & 0 deletions ext/opcache/tests/gh16186_002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
--TEST--
GH-16186 002 (Non-tracing JIT uses Closure scope)
--EXTENSIONS--
opcache
--INI--
opcache.enable=1
opcache.enable_cli=1
opcache.jit_buffer_size=64M
opcache.jit=0214
opcache.file_update_protection=0
opcache.revalidate_freq=0
opcache.protect_memory=1
--FILE--
<?php

class A {
private $x;
public function getIncrementor() {
return function() { $this->x++; };
}
}
$a = new A();
$f = $a->getIncrementor();
$c = new stdClass();
$f();
$f2 = Closure::bind($f, $c);
$f2();
$f2();

var_dump($c);

?>
--EXPECTF--
Warning: Undefined property: stdClass::$x in %s on line %d
object(stdClass)#%d (1) {
["x"]=>
int(2)
}