Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:75942 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 8524 invoked from network); 23 Jul 2014 09:34:12 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 23 Jul 2014 09:34:12 -0000 Authentication-Results: pb1.pair.com smtp.mail=dmitry@zend.com; spf=pass; sender-id=pass Authentication-Results: pb1.pair.com header.from=dmitry@zend.com; sender-id=pass Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.220.173 as permitted sender) X-PHP-List-Original-Sender: dmitry@zend.com X-Host-Fingerprint: 209.85.220.173 mail-vc0-f173.google.com Received: from [209.85.220.173] ([209.85.220.173:37958] helo=mail-vc0-f173.google.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 9C/36-21666-3918FC35 for ; Wed, 23 Jul 2014 05:34:11 -0400 Received: by mail-vc0-f173.google.com with SMTP id hy10so1599922vcb.32 for ; Wed, 23 Jul 2014 02:34:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=EWUUVDaiIeZ6AF7b8R+L+qep+qb+A/HTyi5hFhCPtUk=; b=Rb3u/lju/VgfkE6ysj4vAKmccdvAWUpnHVOEAD2/728WpAGOT4WbMIa1KXB07r3aWJ wxrOp7uX+S+TAx3ApR8pHsya83xgANl+VtbXSEBuNJw+nnuspzEa/lbc4OIJ/y4jOm2b 0cSKARuf2ZqzFtVJ0MjNRlC26pNvl7X6NOeSmO2p46n7AmpItkOHIR4bsOKHl9YfWQZK bXAHaGAmIob0DVS7adCOMkGr+V9OgrJAk4SY9u3VLBwF+nNo/Gl9AGmMsuZFHoea8ESf WnYzK1CVKZjwiq5azvjTqp94LvjjUJQt3baiyKXnJyriSx+K2JwpCJTAdDm70nP9qnxZ t8Rg== X-Gm-Message-State: ALoCoQlrQrftn3KyO/VbxLhT2tVfiIJw8RQ73TZG/TVVodxVg9U6GgkSwVbJoi26ovMVjcR9l+rZrwkeywPUc3tdm2FXrAlNh7bVO65Te3u0JbkrLo4zB2vx9bSmivUKCZP9FYvdkner MIME-Version: 1.0 X-Received: by 10.220.94.135 with SMTP id z7mr60159vcm.46.1406108050119; Wed, 23 Jul 2014 02:34:10 -0700 (PDT) Received: by 10.52.110.170 with HTTP; Wed, 23 Jul 2014 02:34:10 -0700 (PDT) In-Reply-To: References: Date: Wed, 23 Jul 2014 13:34:10 +0400 Message-ID: To: Bob Weinand Cc: Nikita Popov , Stas Malyshev , Julien Pauli , PHP Internals Content-Type: multipart/alternative; boundary=001a11c1e488d827fd04fed90abb Subject: Re: [PHP-DEV] Weird constant expression syntax and bug From: dmitry@zend.com (Dmitry Stogov) --001a11c1e488d827fd04fed90abb Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Wed, Jul 23, 2014 at 1:20 PM, Bob Weinand 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. > > 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. Thanks. Dmitry. > > Bob > > Am 23.7.2014 um 10:47 schrieb Dmitry Stogov : > > On Wed, Jul 23, 2014 at 12:16 PM, Bob Weinand wrote= : > >> Hey, thank you for looking into it :-) >> >> Am 23.7.2014 um 00:23 schrieb Dmitry Stogov : >> > 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 a= nd >> > what problems they brought. The following script works without any >> warnings. >> > >> > > > 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. >> > >> > > > 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 erro= r: >> > 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) >> > >> > > > 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: >> >> > 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 followin= g >> > 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 ide= as >> > 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 >> 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 >> wrote: >> >> >> >>> Am 2.7.2014 um 15:43 schrieb Dmitry Stogov : >> >>> >> >>> 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 >> > --001a11c1e488d827fd04fed90abb--