Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93462
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.161.175 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <fbe8c464-7e2e-8bc0-5434-0135788c6569@zend.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>
Date: Tue, 24 May 2016 00:12:31 +0200
Message-ID: <CAF+90c-PGRpP+peeWUx9xMuQN+tAbzeexpyBdVOXNe8=B-7aEg@mail.gmail.com>
To: Dmitry Stogov <dmitry@zend.com>
Cc: Xinchen Hui <xinchen.h@zend.com>, internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a1140723a988a9d053389bc38
Subject: Re: "finally" handling refactoring (Bug #72213)
From: nikita.ppv@gmail.com (Nikita Popov)

--001a1140723a988a9d053389bc38
Content-Type: text/plain; charset=UTF-8

On Mon, May 23, 2016 at 11:36 PM, Dmitry Stogov <dmitry@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> wrote:
>
>> Thanks for review.
>>
>> Both problems should be fixed now
>> <https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330>
>> https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330
>>
>> 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
> all 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
> discarded 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
which 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 in
this case, but I'm actually getting an assertion failure (and a conditional
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 == 7 || (ref)->gc.u.v.type == 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
> replace it with a DISCARD_EXCEPTION and then pop it after the finally. This
> should generate all the necessary DISCARD_EXCEPTION opcodes, and in the
> right order.
>
>
> May be I'll commit the existing fix, and you'll try to implement this idea
> on top?
>
> Thanks. Dmitry.
>
>
> Nikita
>
>
>

--001a1140723a988a9d053389bc38--