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 2C8596ECCC; Mon, 1 Aug 2022 13:07:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2C8596ECCC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1659348446; bh=j78UPUwiOBfYMHN2L9My3nW4VPVu4m7LgLP/Q650xQ8=; 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=r9IahP/GnaYNZOlXKL0SCO6sYP/PbnPTvBZ9bhNeh1CSGD069Zia1vWNX8VU2NNSr egtXpJlnIno/mT8V84EjTZRD506Ntt+UF8gNo1y553JMCyHL3MvsJibuJpRxbe9EJ8 LtOjF0vMi85YAcYH2DQp4yJDxI8fvgjW3mhA0lJE= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 5E7856ECCC for ; Mon, 1 Aug 2022 13:07:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5E7856ECCC Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1oISKa-0002YO-Dg; Mon, 01 Aug 2022 13:07:24 +0300 Date: Mon, 1 Aug 2022 13:04:57 +0300 To: sergos Message-ID: References: <2409b71740006f7ea89e8f360ea77f68de7be1d5.1659264154.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9626C4810127D410702BFDBFE214C589B9B69B3DADAEB9BCA182A05F538085040110B1E5825B2852EF4861F2B9B23D2DE35863A86CDE7DB25E9544BC53203A238 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76D34FAA3D8B31588C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7CB1634DB9A2F7B99EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B8859CA687ABA27BAA66F8413B3D9AAB4A336199FC008F7BFCC7F00164DA146DAFE8445B8C89999728AA50765F790063753733A85D94CE7A19FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637BC468E7E89D8C5D6EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0F56C66D0505D98346791A85E4603CDE2E378CEE356019CD7325841A1AF5981749C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3498910055B812BD9C3AA69B09AEB815BBED6022A241DD0EE6E2B11241CB3E55D2BB007AACCFC40AE41D7E09C32AA3244C0A9C3310F3D804E985704CA6656A3E8E7C0C08F7987826B9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPX9Kc2YQ0o1qLMreBGJA7g== X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284E20BFDAB045413DAB348F57DADBB5335E0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore. 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 comments fixes LGTM. > > Sergos > > > > On 31 Jul 2022, at 13:58, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > (cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8) > > > > When an error is raised during snapshot restore, `err_unwind()` skipped the > the the > > correct cframe to stop unwinding. It happens due this frame is C frame > that should stop The reason is a > > without Lua frame and the special negative value of `cfram_nres()` for > ^^^^^^^ ^e > not sure if I got it right - wrapping frame? Anyways an article is missing > > > this frame isn't set. > > > > This patch sets `cframe_nres()` for cframe with snap restoration to > the a that contains a > > `-2*LUAI_MAXSTACK` to guarantee that an error will be always caught > > here. > Not quite clear why this should be done always, since you mentioned before the > Lua frame presence mitigates the problem. Does it mean the cframe_nres() is > ignored if Lua frame is present? Mention it if it is true. Sorry, my bad wording. I've meant that this is just pseudo frame without true honestly frame set up like it is done for `lua_cpcall()`. I've reformulated the commit message as the following, considerring your comments: | Fix handling of errors during snapshot restore. | | (cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8) | | When an error is raised during a snapshot restore, the `err_unwind()` | skips the correct cframe that should stop unwinding. The reason is this | frame is the pseudo C frame for error handling without an actual frame | and the special negative value of `cframe_nres()` for this frame isn't | set. | | This patch sets `cframe_nres()` for a cframe that contains a snap | restoration to `-2*LUAI_MAXSTACK` to guarantee that an error will be | always caught here. | | Sergey Kaplun: | * added the description and the test for the problem | | Part of tarantool/tarantool#7230 > > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#7230 > > --- > > src/lj_trace.c | 2 ++ > > .../lj-603-err-snap-restore.test.lua | 30 +++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua > > > > diff --git a/src/lj_trace.c b/src/lj_trace.c > > index d7a78d4d..68a657a7 100644 > > --- a/src/lj_trace.c > > +++ b/src/lj_trace.c > > @@ -803,6 +803,8 @@ static TValue *trace_exit_cp(lua_State *L, lua_CFunction dummy, void *ud) > > { > > ExitDataCP *exd = (ExitDataCP *)ud; > > cframe_errfunc(L->cframe) = -1; /* Inherit error function. */ > > + /* Always catch error here. */ > > + cframe_nres(L->cframe) = -2*LUAI_MAXSTACK*(int)sizeof(TValue); > > exd->pc = lj_snap_restore(exd->J, exd->exptr); > > UNUSED(dummy); > > return NULL; > > diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > new file mode 100644 > > index 00000000..82ce6a8f > > --- /dev/null > > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > > @@ -0,0 +1,30 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate the incorrect JIT behaviour when an > ??? > > > +-- error is raised on restoration from the snapshot. > > +-- See also https://github.com/LuaJIT/LuaJIT/issues/603. > > + > > +test:ok(true) > > + > > +-- XXX: Don't use `os.exit()` here intense. When error on snap > ^^^ explicitly? | by intention? > > > +-- restoration is raised, `err_unwind()` doesn't stop on correct > > +-- cframe. So later, on exit from VM this corrupted cframe chain > > +-- shows itself. `os.exit()` literally calls `exit()` and doesn't > > +-- show the issue. Fixed. See the iterative patch below. =================================================================== 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..d20c4d6a 100644 --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua @@ -1,7 +1,7 @@ local tap = require('tap') --- Test file to demonstrate the incorrect JIT behaviour when an --- error is raised on restoration from the snapshot. +-- Test to demonstrate the incorrect JIT behaviour when an 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) @@ -23,8 +23,8 @@ recursive_f() test:ok(true) --- XXX: Don't use `os.exit()` here intense. When error on snap --- restoration is raised, `err_unwind()` doesn't stop on correct --- cframe. So later, on exit from VM this corrupted cframe chain --- shows itself. `os.exit()` literally calls `exit()` and doesn't --- show the issue. +-- XXX: Don't use `os.exit()` here by intention. When error on +-- snap restoration is raised, `err_unwind()` doesn't stop on +-- correct cframe. So later, on exit from VM this corrupted cframe +-- chain shows itself. `os.exit()` literally calls `exit()` and +-- doesn't show the issue. =================================================================== > > -- > > 2.34.1 > > > -- Best regards, Sergey Kaplun