Hi Levi,
I made just few tests, to understand that the implementation is at least
incomplete.
The warning message depends on class declaration order and may be emitted or
not.
1. This test produces a warning (as RFC proposes)
<?php
class C {}
class D {}
class A {
function bar(C $c) {}
}
class B extends A {
function bar(D $D) {
echo "ok\n";
}
}
?>
2. But this test misses warning
<?php
$q = 1;
if ($q) {
class C {}
class D {}
}
class A {
function bar(C $c) {}
}
class B extends A {
function bar(D $D) {
echo "ok\n";
}
}
?>
- The patch is incompatible with opcache (crashes on Wordpress, Drupal, and
probably any real-life app).
- the incompatibility with opcache, doesn't allow to check the performance
implication of the patch
- the patch has merge conflicts and travis tests doesn't run.
Personally, I'm not against the proposal, but I'm definitely against this
implementation.
Probably, it's better to cancel the voting and restart when the implementation
is ready.
It's pity to see, that nobody tries the implementation but blindly vote...
Thanks. Dmitry.
________________________________
From: Dmitry Stogov <[email protected]>
Sent: Friday, December 21, 2018 10:28:05 AM
To: Levi Morrison
Cc: internals
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant
Parameters
On 12/21/18 3:12 AM, Levi Morrison wrote:
> On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov <[email protected]> wrote:
>>
>> Hi Levi,
>>
>>
>> It looks like the patch broke something related to opcache.
>>
>> It crashes at least on Wordpress and Drupal.
>>
>> The backtrace
>> https://gist.github.com/dstogov/a2305381a5c9982cceca9e4e252d26c7 shows
>> use-after-free in opcache (works fine with master).
>>
>> Inability to work with opcache, doesn't allow to check the performance
>> impact.
>>
>>
>> It also broke few tests. Some crash. Some produce different warning/errors.
>>
>>
>> $ make test TESTS="Zend tests"
>>
>> ...
>>
>> =====================================================================
>> FAILED TEST SUMMARY
>> ---------------------------------------------------------------------
>> ZE2 ArrayAccess and ArrayAccessReferenceProxy with references to main array
>> [tests/classes/array_access_011.phpt]
>> Bug #21478 (Zend/zend_alloc.c :: shutdown_memory_manager produces segfault)
>> [Zend/tests/bug21478.phpt]
>> Generator methods can yield by reference
>> [Zend/tests/generators/generator_method_by_ref.phpt]
>> Testing to implement Serializable interface by traits
>> [Zend/tests/traits/interface_003.phpt]
>> Handling of public fields with traits needs to have same semantics as with
>> normal inheritance, however, we do add strict warnings since it is easier to
>> run into something unexpeted with changing traits.
>> [Zend/tests/traits/property009.phpt]
>> iterable type#004 - Parameter covariance
>> [Zend/tests/type_declarations/iterable_004.phpt]
>> iterable type#005 - Return type covariance
>> [Zend/tests/type_declarations/iterable_005.phpt]
>> =====================================================================
>>
>> I'll try to play with patch and make a full code review on next week.
>>
>>
>> It would be great, if you fix opcache compatibility.
>>
>> If it can't be done in reasonable time, it's probably better to cancel
>> voting and restart when ready.
>
> What OS and compiler are these on?
Linux (Fedora 28), GCC 8.2.1.
> How are you ensuring that opcache
> is on when these tests are run? I have not been experiencing these
> issues, so maybe I am not running it correctly. If I cannot reproduce
> them soon then I will agree to cancel the voting.
You should enable opcache in your php.ini
zend_extension=opcache.so
opcache.enable=1
opcache.enable_cli=1
opcache.optimization_level=-1
opcache.protect_memory=1 ; this is for testing only
Memory corruption bugs may lead to crash or not because of "luck", but
it's possible to see them using valgrind.
$ make test TESTS="-m tests/classes/array_access_011.phpt"
$ cat tests/classes/array_access_011.mem
This shows almost the same backtrace, as I already published.
Looks like an incorrect reference-counting on some string.
>
> There are some known issues outside of Zend. Notably some internal
> classes do not have valid method signatures with regards to
> inheritance which this patch exposed. These need fixed regardless of
> this RFC and I have begun work on some of them (see
> https://github.com/php/php-src/pull/3686 for one example).
That PR looks fine.
There is also a problem that this PR has merge conflict with master and
travis doesn't run tests.
Thanks. Dmitry.
