Everything

php

"finally" handling refactoring (Bug #72213)

php.internals

Dmitry Stogov

10 years ago

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.

Derick Rethans

10 years ago

On Fri, 20 May 2016, Dmitry Stogov wrote:

> 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

Nikita Popov

10 years ago

On Fri, May 20, 2016 at 2:07 PM, Dmitry Stogov <dmitry@zend.com> wrote:

> 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

Nikita Popov

10 years ago

On Fri, May 20, 2016 at 2:07 PM, Dmitry Stogov <dmitry@zend.com> wrote:

> 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

Dmitry Stogov

10 years ago

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.

________________________________
From: Nikita Popov <nikita.ppv@gmail.com>
Sent: Friday, May 20, 2016 4:54:07 PM
To: Dmitry Stogov
Cc: Xinchen Hui; internals
Subject: Re: "finally" handling refactoring (Bug #72213)

On Fri, May 20, 2016 at 2:07 PM, Dmitry Stogov <dmitry@zend.com<mailto:dmitry@zend.com>> wrote:

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

Nikita Popov

10 years ago

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

Dmitry Stogov

10 years ago

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

10 years ago

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.

Dmitry Stogov

10 years ago

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