On Wed, Jul 23, 2014 at 1:20 PM, Bob Weinand <bobw...@hotmail.com> wrote:
> Yes. Did you see my thoughts before?
>
> I'm just wondering if we can't somehow deeply copy the asts for opcache
>> between compile time and run time in pass_two() (If I'm not wrong
>> pass_two() has some hook for zend extensions?)
>>
>> Then we can fix the ast and don't have to take care of opcache at run
>> time (= when the (dynamic) asts will be evaluated). It'd maybe even be a
>> bit faster as it then doesn't have to copy so much at run-time.
>>
>
> ^ these?
>
Yes, I saw, but it not always possible. At least some pointers to AST are
kept in shared memory. So AST may not be duplicated at all.
>
> I'm also not very happy with it.
> I think I just should remove that restriction of run-time completely. It's
> just causing more confusion than it helps…
> I don't even remember why I even added that restriction there.
>
It was a restriction to not support arrays in constant context. It seems
like nobody can remember why it was introduced.
However, I think it's too dangerous to break it in last minute before
release.
Actually, you already broke it, and just prohibited usage at run-time.
Thanks. Dmitry.
>
> Bob
>
> Am 23.7.2014 um 10:47 schrieb Dmitry Stogov <dmi...@zend.com>:
>
> On Wed, Jul 23, 2014 at 12:16 PM, Bob Weinand <bobw...@hotmail.com> wrote:
>
>> Hey, thank you for looking into it :-)
>>
>> Am 23.7.2014 um 00:23 schrieb Dmitry Stogov <dmi...@zend.com>:
>> > hi Bob,
>> >
>> > I still think that current array usage in constant expressions is not
>> > consistent and dangerous. It "smells" to me, and I think it may bring
>> > troubles in the future even if the existing known bugs are fixed.
>> >
>> > I see few issues:
>> >
>> > 1) It is possible to declare array class constants however they can't be
>> > used. I can't remember why array in constants were prohibited before and
>> > what problems they brought. The following script works without any
>> warnings.
>> >
>> > <?php
>> > class Foo {
>> > const BAR = [1];
>> > }
>> > ?>
>>
>> Because it's actually valid. You don't use it in non-static scalar
>> context.
>>
>> > 2) In some cases array constants may be used, but not in the others.
>> >
>> > <?php
>> > class Foo {
>> > const BAR = [0];
>> > static $a = Foo::BAR; // constant array usage
>> > }
>> > var_dump(Foo::$a); // prints array
>> > var_dump(Foo::BAR); // emits fatal error
>> > ?>
>>
>> They can only be used in static scalar contexts.
>>
>> I wanted to introduce constants to be used and dereferenced also at
>> run-time, but that requires a RFC.
>> If anyone would allow me to introduce that still now (it'd be a
>> relatively simple patch), I'll happily do it.
>> The issue just was that I was a bit late to create a RFC (beta freeze
>> etc...)
>>
>> > 3) The fact that constants are allowed in compile time and even stored,
>> but
>> > can't be used confuses me as well as the error message "PHP Fatal error:
>> > Arrays are not allowed in constants at run-time".
>>
>> See above...
>>
>
> Yeah all the issues above (1-3) are actually a single inconsistency.
> You may find it logical, but I think differently.
>
>
> > 4) Zend/tests/constant_expressions_arrays.phpt crashes whit
>> > opcache.protect_memory=1 (that indicates petential SHM memory
>> corruption)
>> >
>> > This may be fixed with the following patch:
>> >
>> > diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h
>> > index 144930e..f1aab9a 100644
>> > --- a/Zend/zend_vm_execute.h
>> > +++ b/Zend/zend_vm_execute.h
>> > @@ -4323,6 +4323,16 @@ static int ZEND_FASTCALL
>> > ZEND_DECLARE_CONST_SPEC_CONST_CONST_HANDLER(ZEND_OPCOD
>> > c.value = *tmp_ptr;
>> > } else {
>> > INIT_PZVAL_COPY(&c.value, val);
>> > + if (Z_TYPE(c.value) == IS_ARRAY) {
>> > + HashTable *ht;
>> > +
>> > + ALLOC_HASHTABLE(ht);
>> > + zend_hash_init(ht,
>> > zend_hash_num_elements(Z_ARRVAL(c.value)), NULL, ZVAL_PTR_DTOR, 0);
>> > + zend_hash_copy(ht, Z_ARRVAL(c.value),
>> > (copy_ctor_func_t) zval_deep_copy, NULL, sizeof(zval *));
>> > + Z_ARRVAL(c.value) = ht;
>> > + } else {
>> > + zval_copy_ctor(&c.value);
>> > + }
>> > zval_copy_ctor(&c.value);
>> > }
>> > c.flags = CONST_CS; /* non persistent, case sensetive */
>>
>> I assume you wanted to patch zend_vm_def.h, not zend_vm_execute.h.
>>
>
> Yes. Of course.
>
>
>> If you can fix it, please apply the patch, I'm not so deep into opcache
>> to take responsibility for that one.
>>
>
> OK. This part of the patch must be safe. I'll apply it later.
>
>
>>
>> > 5) Circular constant references crash (found by Nikita)
>> >
>> > <?php
>> > class A {
>> > const FOO = [self::BAR];
>> > const BAR = [self::FOO];
>> > }
>> > var_dump(A::FOO); // crashes because of infinity recursion
>> > ?>
>>
>> That isn't a specific problems with arrays:
>>
>> <?php
>> class test {
>> const BAR = 0 + self::FOO;
>> const FOO = 0 + self::BAR;
>> }
>> var_dump(test::BAR);
>>
>> just segfaults too because of the exact same issue
>>
>
> Oh... This is really bad.
> It means we have a general AST evaluation problem.
> It must be fixed before 5.6 release.
> I'll try to make another attempt in the evening today or tomorrow.
>
>
> Thanks. Dmitry.
>
>
>>
>> > I didn't find any useful way to fix it. One of the ideas with following
>> > hack seemed to work, but it breaks another test
>> > (Zend/tests/constant_expressions_classes.phpt)
>> >
>> > diff --git a/Zend/zend_ast.c b/Zend/zend_ast.c
>> > index 12f9405..8798737 100644
>> > --- a/Zend/zend_ast.c
>> > +++ b/Zend/zend_ast.c
>> > @@ -251,10 +251,22 @@ ZEND_API void zend_ast_evaluate(zval *result,
>> > zend_ast *ast, zend_class_entry *s
>> > zval_dtor(&op2);
>> > break;
>> > case ZEND_CONST:
>> > - *result = *ast->u.val;
>> > - zval_copy_ctor(result);
>> > - if (IS_CONSTANT_TYPE(Z_TYPE_P(result))) {
>> > - zval_update_constant_ex(&result, 1,
>> scope
>> > TSRMLS_CC);
>> > + if (EG(in_execution) && EG(opline_ptr) &&
>> > *EG(opline_ptr) &&
>> > + ((*EG(opline_ptr))->opcode ==
>> ZEND_RECV_INIT ||
>> > + (*EG(opline_ptr))->opcode ==
>> > ZEND_DECLARE_CONST)) {
>> > + *result = *ast->u.val;
>> > + zval_copy_ctor(result);
>> > + if (IS_CONSTANT_TYPE(Z_TYPE_P(result)))
>> {
>> > +
>> zval_update_constant_ex(&result, 1,
>> > scope TSRMLS_CC);
>> > + }
>> > + } else {
>> > + if
>> (IS_CONSTANT_TYPE(Z_TYPE_P(ast->u.val)))
>> > {
>> > +
>> > zval_update_constant_ex(&ast->u.val, 1, scope TSRMLS_CC);
>> > + *result = *ast->u.val;
>> > + } else {
>> > + *result = *ast->u.val;
>> > + zval_copy_ctor(result);
>> > + }
>> > }
>> > break;
>> > case ZEND_BOOL_AND:
>> >
>> > I spent few hours trying to find a solution, but failed. May be my ideas
>> > could lead you to something...
>> >
>> > Otherwise, I would recommend to remove this feature from PHP-5.6.
>> >
>> > Thanks. Dmitry.
>>
>>
>> Bob
>>
>> > On Tue, Jul 22, 2014 at 10:00 AM, Dmitry Stogov <dmi...@zend.com>
>> wrote:
>> >
>> >> Hi Bob,
>> >>
>> >> Now I think it's not fixable by design :(
>> >>
>> >> I'll try to think about it later today.
>> >> Could you please collect all related issues.
>> >>
>> >> Thanks. Dmitry.
>> >>
>> >>
>> >> On Mon, Jul 21, 2014 at 8:36 PM, Bob Weinand <bobw...@hotmail.com>
>> wrote:
>> >>
>> >>> Am 2.7.2014 um 15:43 schrieb Dmitry Stogov <dmi...@zend.com>:
>> >>>
>> >>> I don't have good ideas out of the box and I probably won't be able to
>> >>> look into this before next week.
>> >>>
>> >>>
>> >>> Hey, I still have no real idea how to solve it without breaking
>> opcache.
>> >>>
>> >>> This one seems to be considered like a blocking bug for 5.6.
>> >>>
>> >>> Could you please try to fix this in a sane manner?
>> >>>
>> >>> Bob
>>
>
