Wired constant expression syntax and bug

10 years ago by Dmitry Stogov — view source

unread

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

<?php
function foo($query = NULL, array $exclude = array('q')) {}
foo();
?>

I propose a simple fix: https://gist.github.com/dstogov/b73884e252b376957ebc
Please review and apply if agree.

Thanks. Dmitry.

10 years ago by Alexey Zakhlestin — view source

unread

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

10 years ago by Dmitry Stogov — view source

unread

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 indeyets@gmail.com
wrote:

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

10 years ago by Andrea Faulds — view source

unread

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/

10 years ago by Bob Weinand — view source

unread

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 ajf@ajf.me:

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/

10 years ago by Dmitry Stogov — view source

unread

Hi Bob,

I'm wondered why you introduced this wired syntax in PHP-5.6.

Do you mean weird?

yes :) sorry

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.

?: 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:

<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?>

<?php
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
?>

<?php
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?

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/

10 years ago by Nikita Popov — view source

unread

Hi Bob,

I'm wondered why you introduced this wired syntax in PHP-5.6.

Do you mean weird?

yes :) sorry

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.

?: 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:

<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?>

<?php
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
?>

<?php
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:

<?php
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

10 years ago by Julien Pauli — view source

unread

Hi Bob,

I'm wondered why you introduced this wired syntax in PHP-5.6.

Do you mean weird?

yes :) sorry

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.

?: 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:

<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?>

<?php
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
?>

<?php
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:

<?php
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

10 years ago by Bob Weinand — view source

unread

Am 1.7.2014 um 09:12 schrieb Nikita Popov nikita.ppv@gmail.com:

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.

?: 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:

<?php
class Foo {
const BAR = [
3 => ‘qux’,
4 => ‘bang’,
7 => ‘theta’,
9 => ‘epsilon’
][1];
}
var_dump(Foo::BAR); // works - prints 'bang'
?>

<?php
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
?>

<?php
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:

<?php
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

10 years ago by Bob Weinand — view source

unread

Am 1.7.2014 um 13:25 schrieb Bob Weinand bobwei9@hotmail.com:

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

10 years ago by Dmitry Stogov — view source

unread

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 dmitry@zend.com:

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

<?php
function foo($query = NULL, array $exclude = array('q')) {}
foo();
?>

I propose a simple fix:
https://gist.github.com/dstogov/b73884e252b376957ebc
Please review and apply if agree.

Thanks. Dmitry.

10 years ago by Bob Weinand — view source

unread

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 dmitry@zend.com:

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 dmitry@zend.com:

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

<?php
function foo($query = NULL, array $exclude = array('q')) {}
foo();
?>

I propose a simple fix: https://gist.github.com/dstogov/b73884e252b376957ebc
Please review and apply if agree.

Thanks. Dmitry.

10 years ago by Bob Weinand — view source

unread

Am 30.6.2014 um 21:32 schrieb Dmitry Stogov dmitry@zend.com:

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

10 years ago by Dmitry Stogov — view source

unread

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 dmitry@zend.com:

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

10 years ago by Bob Weinand — view source

unread

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

10 years ago by Dmitry Stogov — view source

unread

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 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