"finally" handling refactoring (Bug #72213)

9 years ago by Dmitry Stogov — view source — reply

unread

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.

9 years ago by Derick Rethans — view source — reply

unread

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

9 years ago by Nikita Popov — view source — reply

unread

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

9 years ago by Nikita Popov — view source — reply

unread

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

<?php

function test() {
try {
throw new Exception(1);
} finally {
try {
throw new Exception(2);
} finally {
return 42;
}
}
}

test();

Nikita

9 years ago by Dmitry Stogov — view source — reply

unread

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?

Thanks. Dmitry.

9 years ago by Nikita Popov — view source — reply

unread

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

9 years ago by Dmitry Stogov — view source — reply

unread

Hi,

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

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

9 years ago by Nikita Popov — view source — reply

unread

Hi,

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

9 years ago by Dmitry Stogov — view source — reply

unread

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.