Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Translating optional zval ** with zend_parse_parameters() to PHP 7 »

Translating optional zval ** with zend_parse_parameters() to PHP 7

php.internals

Thomas Hruska

9 years ago
I'm working on updating an extension for PHP 7 compatibility. I have one function that uses an optional zval ** with zend_parse_parameters(). zval **zprevcount = NULL; int count; int argc = ZEND_NUM_ARGS(); if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z", &zprevcount) == FAILURE) return; ... if (argc > 0) { count = (int)PrevCount; zval_dtor(*zprevcount); ZVAL_LONG(*zprevcount, count); } What's the correct way to translate that into PHP 7?
-- Thomas Hruska CubicleSoft President I've got great, time saving software that you will find useful. http://cubiclesoft.com/

Kalle Sommer Nielsen

9 years ago
Hi Thomas 2016-11-25 4:13 GMT+01:00 Thomas Hruska <thruska@cubiclesoft.com>:
> I'm working on updating an extension for PHP 7 compatibility. I have one > function that uses an optional zval ** with zend_parse_parameters(). > > zval **zprevcount = NULL; > int count; > int argc = ZEND_NUM_ARGS(); > > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z", > &zprevcount) == FAILURE) return; > > ... > > if (argc > 0) > { > count = (int)PrevCount; > > zval_dtor(*zprevcount); > ZVAL_LONG(*zprevcount, count); > } > > What's the correct way to translate that into PHP 7?
Since you are working with integers, you can do: zend_long prevcount = 0; if(zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &prevcount) == FAILURE) { return; } if(prevcount) { /* ... use prevcount ... */ } Notice that all integers returned from the ZPP is a zend_long, you can see more information about the ZPP in php-src/README.PARAMETER_PARSING_API
-- regards, Kalle Sommer Nielsen kalle@php.net

Thomas Hruska

9 years ago
On 11/25/2016 12:27 AM, Kalle Sommer Nielsen wrote:
> Hi Thomas > > 2016-11-25 4:13 GMT+01:00 Thomas Hruska <thruska@cubiclesoft.com>: >> I'm working on updating an extension for PHP 7 compatibility. I have one >> function that uses an optional zval ** with zend_parse_parameters(). >> >> zval **zprevcount = NULL; >> int count; >> int argc = ZEND_NUM_ARGS(); >> >> if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|Z", >> &zprevcount) == FAILURE) return; >> >> ... >> >> if (argc > 0) >> { >> count = (int)PrevCount; >> >> zval_dtor(*zprevcount); >> ZVAL_LONG(*zprevcount, count); >> } >> >> What's the correct way to translate that into PHP 7? > > Since you are working with integers, you can do: > > zend_long prevcount = 0; > > if(zend_parse_parameters(ZEND_NUM_ARGS(), "|l", &prevcount) == FAILURE) > { > return; > } > > if(prevcount) > { > /* ... use prevcount ... */ > } > > Notice that all integers returned from the ZPP is a zend_long, you can > see more information about the ZPP in > php-src/README.PARAMETER_PARSING_API
Unless I missed something, zend_long is generally defined as a standard int64_t or int32_t type (Zend/zend_long.h). I need to be able to *modify* the original variable that was passed in. I already use "|l" and zend_long elsewhere in the extension. I'm thinking the correct ZPP string is "|z/" but the usage examples in core that I've looked at so far are inconsistent, hence the original question of what's the correct approach?
-- Thomas Hruska CubicleSoft President I've got great, time saving software that you will find useful. http://cubiclesoft.com/

Kalle Sommer Nielsen

9 years ago
2016-11-25 13:30 GMT+01:00 Thomas Hruska <thruska@cubiclesoft.com>:
> I need to be able to *modify* the original variable that was passed in. I already use "|l" and zend_long > elsewhere in the extension
Ah my bad I misread it! In that case take a look at where such is implemented in php-src, dns_get_record() is an example despite the slightly clouded code: http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h=f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011 See $weight_list: - Check if the argument is passed - zval_dtor() - Populate it (ZVAL_LONG is the same) The main difference from your code is to use 'z' (lower case) in ZPP, and then use zval* (which is what 'z' returns). I'm not at my dev machine, so I cannot test it but that should be how it goes
-- regards, Kalle Sommer Nielsen kalle@php.net

Jakub Zelenka

9 years ago
On 25 Nov 2016 20:43, "Kalle Sommer Nielsen" <kalle@php.net> wrote: 2016-11-25 13:30 GMT+01:00 Thomas Hruska <thruska@cubiclesoft.com>:
> I need to be able to *modify* the original variable that was passed in.
I already use "|l" and zend_long
> elsewhere in the extension
Ah my bad I misread it! In that case take a look at where such is implemented in php-src, dns_get_record() is an example despite the slightly clouded code: http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h= f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011 See $weight_list: - Check if the argument is passed - zval_dtor() - Populate it (ZVAL_LONG is the same) The main difference from your code is to use 'z' (lower case) in ZPP, and then use zval* (which is what 'z' returns). You also need set 1 for the first param of ZEND_ARG_INFO (specify that it is a ref param) otherwise it won't use the changed val... Jakub

Thomas Hruska

9 years ago
On 11/25/2016 4:32 PM, Jakub Zelenka wrote:
> On 25 Nov 2016 20:43, "Kalle Sommer Nielsen" <kalle@php.net> wrote: > > 2016-11-25 13:30 GMT+01:00 Thomas Hruska <thruska@cubiclesoft.com>: >> I need to be able to *modify* the original variable that was passed in. > I already use "|l" and zend_long >> elsewhere in the extension > > Ah my bad I misread it! > > In that case take a look at where such is implemented in php-src, > dns_get_record() is an example despite the slightly clouded code: > http://git.php.net/?p=php-src.git;a=blob;f=ext/standard/dns.c;h= > f92015eee90d3e93a801e93d6381d89923825166;hb=refs/heads/master#l1011 > > See $weight_list: > > - Check if the argument is passed > - zval_dtor() > - Populate it (ZVAL_LONG is the same) > > The main difference from your code is to use 'z' (lower case) in ZPP, > and then use zval* (which is what 'z' returns). > > > > You also need set 1 for the first param of ZEND_ARG_INFO (specify that it > is a ref param) otherwise it won't use the changed val... > > Jakub
Okay, everyone has been helpful. Thanks. I'll go with: zval *zprevcount = NULL; zend_long count; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z/", &zprevcount) == FAILURE) return; ... if (zprevcount != NULL) { count = (zend_long)PrevCount; zval_dtor(zprevcount); ZVAL_LONG(zprevcount, count); } Just one little thing I found in php_str_replace_common() in ext/standard/string.c. That particular function calls zval_ptr_dtor() instead of zval_dtor() on zcount. Just wondering why it does that - I'm thinking it has something to do with removing the ZPP call for PHP 7 and using the (mostly undocumented?) Z_PARAM_ZVAL_EX() macro. However, php_do_pcre_match() over in ext/pcre/php_pcre.c calls zval_dtor() on subpats but also uses the Z_PARAM_ZVAL_EX() macro in a similar fashion. Looking back at PHP 5.6's php_str_replace_common() shows that it previously called zval_dtor(). So it might also be a bug of some sort.
-- Thomas Hruska CubicleSoft President I've got great, time saving software that you will find useful. http://cubiclesoft.com/

Christoph Becker

9 years ago
On 26.11.2016 at 01:47, Thomas Hruska wrote:
> Okay, everyone has been helpful. Thanks. I'll go with: > > > zval *zprevcount = NULL; > zend_long count; > > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z/", > &zprevcount) == FAILURE) return; > > ... > > if (zprevcount != NULL) > { > count = (zend_long)PrevCount; > > zval_dtor(zprevcount); > ZVAL_LONG(zprevcount, count); > } > > > Just one little thing I found in php_str_replace_common() in > ext/standard/string.c. That particular function calls zval_ptr_dtor() > instead of zval_dtor() on zcount. Just wondering why it does that - I'm > thinking it has something to do with removing the ZPP call for PHP 7 and > using the (mostly undocumented?) Z_PARAM_ZVAL_EX() macro. However, > php_do_pcre_match() over in ext/pcre/php_pcre.c calls zval_dtor() on > subpats but also uses the Z_PARAM_ZVAL_EX() macro in a similar fashion. > Looking back at PHP 5.6's php_str_replace_common() shows that it > previously called zval_dtor(). So it might also be a bug of some sort.
zval_dtor() destroys the value; zval_ptr_dtor() decrements the refcount, and destroys the value only if the refcount has dropped to 0. See also <http://www.phpinternalsbook.com/zvals/memory_management.html> (which is written for PHP 5, but is still useful). In your case, zval_dtor() is appropriate, as the zval has already been separated (ZPP's /), so there can't be other references.
-- Christoph M. Becker

Nikita Popov

9 years ago
On Sat, Nov 26, 2016 at 12:29 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 26.11.2016 at 01:47, Thomas Hruska wrote: > > > Okay, everyone has been helpful. Thanks. I'll go with: > > > > > > zval *zprevcount = NULL; > > zend_long count; > > > > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z/", > > &zprevcount) == FAILURE) return; > > > > ... > > > > if (zprevcount != NULL) > > { > > count = (zend_long)PrevCount; > > > > zval_dtor(zprevcount); > > ZVAL_LONG(zprevcount, count); > > } > > > > > > Just one little thing I found in php_str_replace_common() in > > ext/standard/string.c. That particular function calls zval_ptr_dtor() > > instead of zval_dtor() on zcount. Just wondering why it does that - I'm > > thinking it has something to do with removing the ZPP call for PHP 7 and > > using the (mostly undocumented?) Z_PARAM_ZVAL_EX() macro. However, > > php_do_pcre_match() over in ext/pcre/php_pcre.c calls zval_dtor() on > > subpats but also uses the Z_PARAM_ZVAL_EX() macro in a similar fashion. > > Looking back at PHP 5.6's php_str_replace_common() shows that it > > previously called zval_dtor(). So it might also be a bug of some sort. > > zval_dtor() destroys the value; zval_ptr_dtor() decrements the refcount, > and destroys the value only if the refcount has dropped to 0. See also > <http://www.phpinternalsbook.com/zvals/memory_management.html> (which is > written for PHP 5, but is still useful). > > In your case, zval_dtor() is appropriate, as the zval has already been > separated (ZPP's /), so there can't be other references.
The situation here has become a bit confusing in PHP 7. zval_dtor() is now actually the same as zval_ptr_dtor_nogc(). As such, you can use zval_dtor() on zvals with rc>1, but collectible zvals will not be added to the GC root buffer. In the vast majority of cases, you will want to use zval_ptr_dtor(). zval_dtor() / zval_ptr_dtor_nogc() are useful either as an optimization in cases where you know for certain that a value is a root, or to prevent values from being placed in the GC root buffer, in cases where this violates the memory model (e.g. this is relevant for opcache'd literals). The fact that php_pcre.c uses zval_dtor() is simply a bug, because code like $obj = new stdClass; $obj->obj = $obj; preg_match('/./', 'x', $obj); leaks. Nikita

Christoph Becker

9 years ago
On 26.11.2016 at 12:45, Nikita Popov wrote:
> On Sat, Nov 26, 2016 at 12:29 PM, Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> On 26.11.2016 at 01:47, Thomas Hruska wrote: >> >>> Okay, everyone has been helpful. Thanks. I'll go with: >>> >>> >>> zval *zprevcount = NULL; >>> zend_long count; >>> >>> if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|z/", >>> &zprevcount) == FAILURE) return; >>> >>> ... >>> >>> if (zprevcount != NULL) >>> { >>> count = (zend_long)PrevCount; >>> >>> zval_dtor(zprevcount); >>> ZVAL_LONG(zprevcount, count); >>> } >>> >>> >>> Just one little thing I found in php_str_replace_common() in >>> ext/standard/string.c. That particular function calls zval_ptr_dtor() >>> instead of zval_dtor() on zcount. Just wondering why it does that - I'm >>> thinking it has something to do with removing the ZPP call for PHP 7 and >>> using the (mostly undocumented?) Z_PARAM_ZVAL_EX() macro. However, >>> php_do_pcre_match() over in ext/pcre/php_pcre.c calls zval_dtor() on >>> subpats but also uses the Z_PARAM_ZVAL_EX() macro in a similar fashion. >>> Looking back at PHP 5.6's php_str_replace_common() shows that it >>> previously called zval_dtor(). So it might also be a bug of some sort. >> >> zval_dtor() destroys the value; zval_ptr_dtor() decrements the refcount, >> and destroys the value only if the refcount has dropped to 0. See also >> <http://www.phpinternalsbook.com/zvals/memory_management.html> (which is >> written for PHP 5, but is still useful). >> >> In your case, zval_dtor() is appropriate, as the zval has already been >> separated (ZPP's /), so there can't be other references. > > The situation here has become a bit confusing in PHP 7. zval_dtor() is now > actually the same as zval_ptr_dtor_nogc(). As such, you can use zval_dtor() > on zvals with rc>1, but collectible zvals will not be added to the GC root > buffer. > > In the vast majority of cases, you will want to use zval_ptr_dtor(). > zval_dtor() / zval_ptr_dtor_nogc() are useful either as an optimization in > cases where you know for certain that a value is a root, or to prevent > values from being placed in the GC root buffer, in cases where this > violates the memory model (e.g. this is relevant for opcache'd literals).
Thanks for the explanation, Nikita.
> The fact that php_pcre.c uses zval_dtor() is simply a bug, because code like > > $obj = new stdClass; > $obj->obj = $obj; > preg_match('/./', 'x', $obj); > > leaks.
Indeed. At least preg_replace()'s $count parameter has the same issue. Is there already a bug report about this?
-- Christoph M. Becker

Christoph Becker

9 years ago
On 26.11.2016 at 13:55, Christoph M. Becker wrote:
> On 26.11.2016 at 12:45, Nikita Popov wrote: > >> The fact that php_pcre.c uses zval_dtor() is simply a bug, because code like >> >> $obj = new stdClass; >> $obj->obj = $obj; >> preg_match('/./', 'x', $obj); >> >> leaks. > > Indeed. At least preg_replace()'s $count parameter has the same issue. > Is there already a bug report about this?
FTR: reported as <https://bugs.php.net/bug.php?id=73612>.
-- Christoph M. Becker