Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103630
To: Levi Morrison <levim@php.net>
CC: internals <internals@lists.php.net>
Thread-Topic: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant
 Parameters
Thread-Index: AQHUl/AqXVQDKTyJX0uvYhkVZIp9sqWHwGIygABrD7KAACiCAIAAeZEAgAUDC8Y=
Date: Mon, 24 Dec 2018 12:20:54 +0000
Message-ID:
 <DM6PR02MB5932BD07A788466EA2880114BFBB0@DM6PR02MB5932.namprd02.prod.outlook.com>
References:
 <CAFMT4Nodo_8dw4UkwZqT4vzE1=27xd7wczf=JxYu=R3j71xXag@mail.gmail.com>
 <DM6PR02MB593219A576A1618E7D600C9DBFBF0@DM6PR02MB5932.namprd02.prod.outlook.com>
 <DM6PR02MB5932213FE22E684434FD1557BFBF0@DM6PR02MB5932.namprd02.prod.outlook.com>
 <CAFMT4NrRQnkttJDdBXpyB=8EQC62_MnRfGAJU1hPp3nbUCW-dA@mail.gmail.com>,<f91e88da-a873-dd0a-28e2-4c58cd685064@zend.com>
In-Reply-To: <f91e88da-a873-dd0a-28e2-4c58cd685064@zend.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: zend.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: multipart/alternative;
	boundary="_000_DM6PR02MB5932BD07A788466EA2880114BFBB0DM6PR02MB5932namp_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: e6692258-7e76-4079-66ec-08d6699a40ca
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Dec 2018 12:20:54.9458
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR02MB5962
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant
 Parameters
From: dmitry@zend.com (Dmitry Stogov)

--_000_DM6PR02MB5932BD07A788466EA2880114BFBB0DM6PR02MB5932namp_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Hi Levi,


I made just few tests, to understand that the implementation is at least in=
complete.

The warning message depends on class declaration order and may be emitted o=
r 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 =3D 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 i=
mplementation.

Probably, it's better to cancel the voting and restart when the implementat=
ion is ready.


It's pity to see, that nobody tries the implementation but blindly vote...


Thanks. Dmitry.

________________________________
From: Dmitry Stogov <dmitry@zend.com>
Sent: Friday, December 21, 2018 10:28:05 AM
To: Levi Morrison
Cc: internals
Subject: Re: [PHP-DEV] [RFC][Vote] Covariant Returns and Contravariant Para=
meters



On 12/21/18 3:12 AM, Levi Morrison wrote:
> On Thu, Dec 20, 2018 at 3:35 PM Dmitry Stogov <dmitry@zend.com> 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/a2305381a5c9982cceca9e4e25=
2d26c7 shows use-after-free in opcache (works fine with master).
>>
>> Inability to work with opcache, doesn't allow to check the performance i=
mpact.
>>
>>
>> It also broke few tests. Some crash. Some produce different warning/erro=
rs.
>>
>>
>> $ make test TESTS=3D"Zend tests"
>>
>> ...
>>
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> FAILED TEST SUMMARY
>> ---------------------------------------------------------------------
>> ZE2 ArrayAccess and ArrayAccessReferenceProxy with references to main ar=
ray [tests/classes/array_access_011.phpt]
>> Bug #21478 (Zend/zend_alloc.c :: shutdown_memory_manager produces segfau=
lt) [Zend/tests/bug21478.phpt]
>> Generator methods can yield by reference [Zend/tests/generators/generato=
r_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 wi=
th normal inheritance, however, we do add strict warnings since it is easie=
r to run into something unexpeted with changing traits. [Zend/tests/traits/=
property009.phpt]
>> iterable type#004 - Parameter covariance [Zend/tests/type_declarations/i=
terable_004.phpt]
>> iterable type#005 - Return type covariance [Zend/tests/type_declarations=
/iterable_005.phpt]
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>
>> 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 v=
oting 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=3Dopcache.so
opcache.enable=3D1
opcache.enable_cli=3D1
opcache.optimization_level=3D-1
opcache.protect_memory=3D1 ; 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=3D"-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.

--_000_DM6PR02MB5932BD07A788466EA2880114BFBB0DM6PR02MB5932namp_--