Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:18200
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Date: Wed, 17 Aug 2005 21:58:54 +0200
Reply-To: Marcus Boerger <helly@php.net>
Message-ID: <101570804.20050817215854@marcus-boerger.de>
To: Michael Wallner <mike@php.net>
Cc: internals@lists.php.net
In-Reply-To: <65.2E.33075.C74E2034@pb1.pair.com>
References: <61.69.24081.E876BF24@pb1.pair.com>
	<BF.79.33075.DBAACF24@pb1.pair.com>
	<1997493019.20050816231952@marcus-boerger.de>
	<65.2E.33075.C74E2034@pb1.pair.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: [PATCH] internal class' static properties and constants
From: helly@php.net (Marcus Boerger)

Hello Michael,

Wednesday, August 17, 2005, 9:14:50 AM, you wrote:

> Hi Marcus Boerger, you wrote:
>> Hello Michael,
>>
>> a few questions/comments so that i can understand better:
>>
>> 1) Zend/zend_API.c: <NEW>
>> +ZEND_API int zend_declare_class_constant_stringl(zend_class_entry
>> *ce, char *name, size_t name_length, char *value, size_t value_length
>> TSRMLS_DC)
>> +{
>> +       zval *constant = new_zval(ce->type & ZEND_INTERNAL_CLASS);
>> +       if (ce->type & ZEND_INTERNAL_CLASS) {
>> +               ZVAL_STRINGL(constant, zend_strndup(value, value_length), value_length, 0);
>> +       } else {
>> +               ZVAL_STRINGL(constant, value, value_length, 1);
>> +       }
>> +       return zend_declare_class_constant(ce, name, name_length, constant TSRMLS_CC);
>> +}
>> +
>>
>> If the internal part uses string duplication so shall use the user space
>> code.

> Yes, the internal part uses zend_strndup() while the userspace part
> - I guess - estrndup().

Oh, now i see it there is a 1 for copy which means estrndup().


>> 2) Zend/zend_API.c: <NEW>
>> +ZEND_API int zend_update_static_property(zend_class_entry *scope,
>> char *name, size_t name_len, zval *value TSRMLS_DC)
>> +{
>> +       int retval;
>> +       zval **property = NULL;
>> +       zend_class_entry *old_scope = EG(scope);
>> +
>> +       EG(scope) = scope;
>> +
>> +       if (!(property = zend_std_get_static_property(scope, name, name_len, 0 TSRMLS_CC))) {
>> +               retval = FAILURE;
>> in the caose you utilize this value is a temp thing thus you need to free it
>> in all cases but those you directly use it. A failure is definitively a
>> thing where you do't use it so it must be fr here.
>> +       } else if (*property == value) {
>> +               retval = SUCCESS;
>> Here you compare the address of a zval, was that intended?
>> In case you meant it, then i think it cannot be correct because that should
>> never happen, or is that the case you actually want to prevent?
>> In case you meant to prohibit from setting the same value again you need
>> actual zval comparison which won't work easily for strings.

> This is from zend_reflection_api ReflectionProperty::setValue().
> I assume it is meant to prevent useless operations,
> because when else should the zvals point to the same address?

Yep in reflection api it is used for the exact same reason. I was only
wondering whether that can happen here. Since your code is called outside
request time it can only happen if some code tries to update the same prop
with the same value twice.

>> +       } else {
>> +               if (PZVAL_IS_REF(*property)) {
>> +                       zval_dtor(*property);
>> +                       (*property)->type = value->type;
>> +                       (*property)->value = value->value;
>> +
>> +                       if (value->refcount) {
>> +                               zval_copy_ctor(*property);
>> +                       }
>> +               } else {
>> +                       **property = *value;
>> +                       zval_copy_ctor(*property);
>> +               }
>> +               retval = SUCCESS;
>> +       }
>> +
>> +       if (!value->refcount) {
>> +               zval_dtor(value);
>> +               FREE_ZVAL(value);
>> +       }
>> This only works because your zvals are initialized with refcount=0 which
>> cannot be right. In the end it should always read here
>> zval_ptr_dtor(&value); And tmp_zval() should initialize with refcount=1.

> I actually thought that there should be an "else { zval_ptr_dtor(&value) }"?
> Every place I looked at where "temporary zvals" are used, initialize them
> with refcount=0.

Temp zvals are used in scripts and can never be stored at least that was the
goal. In your case always and unfortunatley also in the engine usage for
edge cases they are going to be stored. So probably it is easier here to go
with normal props initilized with refcount=1. You have to try and see.

>> +
>> +       EG(scope) = old_scope;
>> +
>> +       return retval;
>> +}
>>
>> Your static properties get duplicated and const updated from
>>
>> 3) Zend/zend.c: static void class_to_unicode(zend_class_entry **ce)
>> +       zend_u_hash_init_ex(&new_ce->default_static_properties,
>> (*ce)->default_static_properties.nNumOfElements, NULL,
>> (*ce)->default_static_properties.pDestructor, 1, 1, 0);
>> +       zend_hash_copy(&new_ce->default_static_properties,
>> &(*ce)->default_static_properties, (copy_ctor_func_t)
>> zval_ptr_to_unicode, &tmp_zval, sizeof(zval));
>> +       zend_u_hash_init_ex(&new_ce->static_properties,
>> (*ce)->static_properties.nNumOfElements, NULL,
>> (*ce)->default_static_properties.pDestructor, 1, 1, 0);
>> Why is the above not using
>> "(*ce)->default_static_properties.nNumOfElements", since that is the number
>> of entries the static property table will have exactly after the copy
>> operation on the next line so why not initialize the number to this?

> Yeah, this is a matter of optimization which I was not thinking about
> in the first place, sorry.

>> +       zend_hash_copy(&new_ce->static_properties,
>> &(*ce)->static_properties, (copy_ctor_func_t) zval_ptr_to_unicode,
>> &tmp_zval, sizeof(zval));
>>
>> 4) Zend/zend_API.c: ZEND_API void
>> zend_update_class_constants(zend_class_entry *class_type TSRMLS_DC)
>>                
>> zend_hash_apply_with_argument(&class_type->default_properties,
>> (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
>> -              
>> zend_hash_apply_with_argument(class_type->static_members,
>> (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
>> +              
>> zend_hash_apply_with_argument(&class_type->default_static_properties,
>> (apply_func_arg_t) zval_update_constant, (void *) 1 TSRMLS_CC);
>> Before your patch we have the situation that consts can only be decalred in
>> user space. Thus it is ensured that their data is emalloc'd and only ever
>> needs to be const-updated once. Now that you have this as well as your
>> default static properties it (zend_update_class_constants()) must be called
>> once for every request time. Having said that i think we actuall need two
>> tables. cor consts as well. I think default_consts and
>> default_static_properies never need to be const updated because otherwise
>> you wouldn't react on define changes if a defined const is being referred
>> to. With this in mind we also face another problem, the problem that
>> const-update assumes that the memory was emalloc'd.So what we need to do is
>> ensuring that on the first use in a request the two tables default_const and
>> default_static_properties get copied to their working tables consts and
>> static_properties. After that copy we can update the consts. Now we also
>> ensured that for internal classes the values in the defualt_const table and
>> default_static_properties table can become malloc'd.

> Well, I didn't really get the idea of this const updating yet, so apologize
> if I'm totally wrong here.  Ok, so it is a problem that the const updating
> routine (whatever it does) assumes that memory was emalloc()'d but why do
> constants_table and default_static_properties need to be updated for
> every request? They're not supposed to change their values after
> declaration, are they?

php -r '$d=42; define("a",$d); class t { const x=a;} var_dump(t::x);'

That's why.

>> 5) Zend/zend_compile.c: ZEND_API void
>> zend_initialize_class_data(zend_class_entry *ce, zend_bool
>> nullify_handlers TSRMLS_DC)
>> +       zend_u_hash_init_ex(&ce->default_static_properties, 0, NULL,
>> zval_ptr_dtor_func, persistent_hashes, UG(unicode), 0);
>> +       zend_u_hash_init_ex(&ce->static_properties, 0, NULL,
>> ZVAL_PTR_DTOR, persistent_hashes, UG(unicode), 0);
>> After reading 4) it should be clear that the values in
>> default_static_properties must be malloc'c zval's. With this in mind we of
>> course need to specify a corresponding free method insetad of the currently
>> used one that assumes the memory was emalloc'd.

> Hm...? default_static_properties' destructor is zval_internal_ptr_dtor, so?

Ah, yes, you're right here too, i didn't see the full context and got
wrong with all those function names somehow.


Sorry for getting this mail out that late....i thought it was sent in the
morning.

Best regards,
 Marcus