# Safe Temporary Cleanup on Exception (Variable Liveness) - Thread: https://externals.io/message/89143 - Original archive: https://news-web.php.net/php.internals/89143 - Started by Dmitry Stogov on 2015-11-10T09:53:34+00:00 - Messages: 8 - 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 --- ## #89143 — Dmitry Stogov — 2015-11-10T09:53:34+00:00 URL: https://externals.io/message/89143#89143 Hi, Please take a look into https://gist.github.com/dstogov/43eaa3f3b68583877322 This is variation on Bob's idea about variable liveness. The patch is incomplete. Liveness construction is still naive. However, all tests are passed. Ranges are represented by start/end/variable cartages. (I suppose this representation is more compact, than in the original proposal). In general, it's possible to use few ranges for the same variable, if its whole live range is not linear. (this is not implemented yet). Related ideas and tests are welcome. Thanks. Dmitry. --- ## #89144 — Dmitry Stogov — 2015-11-10T10:03:55+00:00 URL: https://externals.io/message/89143#89144 · In reply to: #89143 BTW: I think, part of this patch (without zend_check_live_ranges) may be good enough, as a first step. It should keep the current PHP-5/7.0 behavior with new data structures. Thanks. Dmitry. > Hi, > > Please take a look into > https://gist.github.com/dstogov/43eaa3f3b68583877322 > > This is variation on Bob's idea about variable liveness. > The patch is incomplete. Liveness construction is still naive. However, > all tests are passed. > > Ranges are represented by start/end/variable cartages. (I suppose this > representation is more compact, than in the original proposal). > In general, it's possible to use few ranges for the same variable, if its > whole live range is not linear. (this is not implemented yet). > > Related ideas and tests are welcome. > > Thanks. Dmitry. --- ## #89146 — Nikita Popov — 2015-11-10T10:42:15+00:00 URL: https://externals.io/message/89143#89146 · In reply to: #89143 > Hi, > > Please take a look into > https://gist.github.com/dstogov/43eaa3f3b68583877322 > > This is variation on Bob's idea about variable liveness. > The patch is incomplete. Liveness construction is still naive. However, > all tests are passed. > > Ranges are represented by start/end/variable cartages. (I suppose this > representation is more compact, than in the original proposal). > In general, it's possible to use few ranges for the same variable, if its > whole live range is not linear. (this is not implemented yet). > > Related ideas and tests are welcome. > > Thanks. Dmitry. > One leak this does not cover yet is a discarded delayed return value: function test() { try { $a = [1, 2, 3]; return $a + []; } finally { throw new Exception; } } try { test(); } catch (Exception $e) {} This case is a bit tricky because it violates the usual invariant that all temporaries have well-defined liveness ranges (either alive or not, no maybe). We'd have to change that first. Nikita --- ## #89147 — Dmitry Stogov — 2015-11-10T11:29:57+00:00 URL: https://externals.io/message/89143#89147 · In reply to: #89146 > > > > […] > > One leak this does not cover yet is a discarded delayed return value: > > function test() { > try { > $a = [1, 2, 3]; > return $a + []; > } finally { > throw new Exception; > } > } > > try { > test(); > } catch (Exception $e) {} > > This case is a bit tricky because it violates the usual invariant that all > temporaries have well-defined liveness ranges (either alive or not, no > maybe). We'd have to change that first. > I see the problem. We can't statically define live-ranges for finally code, because we don't know where it's going to be called from. May be we may reconstruct this at run-time. Thanks. Dmitry, > > Nikita --- ## #89153 — Dmitry Stogov — 2015-11-10T19:28:30+00:00 URL: https://externals.io/message/89143#89153 · In reply to: #89147 I've committed unrelated and obvious parts (without semantic changes). The updated and extended patch now is much more clean https://gist.github.com/dstogov/43eaa3f3b68583877322 Anyway, it's still incomplete. Thanks. Dmitry. > > > On Tue, Nov 10, 2015 at 1:42 PM, Nikita Popov > wrote: > > > […] > > I see the problem. We can't statically define live-ranges for finally > code, because we don't know where it's going to be called from. > May be we may reconstruct this at run-time. > > Thanks. Dmitry, > > > > > […] --- ## #89169 — Xinchen Hui — 2015-11-11T03:10:15+00:00 URL: https://externals.io/message/89143#89169 · In reply to: #89147 Hey: > > > On Tue, Nov 10, 2015 at 1:42 PM, Nikita Popov > wrote: > > > […] > > I see the problem. We can't statically define live-ranges for finally > code, because we don't know where it's going to be called from. > May be we may reconstruct this at run-time. > Or maybe we could separate ZEND_RETURN(_BY_REF) into two parts the first part, set the return value to EX(return_value), the second part do the really leave(leave_helper). thus, we can keep the liveness of the variable won't be intrruptted? like: previously we emit: ZEND_ADD, ZEND_FAST_CALL, ZEND_RETURN. now we emit: ZEND_ADD, ZEND_RETURN, ZEND_FAST_CALL, ZEND_RETURN then in HANDLE_EXCEPTION we can dtor EX(return_value) if it is already set? this could also fixed double return memory leak like Zend/tests/try/bug70228.phpt thanks > > Thanks. Dmitry, > > > > > […] > -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #89170 — Xinchen Hui — 2015-11-11T03:11:02+00:00 URL: https://externals.io/message/89143#89170 · In reply to: #89169 > Hey: > > > > > […] > Or maybe we could separate ZEND_RETURN(_BY_REF) into two parts > > the first part, set the return value to EX(return_value), the second part > do the really leave(leave_helper). > > thus, we can keep the liveness of the variable won't be intrruptted? > > like: previously we emit: ZEND_ADD, ZEND_FAST_CALL, ZEND_RETURN. > > now we emit: ZEND_ADD, ZEND_RETURN, ZEND_FAST_CALL, ZEND_RETURN > s #ZEND_RETURN$#ZEND_LEAVE# thanks > > then in HANDLE_EXCEPTION we can dtor EX(return_value) if it is > already set? > > this could also fixed double return memory leak > like Zend/tests/try/bug70228.phpt > > thanks > > > […] > > > -- > Xinchen Hui > @Laruence > http://www.laruence.com/ > -- Xinchen Hui @Laruence http://www.laruence.com/ --- ## #89176 — Dmitry Stogov — 2015-11-11T13:19:25+00:00 URL: https://externals.io/message/89143#89176 · In reply to: #89153 It looks like the last version of the patch https://github.com/php/php-src/pull/1634 solves all the problems (except for reported by Nikita "unknown liveness in finally" - Zend/tests/temporary_cleaning_010.phpt). Please, analyse the patch and try to craft a script not covered by the patch. If nobody object and no problems found, I'll commit this on Friday. Thanks. Dmitry. > I've committed unrelated and obvious parts (without semantic changes). > The updated and extended patch now is much more clean > https://gist.github.com/dstogov/43eaa3f3b68583877322 > Anyway, it's still incomplete. > > Thanks. Dmitry. > > > > > […]