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().
> 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?
> + } 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.
> +
> + 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?
> 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?
> 6) Renamic static_members to static_properties is a very good idea if you
> ask me but it requires that we have this patch ready and set before 5.1 is
> out.
Well, I'm the one who most strongly hopes that, as my HttpResponse class
is just a bad hack until so-called v6. As you probably know, I'm a first
time extension writer and still learning a lot and my knowledge of the
internals is still quite low.
Thanks,
--
Michael - < mike(@)php.net >
signature.asc
Description: OpenPGP digital signature
