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 250825992AB; Mon, 21 Aug 2023 16:38:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 250825992AB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692625123; bh=6wa0kYvKMGyz87WiRQU86+5s9dWc8Nc4UmwRBL+QQyc=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=uZekp0l1/DpXvEoZ0/dQw+3BPgKGvtcWYq9xgv+nFppib1sC5xdU30BJRhKRFLZ76 XF2enNqhub3NulV5viWy67L8Y2XDH61OBo/PJmKYqM926q+Yk+koV730ouna74pbzq jzYibtPfAufzsf7eU/3md8pyjKDZZCpXaa+H8L4E= Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [95.163.41.98]) (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 0A201596289 for ; Mon, 21 Aug 2023 16:38:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0A201596289 Received: by smtp60.i.mail.ru with esmtpa (envelope-from ) id 1qY57A-001wEV-39; Mon, 21 Aug 2023 16:38:41 +0300 Message-ID: <27744ffa-a0f8-56dc-15f7-3c367b68a7f8@tarantool.org> Date: Mon, 21 Aug 2023 16:38:40 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20230815123215.510-1-skaplun@tarantool.org> In-Reply-To: <20230815123215.510-1-skaplun@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD93C8852532D76B9E3B40810D8949158306E0746822E25C650182A05F5380850401C1FCA0D06BEB2B17E1D16FC79C7770678392CD62C05E38528A787FFD79C73E3 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77BF46084C0059042EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374FA524FBD8B89EAA8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F5317D592548CA17A6C468734CC2D731117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCDBA3F3F673D6AB81A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18E5D25F19253116ADD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE26055571C92BF10F4DB3626BA78294CCD8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32D01283D1ACF37BA302FCEF25BFAB345C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C1D471462564A2E192E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F890A246B268E114E57739F23D657EF2BB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5ADC7F17A3336AFA411E2920FD588F62DE82865275326A57DF87CCE6106E1FC07E67D4AC08A07B9B02A336C6518635091CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF54DAD115D556E56A068A95ABF187C16DBA8CBE722AA6162D737C463FFE508B80AE7801A017AB3E4AA7CA3A83B0F0804D60B7CD0F09930DA2C08E7933AEDF3DE7E48CAC7CA610320002C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojHJI2DMjVra0ngLI94xIXpg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76951F410472C7EB82D7E1D16FC79C777066CCF71F689FD47EEEBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey thanks for the patch! See comment inline. On 8/15/23 15:32, 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, > `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 > 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 ] > + -- Since these slots are omitted, they are not restored > + -- correctly, when restoring from 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. > + assert(i > SIDE_ITER) > + end > +end > + > +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result') This always passed testcase is confused and it will confuse everyone who will use test. I propose to replace test:ok() to test:diag: 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 30a87e54..32566c54 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 @@ -3,7 +3,7 @@ local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({    ['Test requires JIT enabled'] = not jit.status(),  }) -test:plan(2) +test:plan(0)  jit.opt.start('hotloop=1') @@ -49,7 +49,7 @@ for i = 1, N_ITER do    end  end -test:ok(true, 'BC_VARG recording 0th frame depth, 1 result') +test:diag('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. @@ -67,6 +67,6 @@ 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:diag('BC_VARG recording with VARG slots defined on trace, 1 result')  test:done(true) or you can define a global variable and replace assert with assigning result to this variable. then check expected and actual value for global variable using test:ok(). something like this:  test:ok(true, 'BC_VARG recording 0th frame depth, 1 result') +local varg_recorded = false +  -- 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 = ... -  if i > side_iter then -    -- 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 +  varg_recorded = i > side_iter  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:ok(varg_recorded, 'BC_VARG recording with VARG slots defined on trace, 1 result')  test:done(true) > + > +-- 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. > + 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. > + 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)