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 673F8580C95; Wed, 16 Aug 2023 16:51:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 673F8580C95 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692193885; bh=2OyXUYyo751S5GYvHRhgqsHGXBjxt2q2NUx/9ErF24s=; 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=HfHHeufSfUyxZUrRCid4897lZZZR6h7icDCLPU3VhQHvEui7pQkdBerYZMoJAG69x g9s2Go/hlvp6ePcF/g52J/mMyaq+vvylYmUA5QadXqIh88Z0fFmbTJkaYADHQCY6HX vHmvWH8aFfJMe0q9h0NiDPYlPld0xJv5PuP08grE= Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [95.163.41.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 54216532601 for ; Wed, 16 Aug 2023 16:51:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 54216532601 Received: by smtp33.i.mail.ru with esmtpa (envelope-from ) id 1qWGvi-00Elkc-32; Wed, 16 Aug 2023 16:51:23 +0300 Date: Wed, 16 Aug 2023 16:51:21 +0300 To: Sergey Kaplun Message-ID: References: <20230815123215.510-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230815123215.510-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754D12A1D0741C7E82ED7F70A80435AA08BE182A05F53808504028C360AD9F421CDC80733C4D300D01A2227F1D6C00AC3B27E01DFABD1AAAA654 X-C1DE0DAB: 0D63561A33F958A580C7DFE44B8DB5A0176A1DE40F182EBF9FBFE7CF7BD4C5EBF87CCE6106E1FC07E67D4AC08A07B9B013BDA61BF53F5E1DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527F8B16105F904548CB2F3ABBA0E5D2F7CF5068136D435B2CE09FE3A7E33566D7DA6527C57D964398CF37FD76D11AF80A0D06906B28EB14057AE787FDC7D7B129C6EA455F16B58544A2557BDE0DD54B3590965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHVl7ekwB6hglo4Q3LoLxsg== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE40778EDB8BB120A8892D718813735F43DBE327D77830F209D78D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_VARG, part 3. 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. On Tue, Aug 15, 2023 at 03:32:15PM +0300, Sergey Kaplun wrote: > From: Mike Pall > > Thanks to Peter Cawley. > > (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32) > > In case, when `BC_VARG` set the VARG slot to the non-top stack slot, Typo: s/set/sets/ > `maxslot` value was unconditionally set to the destination slot, so some > top slots may be omitted in the snapshot entry. Since these slots are > omitted, they are not restored correctly, when restoring from snapshot Typo: s/snapshot/a snapshot/ > for this side exit. > > This patch adds the check for the aforementioned case, to avoid maxslot > shrinking. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording > PR: https://github.com/tarantool/tarantool/pull/8986 > Related issues: > * https://github.com/LuaJIT/LuaJIT/issues/1046 > * https://github.com/tarantool/tarantool/issues/8825 > > src/lj_record.c | 12 +++- > .../lj-1046-fix-bc-varg-recording.test.lua | 58 +++++++++++++++++++ > 2 files changed, 67 insertions(+), 3 deletions(-) > create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua > > diff --git a/src/lj_record.c b/src/lj_record.c > index 34d1210a..6bcdb04c 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -1807,8 +1807,12 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults) > if (J->framedepth > 0) { /* Simple case: varargs defined on-trace. */ > ptrdiff_t i; > if (nvararg < 0) nvararg = 0; > - if (nresults == -1) nresults = nvararg; > - J->maxslot = dst + (BCReg)nresults; > + if (nresults != 1) { > + if (nresults == -1) nresults = nvararg; > + J->maxslot = dst + (BCReg)nresults; > + } else if (dst >= J->maxslot) { > + J->maxslot = dst + 1; > + } > for (i = 0; i < nresults; i++) > J->base[dst+i] = i < nvararg ? getslot(J, i - nvararg - 1 - LJ_FR2) : TREF_NIL; > } else { /* Unknown number of varargs passed to trace. */ > @@ -1840,7 +1844,9 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults) > } > for (i = nvararg; i < nresults; i++) > J->base[dst+i] = TREF_NIL; > - J->maxslot = dst + (BCReg)nresults; > + if (nresults != 1 || dst >= J->maxslot) { > + J->maxslot = dst + (BCReg)nresults; > + } > } else if (select_detect(J)) { /* y = select(x, ...) */ > TRef tridx = J->base[dst-1]; > TRef tr = TREF_NIL; > diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua > new file mode 100644 > index 00000000..34c5c572 > --- /dev/null > +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua > @@ -0,0 +1,58 @@ > +local tap = require('tap') > +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(2) > + > +jit.opt.start('hotloop=1') > + > +-- luacheck: ignore > +local anchor > +local N_ITER = 5 > +local SIDE_ITER = N_ITER - 1 > +for i = 1, N_ITER do > + -- In case, when `BC_VARG` set the VARG slot to the non-top > + -- stack slot, `maxslot` value was unconditionally set to the > + -- destination slot, so the following snapshot is used: > + -- SNAP #4 [ ---- ---- ---- nil ] > + -- instead of: > + -- SNAP #4 [ ---- nil ---- ---- 0009 0001 ---- 0009 ] Snapshot examples here ceratinly give the idea of what goes wrong, but the `0009` and `0001` are meaningless by themselves. I think it would be nice to include IRs here too. > + -- Since these slots are omitted, they are not restored > + -- correctly, when restoring from snapshot for this side exit. Please fix the same typos as in the commit message here. > + anchor = ... > + if i > SIDE_ITER then > + -- XXX: Don't use `test:ok()` here to avoid double-running of I think better phrasing would be: | `test:ok()` is not used here ... > + -- tests in case of `i` incorrect restoring from the snapshot. Typo: s/restoring/restoration/ > + assert(i > SIDE_ITER) > + end > +end > + > +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result') > + > +-- Now the same case, but with an additional frame, so VARG slots > +-- are defined on the trace. > +local function varg_frame(anchor, i, side_iter, ...) > + anchor = ... > + -- In case, when `BC_VARG` set the VARG slot to the non-top > + -- stack slot, `maxslot` value was unconditionally set to the > + -- destination slot, so the following snapshot is used: > + -- SNAP #4 [ | nil nil nil `varg_frame` | nil ] > + -- instead of: > + -- SNAP #4 [ | nil nil nil `varg_frame` | nil 0009 0005 ] > + -- Since these slots are omitted, they are not restored > + -- correctly, when restoring from snapshot for this side exit. I guess we don't need to repeat the entire comment again. > + if i > side_iter then > + -- XXX: Don't use `test:ok()` here to avoid double-running of > + -- tests in case of `i` incorrect restoring from the snapshot. Same typos as in the comment above. > + assert(i > side_iter) > + end > +end > + > +for i = 1, N_ITER do > + varg_frame(nil, i, SIDE_ITER) > +end > + > +test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result') > + > +test:done(true) > -- > 2.41.0 >