Weird constant expression syntax and bug

10 years ago by Dmitry Stogov — view source

unread

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:

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 = [1];
}
?>

In some cases array constants may be used, but not in the others.

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

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".
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 */

Circular constant references crash (found by Nikita)

<?php
class A {
const FOO = [self::BAR];
const BAR = [self::FOO];
}
var_dump(A::FOO); // crashes because of infinity recursion
?>

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.

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 Bob Weinand — view source

unread

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar context.

In some cases array constants may be used, but not in the others.

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

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

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.

Circular constant references crash (found by Nikita)

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

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

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)

                  *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

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

10 years ago by Stas Malyshev — view source

unread

Hi!

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

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.

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

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.

Circular constant references crash (found by Nikita)

<?php
class A {
const FOO = [self::BAR];
const BAR = [self::FOO];
}
var_dump(A::FOO); // crashes because of infinity recursion
?>

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/

10 years ago by Dmitry Stogov — view source

unread

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar context.

In some cases array constants may be used, but not in the others.

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

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.

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.

Circular constant references crash (found by Nikita)

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

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

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

10 years ago by Bob Weinand — view source

unread

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

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar context.

In some cases array constants may be used, but not in the others.

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

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

Circular constant references crash (found by Nikita)

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

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

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

10 years ago by Dmitry Stogov — view source

unread

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.

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

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar
context.

In some cases array constants may be used, but not in the others.

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

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

Circular constant references crash (found by Nikita)

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

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

10 years ago by Bob Weinand — view source

unread

Am 23.7.2014 um 11:34 schrieb Dmitry Stogov dmitry@zend.com:

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

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar
context.

In some cases array constants may be used, but not in the others.

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

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

Circular constant references crash (found by Nikita)

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

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

10 years ago by Dmitry Stogov — view source

unread

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

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

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:

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 = [1];
}
?>

Because it's actually valid. You don't use it in non-static scalar
context.

In some cases array constants may be used, but not in the others.

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

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.

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.

Circular constant references crash (found by Nikita)

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

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

10 years ago by Julien Pauli — view source

unread

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

10 years ago by Nikita Popov — view source

unread

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

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

10 years ago by Ferenc Kovacs — view source

unread

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

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

10 years ago by Stas Malyshev — view source

unread

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/

10 years ago by Nikita Popov — view source

unread

On Sun, Jul 27, 2014 at 12:02 AM, Stas Malyshev smalyshev@sugarcrm.com
wrote:

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.

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

10 years ago by Stas Malyshev — view source

unread

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/

10 years ago by Bob Weinand — view source

unread

Am 27.7.2014 um 10:55 schrieb Stas Malyshev smalyshev@sugarcrm.com:

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

10 years ago by Dmitry Stogov — view source

unread

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 smalyshev@sugarcrm.com:

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

10 years ago by Stas Malyshev — view source

unread

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/

10 years ago by Bob Weinand — view source

unread

Am 23.7.2014 um 22:33 schrieb Stas Malyshev smalyshev@sugarcrm.com:

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

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…

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.

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/

10 years ago by Pierre Joye — view source

unread

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

10 years ago by Kris Craig — view source

unread

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

--

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

10 years ago by Ferenc Kovacs — view source

unread

Am 23.7.2014 um 22:33 schrieb Stas Malyshev smalyshev@sugarcrm.com:

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

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…

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.

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/

--

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

10 years ago by Ferenc Kovacs — view source

unread

Am 23.7.2014 um 22:33 schrieb Stas Malyshev smalyshev@sugarcrm.com:

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

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…

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.

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/

--

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

Weird constant expression syntax and bug

If we don't find another way soon, I guess porting one from phpng is what we'll have to do.

If we don't find another way soon, I guess porting one from phpng is what we'll have to do.

If we don't find another way soon, I guess porting one from phpng is what we'll have to do.

Cheers,

Cheers,

If we don't find another way soon, I guess porting one from phpng is
what we'll have to do.

If we don't find another way soon, I guess porting one from phpng is
what we'll have to do.

If we don't find another way soon, I guess porting one from phpng is
what we'll have to do.