Refactor FCI API #9723
Refactor FCI API #9723
Conversation
ext/standard/basic_functions.c
Outdated
| bool status; | ||
|
|
||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &entry.fci, &entry.fci_cache, ¶ms, ¶m_count) == FAILURE) { | ||
| if (zend_parse_parameters(ZEND_NUM_ARGS(), "f*", &entry.fci, &entry.fci_cache, &entry.fci.params, &entry.fci.param_count) == FAILURE) { |
There was a problem hiding this comment.
entry.fci.params will point to the VM call stack, and needs to be duplicated as it outlives the register_shutdown_function() call
There was a problem hiding this comment.
Ahhh that's indeed an issue. Okay here it makes sense to realloc. :)
There was a problem hiding this comment.
Would you have a test that fails? As currently it does not even with this change.
Girgias
force-pushed
the
fci-api-refactoring
branch
from
8788f17 to
9fc9151
Compare
|
Tried reproducing the ASAN failure but it doesn't happen on my machine (Fedora 36) |
|
@Girgias I assume you passed the |
I did, might try a complete recompile, but I wonder if there is some reason why Linux is less picky then FreeBSD Update: Finally managed to reproduce |
|
It also seems to be due to the usage of |
|
The ASAN build is run on Linux 🙂 |
Girgias
force-pushed
the
fci-api-refactoring
branch
from
9fc9151 to
a99bbbd
Compare
Girgias
force-pushed
the
fci-api-refactoring
branch
from
a99bbbd to
fa731af
Compare
github-actions
bot
removed
Extension: curl
Extension: mysqli
Extension: pgsql
Extension: spl
SAPI: phpdbg
labels
Instead handle the copying and safe reallocation of the param zvals ourself.
The last parameter which is named zval *args, does NOT set the FCI params field. It is expected to be a PHP array which gets looped over to set the arguments which is also a zval pointer... Since PHP 8.0, the named_params field is a more appropriate way of doing this. Also remove zend_fcall_info_args_save() and zend_fcall_info_args_restore() as they were only used in the implementation of zend_fcall_info_call()
Usage of these function heap allocates the zvals when they can already be used directly with an FCI
This function reallocates the param zvals on the heap, when in general they can be directly assigned to the FCI
TODO better description of what to do in each case
Girgias
force-pushed
the
fci-api-refactoring
branch
from
fa731af to
2903fa7
Compare
The current FCI API is highly confusing IMHO, the
zval *argsparameters is not as one could expect the parameter which will set theparamsfield within an FCI structure. Instead it is expected to be a PHP array, something it doesn't assert and causes confusions as to why usage of the API fails.Moreover, since PHP 8.0, if one has a HashTable for the positional arguments it is simpler to pass this HashTable to the
named_paramsfield which also supports positional arguments. Using this field instead of the current FCI API removes unnecessary heap allocations for zvals which are for the most part locally stack allocated.This PR should be reviewed commit by commit as the change from the FCI API to using the
named_paramsfield is split from the actual removal of the API.Note: The removal of
zend_fcall_info_argp()is still a WIP as I'm hitting a very strange bug on test file:Zend/tests/declare_007.phpt