[PATCH] internal class' static properties and constants

I wrote:

Though there's already a diff against HEAD, but it's
now about 7 hours old, so it may be inapplicable.

http://dev.iworks.at/PATCHES/

Actually it seems that there were no more commits,
so the available patch should work fine.

Regards,
Mike

I wrote:

Hi Marcus Boerger, you wrote:

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.

Now that I know what zval_update_constant() is good for...

Is it even needed to call zend_update_class_constants() for
ZEND_INTERNAL_CLASSes? One most probably won't declare a
property of an internal class with a zval that's IS_CONSTANT.

Thoughts?

--
Michael - < mike(@)php.net

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