# Wired constant expression syntax and bug - Thread: https://externals.io/message/75149 - Original archive: https://news-web.php.net/php.internals/75149 - Started by Dmitry Stogov on 2014-06-30T19:06:06+00:00 - Messages: 16 - 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 --- ## #75149 — Dmitry Stogov — 2014-06-30T19:06:06+00:00 URL: https://externals.io/message/75149#75149 Hi Bob, I'm wondered why you introduced this wired syntax in PHP-5.6. class FooBar { const bar = ["bar" => 3]["bar"]; } It wasn't a part of RFC, it wasn't covered by tests, and it actually doesn't make a lot of sense. May be it's better to remove it? Also I found a constant expression related bug, that leads to unpredictable crashes from time to time. Previously we had IS_CONSTANT_ARRAY that was handled in a special way. When you replaced it with IS_ARRAY, you missed this handling, and I missed it as well when reviewed your patch. Now IS_ARRAY constants might be incompletely copied from OPCache shared memory and modified (incremented/decremented reference counter) directly in SHM. Such modifications occur in simultaneously running processes and this finally leads to crash on some race condition. It's possible to emulate the problem running the following script with opcache.protect_memory=1 I propose a simple fix: https://gist.github.com/dstogov/b73884e252b376957ebc Please review and apply if agree. Thanks. Dmitry. --- ## #75150 — Alexey Zakhlestin — 2014-06-30T19:17:11+00:00 URL: https://externals.io/message/75149#75150 · In reply to: #75149 > I'm wondered why you introduced this wired syntax in PHP-5.6. > > class FooBar { > const bar = ["bar" => 3]["bar"]; > } > > It wasn't a part of RFC, it wasn't covered by tests, and it actually > doesn't make a lot of sense. May be it's better to remove it? Does it harm? This example doesn’t make much sense, but intention of such code is unambiguous. It is clear what should happen. So, I’m +0 to leave it if tests are written. -- Alexey Zakhlestin CTO at Grids.by/you https://github.com/indeyets PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc --- ## #75151 — Dmitry Stogov — 2014-06-30T19:32:14+00:00 URL: https://externals.io/message/75149#75151 OK. I'll commit the patch. According to the wired syntax, note that your example code will make PHP at first construct an array and then destroy it. why not to write $var = DO ? "value1" : "value2"; it must be more clear and efficient. In case you like to keep this new syntax in PHP-5.6 at least please cover it with tests. I'm not a big fun of it, but won't object if it can't make real harm. Thanks. Dmitry. > That syntax wasn't part of the RFC because it wasn't yet possible > (existence of IS_CONSTANT_ARRAY at that time). > Also the use case for that one is mainly: > > class Foo { > public $var = ["value1", "value2"][DO]; > } > > And then just write > const DO = 0; (or 1) > > I don't exactly know at which point that conflicts with opcache. Or if > that really fixes the bug. > The copying itself in your patch looks fine, but please take the > responsibility for it and apply it yourself. > > Thanks, > Bob > > Am 30.6.2014 um 21:06 schrieb Dmitry Stogov : > > […] > unpredictable > > […] > memory > > […] > https://gist.github.com/dstogov/b73884e252b376957ebc > > […] --- ## #75152 — Dmitry Stogov — 2014-06-30T19:40:27+00:00 URL: https://externals.io/message/75149#75152 · In reply to: #75150 at least we didn't have a single test for this new syntax. Me and probably many others just didn't know about it. I just noticed Xinchen's commit, that fixed related bug. May be it's safe, may be not. I cant be sure. Thanks. Dmitry. On Mon, Jun 30, 2014 at 11:17 PM, Alexey Zakhlestin wrote: > > > > > […] > > Does it harm? > > This example doesn’t make much sense, but intention of such code is > unambiguous. It is clear what should happen. > So, I’m +0 to leave it if tests are written. > > -- > Alexey Zakhlestin > CTO at Grids.by/you > https://github.com/indeyets > PGP key: http://indeyets.ru/alexey.zakhlestin.pgp.asc --- ## #75154 — Andrea Faulds — 2014-06-30T21:05:17+00:00 URL: https://externals.io/message/75149#75154 · In reply to: #75149 > Hi Bob, > > I'm wondered why you introduced this wired syntax in PHP-5.6. Do you mean weird? > > class FooBar { > const bar = ["bar" => 3]["bar"]; > } > > It wasn't a part of RFC, it wasn't covered by tests, and it actually > doesn't make a lot of sense. May be it's better to remove it? I disagree, it makes perfect sense: class FooBar { const FOO = 3; const BAR = [ 3 => ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ][FOO]; } ?: and ? only work when there are just two possibilities. It is surprising that it wasn’t in the RFC though, I suspect that was just an oversight on Bob’s part. -- Andrea Faulds http://ajf.me/ --- ## #75155 — Bob Weinand — 2014-06-30T21:27:55+00:00 URL: https://externals.io/message/75149#75155 · In reply to: #75154 That wasn't in the RFC because there still was the IS_CONSTANT_ARRAY I removed only months later (and replaced with AST) to fix some bugs. It's only at that moment where I now could add that syntax. Bob Am 30.6.2014 um 23:05 schrieb Andrea Faulds : > > > > […] > > Do you mean weird? > > > […] > > I disagree, it makes perfect sense: > > class FooBar { > const FOO = 3; > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ][FOO]; > } > > ?: and ? only work when there are just two possibilities. > > It is surprising that it wasn’t in the RFC though, I suspect that was just an oversight on Bob’s part. > -- > Andrea Faulds > http://ajf.me/ --- ## #75156 — Bob Weinand — 2014-06-30T21:29:07+00:00 URL: https://externals.io/message/75149#75156 · In reply to: #75151 I'll add some tests later (Please remind me if I didn't add them until tomorrow evening). Bob Am 30.6.2014 um 21:32 schrieb Dmitry Stogov : > OK. I'll commit the patch. > > According to the wired syntax, note that your example code will make PHP at first construct an array and then destroy it. > > why not to write $var = DO ? "value1" : "value2"; > > it must be more clear and efficient. > > In case you like to keep this new syntax in PHP-5.6 at least please cover it with tests. > I'm not a big fun of it, but won't object if it can't make real harm. > > Thanks. Dmitry. > > > > That syntax wasn't part of the RFC because it wasn't yet possible (existence of IS_CONSTANT_ARRAY at that time). > Also the use case for that one is mainly: > > class Foo { > public $var = ["value1", "value2"][DO]; > } > > And then just write > const DO = 0; (or 1) > > I don't exactly know at which point that conflicts with opcache. Or if that really fixes the bug. > The copying itself in your patch looks fine, but please take the responsibility for it and apply it yourself. > > Thanks, > Bob > > Am 30.6.2014 um 21:06 schrieb Dmitry Stogov : > > […] --- ## #75158 — Dmitry Stogov — 2014-07-01T05:48:18+00:00 URL: https://externals.io/message/75149#75158 · In reply to: #75154 > > > > > […] > > Do you mean weird? > yes :) sorry > > > […] > > I disagree, it makes perfect sense: > > class FooBar { > const FOO = 3; > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ][FOO]; > } > > ?: and ? only work when there are just two possibilities. > ?: may work with brackets, but I have to admit that it's less readable. class Foo { const X = 2; const Y = (Foo::X == 0 ? 1 : (Foo::X == 1 ? 2 : (Foo::X == 2 ? 3 : (Foo::X == 3 ? 4 : 5)))); } The situation is really inconsistent because we allow expressions on constant arrays but at the same time prohibit array usage. Look into the following scripts: ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ][1]; } var_dump(Foo::BAR); // works - prints 'bang' ?> ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ]; } var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed in constants at run-time ?> ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ]; } // This works again! We may declare array class constants if we don't use them? ?> Where is the logic? Thanks. Dmitry. > > It is surprising that it wasn’t in the RFC though, I suspect that was just > an oversight on Bob’s part. > -- > Andrea Faulds > http://ajf.me/ --- ## #75159 — Nikita Popov — 2014-07-01T07:12:33+00:00 URL: https://externals.io/message/75149#75159 · In reply to: #75158 > > > > […] > > yes :) sorry > > > > […] > > ?: may work with brackets, but I have to admit that it's less readable. > > class Foo { > const X = 2; > const Y = (Foo::X == 0 ? 1 : > (Foo::X == 1 ? 2 : > (Foo::X == 2 ? 3 : > (Foo::X == 3 ? 4 : > 5)))); > } > > The situation is really inconsistent because we allow expressions on > constant arrays but at the same time prohibit array usage. > As we support the [..., ...][...] syntax in normal PHP code as well, I think it's reasonable to allow it here as well. Of course it isn't very practically useful, but there doesn't seem much point to explicitly disallowing it here. > Look into the following scripts: > > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ][1]; > } > var_dump(Foo::BAR); // works - prints 'bang' > ?> > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ]; > } > var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed > in constants at run-time > ?> > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ]; > } > // This works again! We may declare array class constants if we don't use > them? > ?> > > Where is the logic? > The reason here is probably that we cannot detect whether something will be an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 : [1] or similar. However we might want to detect the case where an array is the root of the AST, as that is invalid for sure and the most common case. Another small issue with the constant expressions I noticed is that our recursion detection doesn't properly work for constant ASTs. For example: > > […] > > > As we support the [..., ...][...] syntax in normal PHP code as well, I think > it's reasonable to allow it here as well. Of course it isn't very > practically useful, but there doesn't seem much point to explicitly > disallowing it here. > > > […] > > > The reason here is probably that we cannot detect whether something will be > an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 : > [1] or similar. However we might want to detect the case where an array is > the root of the AST, as that is invalid for sure and the most common case. > > Another small issue with the constant expressions I noticed is that our > recursion detection doesn't properly work for constant ASTs. For example: > > class A { > const FOO = [self::BAR]; > const BAR = [self::FOO]; > } > var_dump(A::FOO); > > This will result in a stack overflow instead of a fatal error about > self-referencing constants. So what's the status ? We release RC2 this week. Final is not far, and as I can read, it's not stable. So I suggest we use more RCs to try to make this stable (and I mean with OPCache activated). If we can't make it in reasonnable time, then the feature will have to be reverted. Please, note we won't have too many RCs as we are already late on our original timetable. Julien Pauli --- ## #75161 — Bob Weinand — 2014-07-01T11:25:12+00:00 URL: https://externals.io/message/75149#75161 · In reply to: #75159 Am 1.7.2014 um 09:12 schrieb Nikita Popov : > > > […] > > ?: may work with brackets, but I have to admit that it's less readable. > > class Foo { > const X = 2; > const Y = (Foo::X == 0 ? 1 : > (Foo::X == 1 ? 2 : > (Foo::X == 2 ? 3 : > (Foo::X == 3 ? 4 : > 5)))); > } > > The situation is really inconsistent because we allow expressions on > constant arrays but at the same time prohibit array usage. > > As we support the [..., ...][...] syntax in normal PHP code as well, I think it's reasonable to allow it here as well. Of course it isn't very practically useful, but there doesn't seem much point to explicitly disallowing it here. That's exactly my point. > Look into the following scripts: > > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ][1]; > } > var_dump(Foo::BAR); // works - prints 'bang' > ?> > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ]; > } > var_dump(Foo::BAR); // doesn't work - Fatal error: Arrays are not allowed > in constants at run-time > ?> > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ]; > } > // This works again! We may declare array class constants if we don't use > them? > ?> > > Where is the logic? > > The reason here is probably that we cannot detect whether something will be an array or not until runtime constant updating. E.g. const BAR = FOO ? 1 : [1] or similar. However we might want to detect the case where an array is the root of the AST, as that is invalid for sure and the most common case. Not sure if we should check that. That would be adding a new arbitrary restriction. Now the following works: class Foo { const BAR = [ 3 => ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ]; const BAZ = FOO[4]; } var_dump(Foo::BAZ); > Another small issue with the constant expressions I noticed is that our recursion detection doesn't properly work for constant ASTs. For example: > > class A { > const FOO = [self::BAR]; > const BAR = [self::FOO]; > } > var_dump(A::FOO); > > This will result in a stack overflow instead of a fatal error about self-referencing constants. > > Nikita I think that this is caused by some bugfix for opcache. http://lxr.php.net/xref/PHP_5_6/Zend/zend_ast.c#255 That zval_copy_ctor call duplicates the AST to save it for opcache so that opcache can store it later. But as that is called before the zend_ast_evaluate, the effects of MARK_CONSTANT_VISITED() won't affect the contents of the ast (and the zval with the constant) copied before… @Dmitry: Removing this call (and the dtoring in the ast destructor) will break opcache and fix this bug. How to fix both now? I don't immediately see a good fix here? Bob --- ## #75162 — Bob Weinand — 2014-07-01T11:48:29+00:00 URL: https://externals.io/message/75149#75162 · In reply to: #75161 Am 1.7.2014 um 13:25 schrieb Bob Weinand : > Not sure if we should check that. That would be adding a new arbitrary restriction. Now the following works: > > class Foo { > const BAR = [ > 3 => ‘qux’, > 4 => ‘bang’, > 7 => ‘theta’, > 9 => ‘epsilon’ > ]; > > const BAZ = FOO[4]; > } > > var_dump(Foo::BAZ); Woops, mistake in code… written too fast... Should be: class Foo { const BAR = [ 3 => ‘qux’, 4 => ‘bang’, 7 => ‘theta’, 9 => ‘epsilon’ ]; const BAZ = Foo::BAR[4]; } var_dump(Foo::BAZ); http://3v4l.org/78S2C#v560beta2 Bob --- ## #75164 — Bob Weinand — 2014-07-02T10:43:01+00:00 URL: https://externals.io/message/75149#75164 · In reply to: #75151 Am 30.6.2014 um 21:32 schrieb Dmitry Stogov : > In case you like to keep this new syntax in PHP-5.6 at least please cover > it with tests. I added two tests now, one of them is failing: the bug with the self-referencing constants Nikita showed here. I still have no idea how to fix it in a way to satisfy opcache and the self-references detector, except copying everything again (at every subnode)... Which is not the way to go… I hope? Any idea? Bob --- ## #75165 — Dmitry Stogov — 2014-07-02T13:43:20+00:00 URL: https://externals.io/message/75149#75165 · In reply to: #75164 I don't have good ideas out of the box and I probably won't be able to look into this before next week. I afraid this feature might be a big problem later, because in last minute we may miss some cases... Thanks. Dmitry. > Am 30.6.2014 um 21:32 schrieb Dmitry Stogov : > > In case you like to keep this new syntax in PHP-5.6 at least please cover > it with tests. > > > I added two tests now, > one of them is failing: the bug with the self-referencing constants Nikita > showed here. > I still have no idea how to fix it in a way to satisfy opcache and the > self-references detector, except copying everything again (at every > subnode)... Which is not the way to go… I hope? > Any idea? > > Bob --- ## #75807 — Bob Weinand — 2014-07-21T16:36:41+00:00 URL: https://externals.io/message/75149#75807 · In reply to: #75165 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 --- ## #75817 — Dmitry Stogov — 2014-07-22T06:00:55+00:00 URL: https://externals.io/message/75149#75817 · In reply to: #75807 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. > 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