# "finally" handling refactoring (Bug #72213) - Thread: https://externals.io/message/93413 - Original archive: https://news-web.php.net/php.internals/93413 - Started by Dmitry Stogov on 2016-05-20T12:07:12+00:00 - Messages: 9 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #93413 — Dmitry Stogov — 2016-05-20T12:07:12+00:00 URL: https://externals.io/message/93413#93413 hi, Please review the path https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330 I hope, it should completely fix https://bugs.php.net/bug.php?id=72213 I'm going to commit this on Monday. Thanks. Dmitry. --- ## #93414 — Derick Rethans — 2016-05-20T12:47:45+00:00 URL: https://externals.io/message/93413#93414 · In reply to: #93413 > Please review the path > https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330 https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L194-L196 https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L356-L358 Why are you switching these around? https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L500-L501 https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L511-L512 You should probably remove them instead of commenting them out. And Nikita, does this fix my issues with trying to walk through the possible paths and branches with Xdebug? > > I hope, it should completely fix https://bugs.php.net/bug.php?id=72213 > > > I'm going to commit this on Monday. To which branches? cheers, Derick --- ## #93417 — Nikita Popov — 2016-05-20T13:41:32+00:00 URL: https://externals.io/message/93413#93417 · In reply to: #93413 > hi, > > > Please review the path > https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330 > > > I hope, it should completely fix https://bugs.php.net/bug.php?id=72213 > > > I'm going to commit this on Monday. > > > Thanks. Dmitry. > USE_ZEND_ALLOC=0 valgrind sapi/cli/php Zend/tests/bug65784.php gives me ==27339== Invalid read of size 8 ==27339== at 0xB93F76: ZEND_CATCH_SPEC_CONST_CV_HANDLER (zend_vm_execute.h:9362) ==27339== by 0xB7E872: execute_ex (zend_vm_execute.h:426) ==27339== by 0xB7EA74: zend_execute (zend_vm_execute.h:471) ==27339== by 0xB161EA: zend_execute_scripts (zend.c:1427) ==27339== by 0xA5319D: php_execute_script (main.c:2492) ==27339== by 0xC058D5: do_cli (php_cli.c:982) ==27339== by 0xC06D65: main (php_cli.c:1352) ==27339== Address 0xf5e0880 is 16 bytes inside a block of size 152 free'd ==27339== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==27339== by 0xAD5B2C: _efree (zend_alloc.c:2423) ==27339== by 0xB6C4A6: zend_objects_store_del (zend_objects_API.c:187) ==27339== by 0xB10FF6: _zval_dtor_func (zend_variables.c:56) ==27339== by 0xB6445E: i_zval_ptr_dtor (zend_variables.h:48) ==27339== by 0xB64636: zend_object_std_dtor (zend_objects.c:68) ==27339== by 0xB6C38D: zend_objects_store_del (zend_objects_API.c:178) ==27339== by 0xB10FF6: _zval_dtor_func (zend_variables.c:56) ==27339== by 0xB7BF6E: i_free_compiled_variables (zend_execute.c:2101) ==27339== by 0xB7EABD: zend_leave_helper_SPEC (zend_vm_execute.h:481) ==27339== by 0xB83F83: ZEND_FAST_RET_SPEC_HANDLER (zend_vm_execute.h:1894) ==27339== by 0xB7E872: execute_ex (zend_vm_execute.h:426) and so on. Nikita --- ## #93418 — Nikita Popov — 2016-05-20T13:54:07+00:00 URL: https://externals.io/message/93413#93418 · In reply to: #93413 > hi, > > > Please review the path > https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330 > > > I hope, it should completely fix https://bugs.php.net/bug.php?id=72213 > > > I'm going to commit this on Monday. > > > Thanks. Dmitry. > From a quick look: https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L74 => should this be orig_try_catch_offset? https://gist.github.com/dstogov/0a809891c6a3ac3fac4bd0d9711dd330#file-bug72213-diff-L303 => This assumes that either there's a catch block or a finally block to go to. However, if we're in the catch block of a try/catch (without finally) that may not be true. Also, this still leaks one exception :( Thanks for review. > > Both problems should be fixed now > 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 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. Nikita --- ## #93460 — Dmitry Stogov — 2016-05-23T21:36:35+00:00 URL: https://externals.io/message/93413#93460 · In reply to: #93450 Hi, > On Mon, May 23, 2016 at 1:25 PM, Dmitry Stogov > wrote: > > Thanks for review. > > Both problems should be fixed now > 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? > > 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 --- ## #93462 — Nikita Popov — 2016-05-23T22:12:31+00:00 URL: https://externals.io/message/93413#93462 · In reply to: #93460 > Hi, > > > > > > > […] > 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 --- ## #93474 — Dmitry Stogov — 2016-05-24T07:45:55+00:00 URL: https://externals.io/message/93413#93474 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.