Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75968
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.220.182 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <BLU436-SMTP130D71239852595DCB6193AE2FE0@phx.gbl>
References: <CA+9eiLvLhAWCfRmJ05nm46KdDymFmA=g-mkc0uuJeXbVppXnPA@mail.gmail.com>
	<BLU436-SMTP143809EBE997F38F5F7EF18E2FE0@phx.gbl>
	<CA+9eiLsWkWxDFSmrqru8ue9A6fZasEqy_xfmjCanvx+mkLXfMA@mail.gmail.com>
	<BLU436-SMTP167808268E21EDECAB34E34E2FE0@phx.gbl>
	<CA+9eiLs+R3NDgkrdhso=yJdFWbyxnVA6DptxPZyQs2Vu8=GBTg@mail.gmail.com>
	<BLU436-SMTP130D71239852595DCB6193AE2FE0@phx.gbl>
Date: Wed, 23 Jul 2014 22:12:08 +0400
Message-ID: <CA+9eiLt+W3d168sdqrO1pcc1ZE4x4tT=UuRKx67wLUE6E1PP=Q@mail.gmail.com>
To: Bob Weinand <bobwei9@hotmail.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>, Stas Malyshev <smalyshev@sugarcrm.com>, 
	Julien Pauli <jpauli@php.net>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c1bc1649a4ba04fee04727
Subject: Re: [PHP-DEV] Weird constant expression syntax and bug
From: dmitry@zend.com (Dmitry Stogov)

--001a11c1bc1649a4ba04fee04727
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

PHP-5.6 is frozen for new features for a long time.
Adding new features after RC is not a good idea.
And we will need some kind of RFC and voting.

I help you technically, but you know my opinion about this feature..

anyway, lets fix bugs first, I got an idea that may work...

Thanks. Dmitry.


On Wed, Jul 23, 2014 at 1:45 PM, Bob Weinand <bobwei9@hotmail.com> wrote:

> Am 23.7.2014 um 11:34 schrieb Dmitry Stogov <dmitry@zend.com>:
>
> On Wed, Jul 23, 2014 at 1:20 PM, Bob Weinand <bobwei9@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 (=3D 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.
>
>
> Not sure, but I think that each AST actually has maximum one pointer to i=
t
> after compilation time.
> We actually don't need to refcount ASTs, so I think it's safe to just e.g=
.
> create a HashTable with which AST is linked to which zval pointer?
>
> 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=E2=80=A6
> 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.
>
>
> Actually, it's not yet *last minute*, AFAIK, we're still having another R=
C
> after this bug fix.
> It is actually safe to remove that restriction. (I've tested it already).
>
> Thanks. Dmitry.
>
>
> Bob
>
> Am 23.7.2014 um 10:47 schrieb Dmitry Stogov <dmitry@zend.com>:
>
> On Wed, Jul 23, 2014 at 12:16 PM, Bob Weinand <bobwei9@hotmail.com> wrote=
:
>
> Hey, thank you for looking into it :-)
>
> Am 23.7.2014 um 00:23 schrieb Dmitry Stogov <dmitry@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 =3D [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 =3D [0];
>   static $a =3D 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=3D1 (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 =3D *tmp_ptr;
>       } else {
>               INIT_PZVAL_COPY(&c.value, val);
> +               if (Z_TYPE(c.value) =3D=3D 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) =3D ht;
> +               } else {
> +                       zval_copy_ctor(&c.value);
> +               }
>               zval_copy_ctor(&c.value);
>       }
>       c.flags =3D 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 =3D [self::BAR];
>   const BAR =3D [self::FOO];
> }
> var_dump(A::FOO); // crashes because of infinity recursion
> ?>
>
>
> That isn't a specific problems with arrays:
>
> <?php
> class test {
>    const BAR =3D 0 + self::FOO;
>    const FOO =3D 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 =3D *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 =3D=3D
>
> ZEND_RECV_INIT ||
>
> +                            (*EG(opline_ptr))->opcode =3D=3D
> ZEND_DECLARE_CONST)) {
> +                               *result =3D *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 =3D *ast->u.val;
> +                               } else {
> +                                       *result =3D *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 <dmitry@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 <bobwei9@hotmail.com>
>
> wrote:
>
>
> Am 2.7.2014 um 15:43 schrieb Dmitry Stogov <dmitry@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
>
>
>
>
>
>
> Bob
>
>
>
>
>

--001a11c1bc1649a4ba04fee04727--