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 65D0458123F; Wed, 16 Aug 2023 19:16:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 65D0458123F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692202606; bh=Y2ZBvl4se6kOZUdFvkbqLRmcJikawAyGhqzRkbcZibc=; 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=X5ToODH4/OVxc+r2NhoD0Wp8tXESAJXaIZJZOL8cZT12PKZT2Eq1Ogqry+YQ2I9dX RRQ7NvGZ0Gj7CCdUfqVjelbNkvc7qZp47F4I0fVQtCg1G12GjhYweUGidJMXCRGdbZ BzcAzRXbL0hEYJM2TVwZrW8eCE48ADXnebRC7ErQ= 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 4229558123F for ; Wed, 16 Aug 2023 19:16:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4229558123F Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qWJCO-0005B2-CP; Wed, 16 Aug 2023 19:16:44 +0300 Date: Wed, 16 Aug 2023 19:11:57 +0300 To: Maxim Kokryashkin 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: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9700E0DCE2907754D9B1C46D604AD036F8866CBFA63B460F7182A05F53808504054D3A690AEFDB3307A49A90A766E9D2CDC5DED6C515F94A83F9D1A35815C049E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922E451CE6E839B1EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A0569EA9A35E44F48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81D94D49E030F8DFB79129106E153EFF6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCCF01C05423B8DB1BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F7900637B6C899C330ACEAEA22CA9DD8327EE4930A3850AC1BE2E735D028CC0B556B22BCC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A536BC1E0DCB2EC76D9176329AB841D8B23E2B04836DFB8CA0F87CCE6106E1FC07E67D4AC08A07B9B02A336C6518635091CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF3EFC31E9C74F75A843EDD9368791272DCB8A7CE8883EB8077227D3E1AA021519744A43F78FC6FA9024F7434E9608BF9FB35067FB8366DF78F6A3497AF5AEA8ADE48CAC7CA610320002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojFRrmMqSMPxqBoymkuy8c0w== X-DA7885C5: 1357380F26914743793BE7F7463A838362A33A02590ADF9177090E9D770DE73D262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B8F0FFFC0F267E415268C9B1983E324A90FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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: 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, 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