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....@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 <dmi...@zend.com<mailto:dmi...@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 <dmi...@zend.com<mailto:dmi...@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
