Repository navigation
ext/standard/stream: Use FCC instead of zval for notification callback - #19024
Conversation
Also check that the callable exists while setting the option
| } | ||
| for (i = 0; i < 6; i++) { | ||
| zend_call_known_fcc(context->notifier->fcc, NULL, 6, zvs, NULL); | ||
| for (uint8_t i = 0; i < 6; i++) { |
There was a problem hiding this comment.
Fun fact: If not unrolled, using an (unsigned) int would be better in the sense that the generated assembly for both x86-64 and arm64 is shorter. For x86 it saves an instruction prefix byte to switch to 8 bit mode, and for arm64 it saves an instruction.
However, the loop is likely unrolled in which case it doesn't matter, but just something to take into account that using a shorter type is not always better.
There was a problem hiding this comment.
Actually, why do we even have a loop? Just call the zval_ptr_dtor(_str) on &zvs[2] ?
There was a problem hiding this comment.
I was thinking about that, but can't this be an issue if the notifier callback takes the arguments by-ref and changes them to strings?
There was a problem hiding this comment.
No, because the conversion happens on the call frame, not on the original zvals.
| if (context->notifier && Z_TYPE(context->notifier->ptr) != IS_UNDEF && context->notifier->func == user_space_stream_notifier) { | ||
| Z_TRY_ADDREF(context->notifier->ptr); | ||
| add_assoc_zval_ex(return_value, "notification", sizeof("notification")-1, &context->notifier->ptr); | ||
| if (context->notifier && context->notifier->fcc) { |
There was a problem hiding this comment.
How can you have a notifier without a valid fcc field?
There was a problem hiding this comment.
That I don't know as I'm not super familiar with the code, so I went the safe route of checking it exists.
There was a problem hiding this comment.
How can you have a notifier without a valid fcc field?
Extensions. In fact this change is an actual breaking change. The .ptr could be used to store store arbitrary metadata for the extension-specific .func. Now it can no longer store arbitrary metadata. It should be a void * instead (that is then cast to zend_fcall_info_cache * for user_space_stream_notifier).
There was a reason the if() previously checked for ->func == user_space_stream_notifier instead of asserting that this is the case. Otherwise we would not need func in the first place and could just hardcode it to user_space_stream_notifier.
…nctions Support for attaching arbitrary metadata for consumption by stream notifier functions got broken in php#19024 when the previous `zval ptr` got changed to `zend_fcall_info_cache *fcc`, making the data specific to the `user_space_stream_notifier`. Fix this by changing the field to `void *ptr`, avoiding the indirection through a `zval`, but preserving the ability to store arbitrary data. If necessary to support different types than `IS_PTR`, extensions can heap-allocate a `zval` to store within `->ptr` as a minimal change to keep compatibility with PHP 8.4 or lower.
…nctions (#19158) Support for attaching arbitrary metadata for consumption by stream notifier functions got broken in #19024 when the previous `zval ptr` got changed to `zend_fcall_info_cache *fcc`, making the data specific to the `user_space_stream_notifier`. Fix this by changing the field to `void *ptr`, avoiding the indirection through a `zval`, but preserving the ability to store arbitrary data. If necessary to support different types than `IS_PTR`, extensions can heap-allocate a `zval` to store within `->ptr` as a minimal change to keep compatibility with PHP 8.4 or lower.
3 participants
Also check that the callable exists while setting the option