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 7F9305C2841; Thu, 17 Aug 2023 16:16:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7F9305C2841 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692278198; bh=Hmm3z/L0VEy4AizL+BuVUAfzhTz3+Dsw+F4f6e+LILI=; 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=UFj33DvHVGlMjCPIkfSH7MTu9IAwhciemz3R6zkwui2gKIHPKqbCynjrLs3DD99wI 3EWTjxZuTXZEpuCEgnVVCKX2yYVFiLA9+C//XjH4C2e77E1HWhNDEGPd/6QqgOW2SC T5KuC3EH7/NZbN7aEX95uZ93+4utJvTN3y6gga3w= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [95.163.41.100]) (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 9CBDC5C0B7B for ; Thu, 17 Aug 2023 16:16:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9CBDC5C0B7B Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1qWcrc-007mZ2-1X; Thu, 17 Aug 2023 16:16:36 +0300 Date: Thu, 17 Aug 2023 16:16:36 +0300 To: Sergey Kaplun Message-ID: <7jfgk5ivjxi5zrvf3kc6vhne74glp3ysi3sknkx42nzk2cgq4j@7krmaw2frlap> References: <20230815123215.510-1-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-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754D77D6FA938422CA5F653AC3988EEC6B12182A05F5380850404FC35C2EFE89AF3DA147B5CAD3E3B9B1D35497ECA4B8B8DAEB5AE8C75DEAB154 X-C1DE0DAB: 0D63561A33F958A50BA0F68226B12097617EA963BDA5E89058D8DE786FBC178DF87CCE6106E1FC07E67D4AC08A07B9B02A336C6518635091CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742DC8270968E61249B1004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D347130F804358653A681EA9A8E34C7DDE01493A8FA3D13D60A271362A9B5435CA00F47CD794D583FD61D7E09C32AA3244C8CC75DD39EA0B531C583BDABC733B10F30452B15D76AEC1485A42E4C463514DC5DA084F8E80FEBD396F07DFE06A4A8314E894E437E78228B66933FA05BD8EF0CAD958392AE682691 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojFRrmMqSMPxqR3D8e/QifzA== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407EED5534759EECFABCF8D3D8FB4DAF21EE29161680CB82618D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 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 fixes! LGTM On Wed, Aug 16, 2023 at 07:11:57PM +0300, Sergey Kaplun wrote: > Hi, Maxim! > Thanks for the review! > Fixed your comments and force-pushed the branch. > > On 16.08.23, Maxim Kokryashkin wrote: > > 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/ > > Fixed. > > > > `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/ > > Fixed. > > > > 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 > > > > > > 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. > > Added. > > > > + -- 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. > > Fixed. > > > > + 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 ... > > Fixed. > > > > + -- tests in case of `i` incorrect restoring from the snapshot. > > Typo: s/restoring/restoration/ > > Fixed. > > > > + 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. > > Dropped then. > > > > + 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. > > Fixed. > > > > + assert(i > side_iter) > > See the iterative patch below. Branch is force-pushed. > > =================================================================== > 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 > index 34c5c572..30a87e54 100644 > --- a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua > +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua > @@ -12,18 +12,39 @@ 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 > + -- This trace generates the following IRs: > + -- 0001 > int SLOAD #7 CRI > + -- 0002 > int LE 0001 +2147483646 > + -- 0003 int SLOAD #6 CI > + -- 0004 int SLOAD #0 FR > + -- 0005 > int LE 0004 +11 > + -- 0006 > num SLOAD #5 T > + -- 0007 num CONV 0003 num.int > + -- .... SNAP #1 [ ---- ---- ---- nil ] > + -- 0008 > num ULE 0007 0006 > + -- 0009 + int ADD 0003 +1 > + -- .... SNAP #2 [ ---- ---- ---- nil ---- ---- ] > + -- 0010 > int LE 0009 0001 > + -- .... SNAP #3 [ ---- ---- ---- nil ---- ---- 0009 0001 ---- 0009 ] > + -- 0011 ------ LOOP ------------ > + -- 0012 num CONV 0009 num.int > + -- .... SNAP #4 [ ---- ---- ---- nil ] > + -- > + -- In case, when `BC_VARG` sets 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 ] > + -- destination slot, so the following snapshot (same for the #1) > + -- is used: > + -- .... SNAP #4 [ ---- ---- ---- nil ] > -- instead of: > - -- SNAP #4 [ ---- nil ---- ---- 0009 0001 ---- 0009 ] > + -- .... SNAP #4 [ ---- ---- ---- nil ---- ---- 0009 0001 ---- 0009 ] > -- Since these slots are omitted, they are not restored > - -- correctly, when restoring from snapshot for this side exit. > + -- correctly, when restoring from the snapshot for this side > + -- exit. > anchor = ... > 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. > + -- XXX: `test:ok()` isn't used here to avoid double-running of > + -- tests in case of `i` incorrect restoration from the > + -- snapshot. > assert(i > SIDE_ITER) > end > end > @@ -34,17 +55,10 @@ test:ok(true, 'BC_VARG recording 0th frame depth, 1 result') > -- 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. > 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. > + -- XXX: `test:ok()` isn't used here to avoid double-running of > + -- tests in case of `i` incorrect restoration from the > + -- snapshot. > assert(i > side_iter) > end > end > =================================================================== > > > > > > > > -- > Best regards, > Sergey Kaplun