Skip to content

Resolve Inconsistency in zend_jit_return Between JIT and Non-JIT Engines #14841

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

Open
wants to merge 4 commits into
base: PHP-8.2
Choose a base branch
from
Open

Resolve Inconsistency in zend_jit_return Between JIT and Non-JIT Engines #14841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b15f5f9
Fix the ordering of zend_jit_return to match ZEND_RETURN_SPEC_OBSERVER
segrax Jul 6, 2024
3815a71
Extend zend_test to allow hooks to be executed while observing the en…
segrax Jul 6, 2024
ceb501a
Add a test to ensure the return variable prior to the end observer ex…
segrax Jul 6, 2024
bc27802
Non-JIT passes a pointer to a temp zval into zend_observer_fcall_end
segrax Jul 23, 2024
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
37 changes: 17 additions & 20 deletions ext/opcache/jit/zend_jit_arm64.dasc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10877,21 +10877,6 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
return_value_used = -1;
}

if (ZEND_OBSERVER_ENABLED) {
if (Z_MODE(op1_addr) == IS_REG) {
zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
return 0;
}
op1_addr = dst;
}
| LOAD_ZVAL_ADDR FCARG2x, op1_addr
| mov FCARG1x, FP
| SET_EX_OPLINE opline, REG0
| EXT_CALL zend_observer_fcall_end, REG0
}

// if (!EX(return_value))
if (Z_MODE(op1_addr) == IS_REG && Z_REG(op1_addr) == ZREG_REG1) {
if (return_value_used != 0) {
Expand Down Expand Up @@ -10951,16 +10936,14 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
}

if (return_value_used == 0) {
|9:
return 1;
}

if (opline->op1_type == IS_CONST) {
/* pass */
} else if (opline->op1_type == IS_CONST) {
zval *zv = RT_CONSTANT(opline, opline->op1);
| ZVAL_COPY_CONST ret_addr, MAY_BE_ANY, MAY_BE_ANY, zv, ZREG_REG0, ZREG_TMP1, ZREG_FPR0
if (Z_REFCOUNTED_P(zv)) {
| ADDREF_CONST zv, REG0, TMP1
}
op1_addr = ret_addr;
} else if (opline->op1_type == IS_TMP_VAR) {
| ZVAL_COPY_VALUE ret_addr, MAY_BE_ANY, op1_addr, op1_info, ZREG_REG0, ZREG_REG2, ZREG_TMP1, ZREG_TMP2, ZREG_FPR0
} else if (opline->op1_type == IS_CV) {
Expand Down Expand Up @@ -11021,6 +11004,20 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
}

|9:
if (ZEND_OBSERVER_ENABLED) {
if (Z_MODE(op1_addr) == IS_REG) {
zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
return 0;
}
op1_addr = dst;
}
| LOAD_ZVAL_ADDR FCARG2x, op1_addr
| mov FCARG1x, FP
| SET_EX_OPLINE opline, REG0
| EXT_CALL zend_observer_fcall_end, REG0
}
return 1;
}

Expand Down
37 changes: 17 additions & 20 deletions ext/opcache/jit/zend_jit_x86.dasc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11584,21 +11584,6 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
return_value_used = -1;
}

if (ZEND_OBSERVER_ENABLED) {
if (Z_MODE(op1_addr) == IS_REG) {
zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
return 0;
}
op1_addr = dst;
}
| LOAD_ZVAL_ADDR FCARG2a, op1_addr
| mov FCARG1a, FP
| SET_EX_OPLINE opline, r0
| EXT_CALL zend_observer_fcall_end, r0
}

// if (!EX(return_value))
if (Z_MODE(op1_addr) == IS_REG && Z_REG(op1_addr) == ZREG_R1) {
if (return_value_used != 0) {
Expand Down Expand Up @@ -11664,16 +11649,14 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
}

if (return_value_used == 0) {
|9:
return 1;
}

if (opline->op1_type == IS_CONST) {
/* pass */
} else if (opline->op1_type == IS_CONST) {
zval *zv = RT_CONSTANT(opline, opline->op1);
| ZVAL_COPY_CONST ret_addr, MAY_BE_ANY, MAY_BE_ANY, zv, ZREG_R0
if (Z_REFCOUNTED_P(zv)) {
| ADDREF_CONST zv, r0
}
op1_addr = ret_addr;
} else if (opline->op1_type == IS_TMP_VAR) {
| ZVAL_COPY_VALUE ret_addr, MAY_BE_ANY, op1_addr, op1_info, ZREG_R0, ZREG_R2
} else if (opline->op1_type == IS_CV) {
Expand Down Expand Up @@ -11733,6 +11716,20 @@ static int zend_jit_return(dasm_State **Dst, const zend_op *opline, const zend_o
}

|9:
if (ZEND_OBSERVER_ENABLED) {
if (Z_MODE(op1_addr) == IS_REG) {
zend_jit_addr dst = ZEND_ADDR_MEM_ZVAL(ZREG_FP, opline->op1.var);

if (!zend_jit_spill_store(Dst, op1_addr, dst, op1_info, 1)) {
return 0;
}
op1_addr = dst;
}
| LOAD_ZVAL_ADDR FCARG2a, op1_addr
| mov FCARG1a, FP
| SET_EX_OPLINE opline, r0
| EXT_CALL zend_observer_fcall_end, r0
}
return 1;
}

Expand Down
41 changes: 41 additions & 0 deletions ext/zend_test/observer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,46 @@ static void observer_end(zend_execute_data *execute_data, zval *retval)
if (EG(exception)) {
php_printf("%*s<!-- Exception: %s -->\n", 2 * ZT_G(observer_nesting_depth), "", ZSTR_VAL(EG(exception)->ce->name));
}

if(ZT_G(observer_observe_end_call_function_name) && strlen(ZT_G(observer_observe_end_call_function_name)) > 0) {
zend_fcall_info fci = empty_fcall_info;
zend_fcall_info_cache fcc = empty_fcall_info_cache;

zval function_name = {.u1.type_info = IS_UNDEF};

// Initialize the zval with the zend_string value
ZVAL_STRING(&function_name, ZT_G(observer_observe_end_call_function_name));

char* error = 0;
if (zend_fcall_info_init(&function_name, 0, &fci, &fcc, NULL, &error) != SUCCESS) {

php_printf("<!-- Hook (%s) not found: %s-->\n", Z_STRVAL(function_name), error);
efree(error);
} else {

zval ret = {.u1.type_info = IS_UNDEF};
fci.param_count = 0;
fci.params = NULL;
fci.named_params = NULL;
fci.retval = &ret;

if (zend_call_function(&fci, &fcc) == SUCCESS) {
if (!Z_ISUNDEF(ret) &&
(fcc.function_handler->op_array.fn_flags &
ZEND_ACC_HAS_RETURN_TYPE) &&
!(ZEND_TYPE_PURE_MASK(
fcc.function_handler->common.arg_info[-1].type) &
MAY_BE_VOID)) {
if (execute_data->return_value) {
zval_ptr_dtor(execute_data->return_value);
ZVAL_COPY(execute_data->return_value, &ret);
ZVAL_UNDEF(&ret);
}
}
}
}
zval_ptr_dtor(&function_name);
}
observer_show_opcode(execute_data);
ZT_G(observer_nesting_depth)--;
if (execute_data->func && execute_data->func->common.function_name) {
Expand Down Expand Up @@ -346,6 +386,7 @@ PHP_INI_BEGIN()
STD_PHP_INI_BOOLEAN("zend_test.observer.observe_functions", "0", PHP_INI_SYSTEM, OnUpdateBool, observer_observe_functions, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_BOOLEAN("zend_test.observer.observe_declaring", "0", PHP_INI_SYSTEM, OnUpdateBool, observer_observe_declaring, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_ENTRY("zend_test.observer.observe_function_names", "", PHP_INI_ALL, zend_test_observer_OnUpdateCommaList, observer_observe_function_names, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_ENTRY("zend_test.observer.observe_end_call_function_name", "", PHP_INI_ALL, OnUpdateString, observer_observe_end_call_function_name, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_BOOLEAN("zend_test.observer.show_return_type", "0", PHP_INI_SYSTEM, OnUpdateBool, observer_show_return_type, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_BOOLEAN("zend_test.observer.show_return_value", "0", PHP_INI_SYSTEM, OnUpdateBool, observer_show_return_value, zend_zend_test_globals, zend_test_globals)
STD_PHP_INI_BOOLEAN("zend_test.observer.show_init_backtrace", "0", PHP_INI_SYSTEM, OnUpdateBool, observer_show_init_backtrace, zend_zend_test_globals, zend_test_globals)
Expand Down
1 change: 1 addition & 0 deletions ext/zend_test/php_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ ZEND_BEGIN_MODULE_GLOBALS(zend_test)
int observer_observe_functions;
int observer_observe_declaring;
zend_array *observer_observe_function_names;
const char *observer_observe_end_call_function_name;
int observer_show_return_type;
int observer_show_return_value;
int observer_show_init_backtrace;
Expand Down
34 changes: 34 additions & 0 deletions ext/zend_test/tests/observer_retval_alter_01.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
--TEST--
Observer: Function return values are modifiable by observers
--EXTENSIONS--
zend_test
--INI--
zend_test.observer.enabled=1
zend_test.observer.show_output=1
zend_test.observer.show_return_value=1
zend_test.observer.observe_function_names=foo
zend_test.observer.observe_end_call_function_name=hook
--FILE--
<?php
function foo(string $pin): string {

return 'original return value';
}

function hook(): string {
return 'hook value';
}

$res = foo('some value');
var_dump($res);
echo 'Done' . PHP_EOL;
?>
--EXPECTF--
<!-- init '%s%eobserver_retval_alter_%d.php' -->
<!-- init foo() -->
<foo>
<!-- init hook() -->
</foo:'hook value'>
<!-- init var_dump() -->
string(10) "hook value"
Done
Loading