On Mon, Aug 18, 2014 at 3:39 PM, Nikita Popov <nikita....@gmail.com> wrote:
> On Mon, Aug 18, 2014 at 1:19 PM, Dmitry Stogov <dmi...@zend.com> wrote:
>
>> On Mon, Aug 18, 2014 at 2:26 PM, Nikita Popov <nikita....@gmail.com>
>> wrote:
>>
>>> On Mon, Aug 18, 2014 at 9:32 AM, Dmitry Stogov <dmi...@zend.com> wrote:
>>>
>>>> Hi Nikita,
>>>>
>>>> I think RFC misses few important notes about behavior changes:
>>>>
>>>> 1) The behavior of $a->$b[$c] is changed. PHP apps that used such
>>>> syntax have to be changed into $a->{$b[$c]}.
>>>>
>>>
>>> This is a change from the uniform variable syntax (
>>> https://wiki.php.net/rfc/uniform_variable_syntax), not due to the AST.
>>> The AST patch already includes the uniform variable syntax patch for
>>> technical reasons.
>>>
>>
>> I missed it. I think you should add a clear note that AST RFC also
>> implements "Uniform Variable Syntax" (completely or with some exceptions).
>>
>>
>>>
>>>
>>>> 2) The evaluation order of left and right parts of assignment is
>>>> changed. $a[$i++] = $a[$i++]; It wasn't guaranteed before, but it may break
>>>> some code anyway.
>>>>
>>>
>>> This is no longer true, the AST now implements left-to-right evaluation
>>> for assignments.
>>>
>>
>> great. I don't talk that the old evaluation order better or worse. I just
>> don't see a big reason to change it.
>>
>>
>>>
>>>
>>>> 3) $a=[1,2]; list($a,$b) = $a; won't work in the same way as before
>>>>
>>>
>>> Yes, this is true. I think assigning list() elements from left-to-right
>>> makes a lot more sense and I also think that this is the kind of behavior
>>> that it's okay to change in a major version. In any case, I've added this
>>> particular example to the list() section of the RFC as well.
>>>
>>
>> thanks.
>>
>>
>>>
>>>
>>>> 4) Usage of undefined constants now leads to fatal errors? (see
>>>> Zend/tests/use_const/no_global_fallback.php)
>>>>
>>>
>>> This was a bug in the previous implementation - it uses a namespaced
>>> constant "foo\bar\baz" and usage of undefined namespaced constants results
>>> in a fatal error. This does not change that undefined unqualified constants
>>> (like just "baz") will results in a notice and string fallback. This only
>>> fixes the resolution of "use const" imports.
>>>
>>> 5) The patch also enables expressions on constant arrays e.g.
>>>> isset(arra(1,2,3)[$x]);
>>>>
>>>
>>> This is also a change from the uniform variable syntax, not from the AST.
>>>
>>>
>>>> Personally, I would prefer to separate syntax and behavior changes
>>>> from the AST patch itself or at least don't miss such changes, because they
>>>> must be very significant from users point of view.
>>>>
>>>
>>> I think the confusion here is mainly that the AST patch already includes
>>> the UVS patch (which was already voted in previously). I didn't list the
>>> changes from the UVS patch in this RFC, because they're already listed in
>>> https://wiki.php.net/rfc/uniform_variable_syntax.
>>>
>>>
>>>> Also some implementation related notes:
>>>>
>>>> - Changes to Zend/tests/bug43450.phpt are unrelated to AST. It's just
>>>> a bad test that may be fixed separately.
>>>>
>>>
>>> The AST makes it fail due to different memory usage patterns, which is
>>> why I changed it to use a test compatible with different peak memory usage.
>>>
>>
>> This test is ugly, and it starts fail from time to time after changes in
>> absolutely unrelated areas :(
>>
>
> Maybe we should just remove the memory usage checks and rely on ZMM
> warnings in debug mode?
>
The test is intended to check that the engine completely frees unused
resources at specific points,
but it may fail if MM cache some of released memory blocks for quick reuse.
It's better to ignore this test failure now, then hide it. Most probably
this failure is completely unrelated to AST, however few weeks ago it
helped me found a real problem in phpng.
>
>
>>
>>
>>>
>>> - I didn't get why you deleted Zend/tests/errmsg_014.php
>>>>
>>>
>>> I allowed doing $obj->__clone() calls, because __clone was the only
>>> magic method that had a compile-time checks preventing some calls to it. If
>>> we allow all other magic methods to be called, then there's no reason to
>>> forbid __clone. I've added a note about this to the RFC.
>>>
>>
>> ok. I can't remember why it prohibited.
>>
>>
>>>
>>>
>>>> - ZEND_INIT_FCALL_BY_NAME handler shouldn't check if constant operand
>>>> is a string. Compiler must not provide non-string constant operand. (All
>>>> the changes to ZEND_INIT_FCALL_BY_NAME need to be verified more
>>>> careful)
>>>>
>>>
>>> How should this be implemented instead? The case where an IS_CONST
>>> operand is called needs to be handled somewhere. One possible approach
>>> would be to send all constant-string calls through ZEND_INIT_FCALL. It uses
>>> the same code code as ZEND_INIT_FCALL_BY_NAME with the difference that
>>> BY_NAME has an extra zval literal storing the original (non-lowercased)
>>> function name. We could store this additional function name only in cases
>>> of a non-ct-bound function, because that's the only case where the
>>> undefined function error can be thrown. Does that make sense?
>>>
>>
>> I mean the additional check (Z_TYPE_P(opline->op2.zv) == IS_STRING) at
>> line 2391 must be useless.
>> Am I wrong?
>>
>> or is it intended to handle ["class_name", "method_name"]() syntax?
>>
>
> Yes, that's the case it handles.
>
uh :)
>
>
>>
>>>
>>>
>>>
>>>> - The same in ext/opcache/Optimizer/optimize_func_calls.c, but it's
>>>> not critical here.
>>>>
>>>> - I think you don't need IS_CONST operand for ZEND_ISSET_ISEMPTY_PROP_OBJ
>>>> handler.
>>>>
>>>> - Can OPcahce always keep AST in shared memory and don't copy it into
>>>> process memory on each request? (
>>>> ext/opcache/zend_accelerator_util_funcs.c)
>>>>
>>>
>>> I'm not particularly familiar with opcache, so I'm not sure whether this
>>> is possible. Right now this doesn't work because the constant expression
>>> AST is heap allocated, so we have efree calls. But if we allocate the
>>> constant-expression AST in CG(arena), maybe this would be possible?
>>>
>>
>> If the constant expression AST is not changed at run-time it may be kept
>> in SHM.
>> In this case you may just avoid copying from SHM to process memory and
>> keep pointers to AST in SHM.
>> Probably, some special flags should be introduced to avoid such AST
>> destruction.
>>
>> You may run PHP disable the AST copying code and run PHP with
>> "opcache.protect_memory=1".
>> Once PHP will try to modify the SHM contents, it'll crash.
>>
>
> Looking at the AST evaluation code, I think this would be problematic:
> http://lxr.php.net/xref/phpng/Zend/zend_ast.c#286 Here the constant is
> updated in-place.
>
I remember some related problems, but it would be great to find a better
solution.
Of course, it's not required to do it together with AST patch, it might be
done later or during vote
Some more problems I found:
- few tests started fail with the patch
Zend Multibyte and ShiftJIS
[Zend/tests/multibyte/multibyte_encoding_001.phpt]
zend multibyte (8) [ext/mbstring/tests/zend_multibyte-08.phpt]
serialization: arrays with references amonst elements
[ext/standard/tests/serialize/serialization_arrays_002.phpt]
Object serialization / unserialization: references amongst properties
[ext/standard/tests/serialize/serialization_objects_013.phpt]
- `make install-pear` reports memory leaks in debug build
Thanks. Dmitry.
>
> Nikita
>
