From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 66F4C6ECCC; Mon, 1 Aug 2022 13:25:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 66F4C6ECCC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1659349551; bh=Dek8ERdIhZgN5k2gTBeMN8IEBC31NHitMa98cCjyk1I=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=UibTVb8oxsN3WEpKdexWt+NylLZK6YNpZI53+IaD5erH6ZlD/CNswqzoUqYiiQOmL t20EMiXKeQ+lIhdWm5altTi/Zlf2ew96RaQ4c1kuq8oo2F1c7k4BnKNDV5ig8OKYN/ QmjpCxWq2/8hT22CKKoOsms9+vaWbWtMyQ+Ww0zw= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A969C6ECCC for ; Mon, 1 Aug 2022 13:25:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A969C6ECCC Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1oIScP-0004xr-Km; Mon, 01 Aug 2022 13:25:50 +0300 Date: Mon, 1 Aug 2022 13:23:22 +0300 To: sergos Message-ID: References: <54b6d24e217454e632c9a0e8dbf29dee1c37601a.1659264154.git.skaplun@tarantool.org> <452CFBEC-6B42-4049-94C7-B1899E008E61@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <452CFBEC-6B42-4049-94C7-B1899E008E61@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD96C5435BECA12D48D2A4C7BF469D974831DC20773FA0542E5182A05F5380850404C228DA9ACA6FE272CEAEBD4BACCCB142E2E5AFC6245084E54EE32108C7A0D2E709F67DDEE7761CE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7678D195E6F08FCA2EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063798FF8892961A0B3C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D84C9CFEF5FE63D02B306F4457B2E0463B117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC7F48962964D238D0A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735209ECD01F8117BC8BEA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CB0EC3B1FCAE4A06943847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0D4C601CEA03E70EE6791A85E4603CDE2F61E9D54AD1F48B683352AA2B0E32C4C9C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342C0B628602DFD0BCA25C99D2C118A809B75702CA292667AC698EDD1A45D248AF4355E37C2D2EDC6C1D7E09C32AA3244CD1659BE859982EF2C0D9CEFDF7A0E7163E8609A02908F271FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPX9Kc2YQ0o0FTtFjtaZ21Q== X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284EE6B545A19BFB7F5EE2A2E60A151D462E0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergos! Thanks for the review! On 01.08.22, sergos wrote: > Hi! > > Thanks for the patch! > > With some updates to the comments LGTM. > > Sergos > > > On 31 Jul 2022, at 13:58, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > (cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01) > > > > This commit changes usage of `lj_err_throw` to `lj_err_run()` in > use Fixed. > > > `lj_vm_exit_interp()` for not GC64 VMs, so the corresponding error > non-GC64 Fixed. > > > handler for `xpcall()` is called after trace exit. It allows to avoid > > calling of error handler, when restoration from a snapshot is failed due > an error handler call Fixed, thanks! > > > to the Lua stack overflow. This type of error can be handled by the > > compiler itself and user shouldn't worry/know about them. > The plural ’them’ needs an ‘Errors of this type’ in the beginning. Fixed, s/them/it/. > > > > > Also, this makes behaviour and code base for GC64 and not GC64 VMs > > consistent. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#7230 > > --- > > > > The test for this patch is very fragile: if the Lua stack layout is > > changed (for example smbd adds a one more parameter in `luaT_call()`) > > the test will fail as far as some error not during snapshot restoration > > will raised. So this particular test is skipped for the Tarantool. > > Also, it is skipped for *BSD as far as there are no traces compiled [1], > > so there is no restoration from a snapshot, so the errfunc is called. > > > > [1]: https://github.com/tarantool/tarantool/issues/4819 > > The new commit message is the following: | Call error function on rethrow after trace exit. | | (cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01) | | This commit changes use of `lj_err_throw` to `lj_err_run()` in the | `lj_vm_exit_interp()` for non-GC64 VMs, so the corresponding error | handler for `xpcall()` is called after trace exit. It allows to avoid | an error handler call, when restoration from a snapshot is failed due | to the Lua stack overflow. This type of error can be handled by the | compiler itself and user shouldn't worry/know about it. | | Also, this makes behaviour and code base for GC64 and not GC64 VMs | consistent. | | Sergey Kaplun: | * added the description and the test for the problem | | Part of tarantool/tarantool#7230 > > diff --git a/src/lj_dispatch.h b/src/lj_dispatch.h > > index 5bda51a2..addf5572 100644 > > --- a/src/lj_dispatch.h > > +++ b/src/lj_dispatch.h > > @@ -46,7 +46,7 @@ extern double __divdf3(double a, double b); > > _(asin) _(acos) _(atan) _(sinh) _(cosh) _(tanh) _(frexp) _(modf) _(atan2) \ > > _(pow) _(fmod) _(ldexp) _(lj_vm_modi) \ > > _(lj_dispatch_call) _(lj_dispatch_ins) _(lj_dispatch_stitch) \ > > - _(lj_dispatch_profile) _(lj_err_throw) \ > > + _(lj_dispatch_profile) _(lj_err_throw) _(lj_err_run) \ > > Wrong alpha sorting of function names? Yes, but this is consistent with upstream, so I left it as is. > > > _(lj_ffh_coroutine_wrap_err) _(lj_func_closeuv) _(lj_func_newL_gc) \ > > _(lj_gc_barrieruv) _(lj_gc_step) _(lj_gc_step_fixtop) _(lj_meta_arith) \ > > _(lj_meta_call) _(lj_meta_cat) _(lj_meta_comp) _(lj_meta_equal) \ > > diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > index 82ce6a8f..5cb487c3 100644 > > --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > @@ -4,11 +4,27 @@ local tap = require('tap') > > -- error is raised on restoration from the snapshot. > > -- See also https://github.com/LuaJIT/LuaJIT/issues/603. > > local test = tap.test('lj-603-err-snap-restore.test.lua') > > -test:plan(1) > > +test:plan(2) > > > > +-- XXX: This is fragile. We need a specific amount of Lua stack > > +-- slots is used to the error on restoration from a snapshot is > XXX ^ cause XXX > > > +-- raised and error handler isn't called according to the new > XXXXXXXX ^ without an ^^^^^^^^^^^ call > > > +-- behaviour. With another amount of used stack slots another one > Different ^ can raise > > +-- error may be raised (`LJ_ERR_STKOV` ("stack overflow") during > XXXXXXXXXXXX > > +-- growing stack while trying to push error message, > > +-- `LJ_ERR_ERRERR` ("error in error handling"), etc.). > > +-- This amount is suited well for GC64 and not GC64 mode. > > +-- luacheck: no unused > > +local _, _, _, _, _, _ > > + > > +local handler_is_called = false > > local recursive_f > > local function errfunc() > > xpcall(recursive_f, errfunc) > > + -- Since this error is occured on snapshot restoration and may > can be > > + -- handled by compiler itself, we shouldn't bother a user with > > + -- it. Fixed comments cosidering your suggestions: =================================================================== diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua index 3c55b090..e9f48ec9 100644 --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua @@ -7,12 +7,12 @@ local test = tap.test('lj-603-err-snap-restore.test.lua') test:plan(2) -- XXX: This is fragile. We need a specific amount of Lua stack --- slots is used to the error on restoration from a snapshot is --- raised and error handler isn't called according to the new --- behaviour. With another amount of used stack slots another one --- error may be raised (`LJ_ERR_STKOV` ("stack overflow") during --- growing stack while trying to push error message, --- `LJ_ERR_ERRERR` ("error in error handling"), etc.). +-- slots used to cause the error on restoration from a snapshot +-- and without error handler call according to the new behaviour. +-- Different amount of used stack slots can raise another one +-- error (`LJ_ERR_STKOV` ("stack overflow") during growing stack +-- while trying to push error message, `LJ_ERR_ERRERR` ("error in +-- error handling"), etc.). -- This amount is suited well for GC64 and not GC64 mode. -- luacheck: no unused local _, _, _, _, _, _ @@ -21,9 +21,9 @@ local handler_is_called = false local recursive_f local function errfunc() xpcall(recursive_f, errfunc) - -- Since this error is occured on snapshot restoration and may - -- handled by compiler itself, we shouldn't bother a user with - -- it. + -- Since this error is occured on snapshot restoration and can + -- be handled by compiler itself, we shouldn't bother a user + -- with it. handler_is_called = true end =================================================================== > > + handler_is_called = true > > end > > > > -- A recursive call to itself leads to trace with up-recursion. > > @@ -22,6 +38,11 @@ end > > recursive_f() > > > > test:ok(true) > > +-- Disabled on *BSD due to #4819. > > +-- XXX: The different amount of stack slots is in-use for > > +-- Tarantool at start, so just skip test for it. > > +-- luacheck: no global > > +test:ok(jit.os == 'BSD' or _TARANTOOL or not handler_is_called) > > > > -- XXX: Don't use `os.exit()` here intense. When error on snap > ??? just drop? Fixed in the previous commit. (Not dropped, I suppose this is some problem with mail client.) > > -- restoration is raised, `err_unwind()` doesn't stop on correct > > unfinished phrase? This is just the old comment. > > > -- > > 2.34.1 > > > -- Best regards, Sergey Kaplun