Re: "finally" handling refactoring (Bug #72213)
| From: | Dmitry Stogov | Date: | Tue, 24 May 2016 07:45:55 +0000 |
| Subject: | Re: "finally" handling refactoring (Bug #72213) | ||
| References: | 1 2 3 4 5 6 | Groups: | php.internals |
| Request: | Send a blank email to internals+get-93474@lists.php.net to get a copy of this message | ||
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: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<mailto: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