# Weird constant expression syntax and bug - Thread: https://externals.io/message/75901 - Original archive: https://news-web.php.net/php.internals/75901 - Started by Dmitry Stogov on 2014-07-22T22:23:58+00:00 - Messages: 22 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #75901 — Dmitry Stogov — 2014-07-22T22:23:58+00:00 URL: https://externals.io/message/75901#75901 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. 2) In some cases array constants may be used, but not in the others. 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". 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 */ 5) Circular constant references crash (found by Nikita) 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. > 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. > > > > > > […] --- ## #75924 — Bob Weinand — 2014-07-23T08:16:45+00:00 URL: https://externals.io/message/75901#75924 · In reply to: #75901 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 and > what problems they brought. The following script works without any warnings. > > 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. > > 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... > 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. If you can fix it, please apply the patch, I'm not so deep into opcache to take responsibility for that one. > 5) Circular constant references crash (found by Nikita) > > 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: 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. So, as we see, it's not especially because of arrays, but everytime the two self-referencing constants are inside an expression of each other, it'll fail. 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. Bob > > > > […] --- ## #75927 — Stas Malyshev — 2014-07-23T08:29:22+00:00 URL: https://externals.io/message/75901#75927 · In reply to: #75924 Hi! > > […] > > 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...) This example really makes little sense as described. So you can define Foo::BAR, but you can not actually use it? This doesn't sound good and would be a major WTF source. Why it requires an RFC to fix it? This is not the case for any constant in PHP now, as far as I know, so it should not be the case in 5.6 either. > > […] I agree, this is an extremely weird behavior - a user would assume if they successfully defined the constant they could use it. If there's a technical problem with having arrays there, it's better to not allow defining them then define them and set the trap for the user trying to use them. Notice that the constant could be defined in the library, and can change - so you have code like var_dump(Foo::BAR); that can suddenly stop working with fatal error because library defining Foo::BAR changed. This is not good. > > […] I think this is an instance of the same thing that makes one of the tests segfault. It is not a good thing to have release with such segfault, we need to find some fix to it ASAP. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #75929 — Dmitry Stogov — 2014-07-23T08:47:36+00:00 URL: https://externals.io/message/75901#75929 · In reply to: #75924 > Hey, thank you for looking into it :-) > > Am 23.7.2014 um 00:23 schrieb Dmitry Stogov : > > […] > warnings. > > […] > > Because it's actually valid. You don't use it in non-static scalar context. > > > […] > > 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...) > > > […] > but > > […] > > See above... > Yeah all the issues above (1-3) are actually a single inconsistency. You may find it logical, but I think differently. > > > […] > > 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. > > > […] > > That isn't a specific problems with arrays: > > 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. > > > […] > || > > […] > 1, > > […] > (IS_CONSTANT_TYPE(Z_TYPE_P(ast->u.val))) > > […] > > So, as we see, it's not especially because of arrays, but everytime the > two self-referencing constants are inside an expression of each other, > it'll fail. > > 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. > > Bob > > > […] > wrote: > > […] > opcache. > > […] --- ## #75939 — Bob Weinand — 2014-07-23T09:20:42+00:00 URL: https://externals.io/message/75901#75939 · In reply to: #75929 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? 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. Bob Am 23.7.2014 um 10:47 schrieb Dmitry Stogov : > > Hey, thank you for looking into it :-) > > Am 23.7.2014 um 00:23 schrieb Dmitry Stogov : > > […] > > Because it's actually valid. You don't use it in non-static scalar context. > > > […] > > 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...) > > > […] > > See above... > > Yeah all the issues above (1-3) are actually a single inconsistency. > You may find it logical, but I think differently. > > > […] > > 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. > > > > […] > > That isn't a specific problems with arrays: > > 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. > > > > […] > > > Bob > > > […] --- ## #75942 — Dmitry Stogov — 2014-07-23T09:34:10+00:00 URL: https://externals.io/message/75901#75942 · In reply to: #75939 > Yes. Did you see my thoughts before? > > I'm just wondering if we can't somehow deeply copy the asts for opcache > > […] > > ^ 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 : > > > > > […] > > Yeah all the issues above (1-3) are actually a single inconsistency. > You may find it logical, but I think differently. > > > > […] > > Yes. Of course. > > > > […] > > OK. This part of the patch must be safe. I'll apply it later. > > > > […] > > 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. > > > > […] --- ## #75944 — Bob Weinand — 2014-07-23T09:45:37+00:00 URL: https://externals.io/message/75901#75944 · In reply to: #75942 Am 23.7.2014 um 11:34 schrieb Dmitry Stogov : > > > > […] > > 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 it 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? > > […] > > 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 RC after this bug fix. It is actually safe to remove that restriction. (I've tested it already). > Thanks. Dmitry. > > > […] Bob --- ## #75968 — Dmitry Stogov — 2014-07-23T18:12:08+00:00 URL: https://externals.io/message/75901#75968 · In reply to: #75944 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. > Am 23.7.2014 um 11:34 schrieb Dmitry Stogov : > > > > 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. > > > Not sure, but I think that each AST actually has maximum one pointer to it > 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… > 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 RC > 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 : > > > > 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 and > what problems they brought. The following script works without any > > warnings. > > > 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. > > 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) > > 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: > > 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 > > 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 > > > > > > > Bob --- ## #75975 — Stas Malyshev — 2014-07-23T20:33:29+00:00 URL: https://externals.io/message/75901#75975 · In reply to: #75942 Hi! > It was a restriction to not support arrays in constant context. It seems > like nobody can remember why it was introduced. My vague recollection is that it had some troubles with keeping refcounts consistent, esp. withing bytecode caching context, but it may be a false memory :) > However, I think it's too dangerous to break it in last minute before > release. We definitely need to fix the WTF with "no runtime use" for defined constant and the segfault before the release. I think for the arrays, if we can't have it working properly we'd better not have array support there for 5.6.0 and fix it in 5.6.1 than have this weird "no runtime use" thing. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #75976 — Bob Weinand — 2014-07-23T20:45:16+00:00 URL: https://externals.io/message/75901#75976 · In reply to: #75975 Am 23.7.2014 um 22:33 schrieb Stas Malyshev : > Hi! > > > […] > > My vague recollection is that it had some troubles with keeping > refcounts consistent, esp. withing bytecode caching context, but it may > be a false memory :) That doesn't really make much sense to me, at least not with current engine... As said, I've tested it and didn't find issues. Maybe I just test the wrong things… > > […] > > We definitely need to fix the WTF with "no runtime use" for defined > constant and the segfault before the release. I think for the arrays, if > we can't have it working properly we'd better not have array support > there for 5.6.0 and fix it in 5.6.1 than have this weird "no runtime > use" thing. Well, we still could fix it now, AFAIK, we will still have another RC now and when we fix it only for 5.6.1, there also will only be one RC in between. That's no gain in testing time etc.. Bob > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ --- ## #75988 — Pierre Joye — 2014-07-24T03:20:31+00:00 URL: https://externals.io/message/75901#75988 · In reply to: #75976 hi, > Well, we still could fix it now, AFAIK, we will still have another RC now and when we fix it only for 5.6.1, there also will only be one RC in between. > That's no gain in testing time etc.. Also please keep in mind that we are already very late with 5.6. I could live without this addition, not like it is critical enough to be a blocker for 5.6. I'd rather wait until we know exactly what is going wrong and add it to 5.7. Cheers, -- Pierre @pierrejoye | http://www.libgd.org --- ## #75990 — Kris Craig — 2014-07-24T04:15:40+00:00 URL: https://externals.io/message/75901#75990 · In reply to: #75988 > hi, > > > > > […] > now and when we fix it only for 5.6.1, there also will only be one RC in > between. > > […] > > Also please keep in mind that we are already very late with 5.6. I > could live without this addition, not like it is critical enough to be > a blocker for 5.6. I'd rather wait until we know exactly what is going > wrong and add it to 5.7. > > Cheers, > -- > Pierre > > @pierrejoye | http://www.libgd.org > > -- > > > > I tend to agree. If it's not critical and it's not ready in time for a given scheduled release (which it wasn't), then it should be held for the next release. We should get 5.6 out as soon as reasonably possible. --Kris --- ## #76123 — Ferenc Kovacs — 2014-07-25T14:36:35+00:00 URL: https://externals.io/message/75901#76123 · In reply to: #75976 > Am 23.7.2014 um 22:33 schrieb Stas Malyshev : > > […] > > That doesn't really make much sense to me, at least not with current > engine... > As said, I've tested it and didn't find issues. Maybe I just test the > wrong things… > > > […] > > Well, we still could fix it now, AFAIK, we will still have another RC now > and when we fix it only for 5.6.1, there also will only be one RC in > between. > That's no gain in testing time etc.. > > Bob > > > […] > > -- > > > > yeah, there will be another RC for sure, but we have to have some resulotion for that. I think it would be safer to remove stuff (even if just the array support) than trying to fix in in a hurry, as it would probably just cause shipping the final with some other bug(s) or having a couple of other RCs. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #76136 — Ferenc Kovacs — 2014-07-25T19:22:38+00:00 URL: https://externals.io/message/75901#76136 · In reply to: #76123 > > > > > > > […] > yeah, there will be another RC for sure, but we have to have some > resulotion for that. > I think it would be safer to remove stuff (even if just the array support) > than trying to fix in in a hurry, as it would probably just cause shipping > the final with some other bug(s) or having a couple of other RCs. > nevermind, I've somehow missed https://github.com/php/php-src/commit/c49a06168ed3759779452e2b2a44da22e75a0702 and https://github.com/php/php-src/commit/d909b6330e314bb434350317fda5bec185ea12c0 -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #76145 — Julien Pauli — 2014-07-25T21:03:13+00:00 URL: https://externals.io/message/75901#76145 · In reply to: #75968 > 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 agree here. Bob, if you've been late proposing an RFC and couldn't get it up in time, I'm sorry about it. Please, understand we are very close to a release, and ATM, we're still talking about weither this *user land* feature or that *user land* feature will be available, makes sense, or not. (that's a technical feature, but it involves user land here). That can't be ! We absolutely need something we, maintainers - creators - authors , think is *stable*. The PHP project needs that, we all agree. If the static array feature can't be added to the AST safely, please, remove it. You've already added so much new great code (AST for this example) to 5.6, I personnally thank you about this, however now it is really time to make it stable, and commit its new features you may think about into 5.7 (or what will be 5.7, ATM : master) , and write new RFCs about those if needed. Knowing your actual work in addition of that, is deep into the engine and has inpact on OPCache, that dmitry and other contributors work hard on to improve/fix every day, is another point that should let us think about a minute : we just can't afford that. 5.6 is late, for many reasons. We need to release it ASAP, but not with code we don't fully trust. Thank you. Julien.Pauli --- ## #76157 — Nikita Popov — 2014-07-26T15:42:13+00:00 URL: https://externals.io/message/75901#76157 · In reply to: #76145 > > > > […] > > I agree here. > > Bob, if you've been late proposing an RFC and couldn't get it up in time, > I'm sorry about it. > Please, understand we are very close to a release, and ATM, we're still > talking about weither this *user land* feature or that *user land* feature > will > be available, makes sense, or not. (that's a technical feature, but it > involves user land here). > That can't be ! > > We absolutely need something we, maintainers - creators - authors , think > is *stable*. > The PHP project needs that, we all agree. > > If the static array feature can't be added to the AST safely, please, > remove it. > > You've already added so much new great code (AST for this example) to 5.6, > I personnally thank you about this, however now it is really time to make > it stable, and commit its new features you may think about into 5.7 (or > what will be 5.7, ATM : master) , and write new RFCs about those if needed. > > Knowing your actual work in addition of that, is deep into the engine and > has inpact on OPCache, that dmitry and other contributors work hard on to > improve/fix every day, is another point that should let us think about a > minute : we just can't afford that. > > 5.6 is late, for many reasons. We need to release it ASAP, but not with > code we don't fully trust. > > Thank you. > > Julien.Pauli > Could somebody please clarify what issues are still open here? From what I understand, both the opcache issue and the recursion issue are fixed now. What's the discussion about? One point I'd like to be considered is the ability to dereference arrays in constant scalar expressions. Currently this syntax allows things like Foo[0], which are not allowed by normal PHP expressions. I would suggest either removing the ability to dereference altogether or at least make sure that it matches normal PHP expressions. Nikita --- ## #76160 — Ferenc Kovacs — 2014-07-26T20:41:21+00:00 URL: https://externals.io/message/75901#76160 · In reply to: #76157 > > > > […] > feature > > […] > 5.6, > > […] > needed. > > […] > > Could somebody please clarify what issues are still open here? From what I > understand, both the opcache issue and the recursion issue are fixed now. > What's the discussion about? > > One point I'd like to be considered is the ability to dereference arrays in > constant scalar expressions. Currently this syntax allows things like > Foo[0], which are not allowed by normal PHP expressions. I would suggest > either removing the ability to dereference altogether or at least make sure > that it matches normal PHP expressions. > > Nikita > my guess is that Julien also missed the fixes from Dmitry. -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #76165 — Stas Malyshev — 2014-07-26T22:02:43+00:00 URL: https://externals.io/message/75901#76165 · In reply to: #76157 Hi! > Could somebody please clarify what issues are still open here? From what > I understand, both the opcache issue and the recursion issue are fixed > now. What's the discussion about? As I understand, the issue is that if you define class constant like this: class Foo { const Bar = [0]; } everything works fine. But if you do var_dump(Foo::Bar), it bombs out with fatal error (the same goes for every other usage of that constant in expression). Please correct me if my info is outdated, but I think it is a behavior that should not be left in the release. If for some reason we can't make array constants work normally, we should just omit them altogether. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #76189 — Nikita Popov — 2014-07-27T08:52:22+00:00 URL: https://externals.io/message/75901#76189 · In reply to: #76165 On Sun, Jul 27, 2014 at 12:02 AM, Stas Malyshev wrote: > Hi! > > > […] > > As I understand, the issue is that if you define class constant like this: > > class Foo { const Bar = [0]; } > > everything works fine. But if you do var_dump(Foo::Bar), it bombs out > with fatal error (the same goes for every other usage of that constant > in expression). Please correct me if my info is outdated, but I think it > is a behavior that should not be left in the release. If for some reason > we can't make array constants work normally, we should just omit them > altogether. > Yes, I agree that this is not correct behavior - and I don't really understand why it was introduced and why it isn't trivial to fix. PHP-5.5 had a check for this case in place ( http://lxr.php.net/xref/PHP_5_5/Zend/zend_compile.c#7071) and phpng contains an AST-compatible variant of the array check ( http://lxr.php.net/xref/phpng/Zend/zend_compile.c#7776). Shouldn't copying the condition from phpng into PHP-5.6 resolve this issue? Nikita --- ## #76190 — Stas Malyshev — 2014-07-27T08:55:37+00:00 URL: https://externals.io/message/75901#76190 · In reply to: #76189 Hi! > Yes, I agree that this is not correct behavior - and I don't really > understand why it was introduced and why it isn't trivial to fix. > PHP-5.5 had a check for this case in place > (http://lxr.php.net/xref/PHP_5_5/Zend/zend_compile.c#7071) and phpng > contains an AST-compatible variant of the array check > (http://lxr.php.net/xref/phpng/Zend/zend_compile.c#7776). Shouldn't > copying the condition from phpng into PHP-5.6 resolve this issue? I agree it should be easy to fix it this way, but I'd like for Bob to provide a bit more input here as to best way to resolve it. I'm not sure why usage of arrays in runtime is disallowed now in 5.6 code, so I'm not sure if we should enable it or remove it. If we don't find another way soon, I guess porting one from phpng is what we'll have to do. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ --- ## #76194 — Bob Weinand — 2014-07-27T11:02:32+00:00 URL: https://externals.io/message/75901#76194 · In reply to: #76190 Am 27.7.2014 um 10:55 schrieb Stas Malyshev : > Hi! > > > […] > > I agree it should be easy to fix it this way, but I'd like for Bob to > provide a bit more input here as to best way to resolve it. I'm not sure > why usage of arrays in runtime is disallowed now in 5.6 code, so I'm not > sure if we should enable it or remove it. > > If we don't find another way soon, I guess porting one from phpng is > what we'll have to do. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ The AST compatible fix in phpng is just for top-level arrays, but not for something like "constant ? [1] : [2]". I think we should just enable it, that would lower the level of confusion. I totally agree that current status isn't optimal. Bob --- ## #76331 — Dmitry Stogov — 2014-08-04T09:53:23+00:00 URL: https://externals.io/message/75901#76331 · In reply to: #76194 Bob, I see you already committed this into PHP-5.6. I would agree that new behavior is more consistent and may be committed into master, but I'm still afraid that it may bring as problems because of last minute changes and lack of tests coverage. Thanks. Dmitry. > Am 27.7.2014 um 10:55 schrieb Stas Malyshev : > > Hi! > > Yes, I agree that this is not correct behavior - and I don't really > understand why it was introduced and why it isn't trivial to fix. > PHP-5.5 had a check for this case in place > (http://lxr.php.net/xref/PHP_5_5/Zend/zend_compile.c#7071) and phpng > contains an AST-compatible variant of the array check > (http://lxr.php.net/xref/phpng/Zend/zend_compile.c#7776). Shouldn't > copying the condition from phpng into PHP-5.6 resolve this issue? > > > I agree it should be easy to fix it this way, but I'd like for Bob to > provide a bit more input here as to best way to resolve it. I'm not sure > why usage of arrays in runtime is disallowed now in 5.6 code, so I'm not > sure if we should enable it or remove it. > > If we don't find another way soon, I guess porting one from phpng is > what we'll have to do. > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > > > The AST compatible fix in phpng is just for top-level arrays, but not for > something like "constant ? [1] : [2]". > > I think we should just enable it, that would lower the level of confusion. > > I totally agree that current status isn't optimal. > > Bob