Re: "finally" handling refactoring (Bug #72213)

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 > > >