Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93474
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 65.55.169.102 as permitted sender)
To: Nikita Popov <nikita.ppv@gmail.com>
CC: Xinchen Hui <xinchen.h@zend.com>, internals <internals@lists.php.net>
Thread-Topic: "finally" handling refactoring (Bug #72213)
Thread-Index: AQHRsooB/asZC+6F5UyFZdE4B+ax2p/B2ROAgASM9cCAAFQwgIAAiVqA///XwICAAJ7QCg==
Date: Tue, 24 May 2016 07:45:55 +0000
Message-ID: <BY2PR0201MB178400103CA4255121229471BF4F0@BY2PR0201MB1784.namprd02.prod.outlook.com>
References: <BY2PR0201MB1784296F01E558FF22C48C7ABF4B0@BY2PR0201MB1784.namprd02.prod.outlook.com>
	<CAF+90c_KWrE0PE9p-BWZz9uXaLFUarnbp3h30rN3THz9S7pLjQ@mail.gmail.com>
	<BY2PR0201MB1784A01C040C1332E4B3D7E8BF4E0@BY2PR0201MB1784.namprd02.prod.outlook.com>
	<CAF+90c8NFf2wO4myKtXfUK4GEpUY4ttjS5bePYOKYnYKrNww4g@mail.gmail.com>
	<fbe8c464-7e2e-8bc0-5434-0135788c6569@zend.com>,<CAF+90c-PGRpP+peeWUx9xMuQN+tAbzeexpyBdVOXNe8=B-7aEg@mail.gmail.com>
In-Reply-To: <CAF+90c-PGRpP+peeWUx9xMuQN+tAbzeexpyBdVOXNe8=B-7aEg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:23
spamdiagnosticmetadata: NSPM
Content-Type: multipart/alternative;
	boundary="_000_BY2PR0201MB178400103CA4255121229471BF4F0BY2PR0201MB1784_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 May 2016 07:45:55.1958
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR02MB1710
Subject: Re: "finally" handling refactoring (Bug #72213)
From: dmitry@zend.com (Dmitry Stogov)

--_000_BY2PR0201MB178400103CA4255121229471BF4F0BY2PR0201MB1784_
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

Ok. One more attempt with both tests fixed.


https://github.com/php/php-src/pull/1919


Nikita, you may take a look only into the last commit.

Do you see any other problems?

Sorry for abusing your time, but your feedback is extremely useful.


Thanks. Dmitry.

Thanks. Dmitry.

________________________________
From: Nikita Popov <nikita.ppv@gmail.com>
Sent: Tuesday, May 24, 2016 1:12:31 AM
To: Dmitry Stogov
Cc: Xinchen Hui; internals
Subject: Re: "finally" handling refactoring (Bug #72213)

On Mon, May 23, 2016 at 11:36 PM, Dmitry Stogov <dmitry@zend.com<mailto:dmi=
try@zend.com>> wrote:

Hi,

On 05/23/2016 07:24 PM, Nikita Popov wrote:
On Mon, May 23, 2016 at 1:25 PM, Dmitry Stogov <dmitry@zend.com<mailto:dmit=
ry@zend.com>> wrote:

Thanks for review.

Both problems should be fixed now <https://gist.github.com/dstogov/0a809891=
c6a3ac3fac4bd0d9711dd330> https://gist.github.com/dstogov/0a809891c6a3ac3fa=
c4bd0d9711dd330

Do you see any other problems or a better way to fix this?

Your fix for DISCARD_EXCEPTION does not look right to me. It will discard a=
ll exceptions too early. For example:

function test() {
    try {
        throw new Exception(1);
    } finally {
        try {
            try {
            } finally {
                return 42;
            }
        } finally {
            throw new Exception(2);
        }
    }
}

test();

This will now not chain exception 1 and 2, because exception 1 is discarded=
 at the return statement.

I thought about this, and I think that the current behavior is right.
If we remove "throw new Exception(2);" the first exception is going to be d=
iscarded at "return 42;".
I think we should do the same independently from the context.

Why do you think Exception(1) should be kept?

Because Exception(3) may be discarded by another catch in the finally, in w=
hich case we should throw Exception(1). Consider this case:

function test() {
    try {
        throw new Exception(1);
    } finally {
        try {
            try {
                try {
                } finally {
                    return 42;
                }
            } finally {
                throw new Exception(3);
            }
        } catch (Exception $e) {}
    }
}

test();

This should throw Exception(1), as Exception(3) is thrown and caught inside=
 the finally block. I thought your current patch would not throw anything i=
n this case, but I'm actually getting an assertion failure (and a condition=
al jump on uninit value in valgrind):

php: /home/nikic/php-src/Zend/zend_gc.c:226: gc_possible_root: Assertion `(=
ref)->gc.u.v.type =3D=3D 7 || (ref)->gc.u.v.type =3D=3D 8' failed.


I think this should be handled the same way we do the fast_call dispatch on=
 return, i.e. when we pop the FAST_CALL from the loop var stack we should r=
eplace it with a DISCARD_EXCEPTION and then pop it after the finally. This =
should generate all the necessary DISCARD_EXCEPTION opcodes, and in the rig=
ht order.

May be I'll commit the existing fix, and you'll try to implement this idea =
on top?

Thanks. Dmitry.


Nikita




--_000_BY2PR0201MB178400103CA4255121229471BF4F0BY2PR0201MB1784_--