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

Search Discussions

  • Derick Rethans at May 20, 2016 at 12:47 pm

    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 at May 20, 2016 at 1:41 pm

    On Fri, May 20, 2016 at 2:07 PM, Dmitry Stogov 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 at May 20, 2016 at 1:54 pm

    On Fri, May 20, 2016 at 2:07 PM, Dmitry Stogov 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 at May 23, 2016 at 11:25 am
    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 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 at May 23, 2016 at 4:25 pm

    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 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 at May 23, 2016 at 9:36 pm
    Hi,

    On 05/23/2016 07:24 PM, Nikita Popov wrote:
    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
  • Nikita Popov at May 23, 2016 at 10:12 pm

    On Mon, May 23, 2016 at 11:36 PM, Dmitry Stogov wrote:

    Hi,

    On 05/23/2016 07:24 PM, Nikita Popov wrote:
    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>
    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

  • Dmitry Stogov at May 24, 2016 at 7:46 am
    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 wrote:

    Hi,

    On 05/23/2016 07:24 PM, Nikita Popov wrote:
    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> 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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMay 20, '16 at 12:07p
activeMay 24, '16 at 7:46a
posts9
users3
websitephp.net

People

Translate

site design / logo © 2018 Grokbase