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 743E096F96C; Tue, 16 Jan 2024 14:54:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 743E096F96C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1705406053; bh=EhONuZOC6EVs9XTINtxFaRLx2Qdot0tsJqnI8V0+6ww=; 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=LOf1ILkAILah5KzWKZd7hH5nUUhp/19GqwD9meTDFDAnoBqC7bhy96SmW12w79yOp EXlYmRk28EajWCv+IOYURR/WZF0+gZbeE8NmrV7hL5yKOAvPW0MlxC66g19q9Glp0v dZqYL5SGARTk+B+iQMn28ZsFFd/8ygov+Xs7g+Jk= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (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 337125650A5 for ; Tue, 16 Jan 2024 14:54:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 337125650A5 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1rPi1B-00F9F3-3B; Tue, 16 Jan 2024 14:54:10 +0300 Message-ID: <2521609b-69c9-4b45-b354-f3bd8c68be72@tarantool.org> Date: Tue, 16 Jan 2024 14:54:09 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20231211153520.9322-1-skaplun@tarantool.org> In-Reply-To: <20231211153520.9322-1-skaplun@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AE5B4AFB3AE2A590E10A57BB36FAC7D4F681D642268DCBB5182A05F5380850403EC7A423764415AEE1C83B306908B4B26C92F944DC9426317DCDC9E3011F3988 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CFDA40FA2326E319EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375E280A1EC162AD7D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BA812F99613A237525F733F738523C63117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC974A882099E279BDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE26055571C92BF10F7B089FF177BE8049D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE32D01283D1ACF37BAAD7EC71F1DB88427C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C1D471462564A2E192E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F9FFED5BD9FB417556D8C47C27EEC5E9FB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A52BF93FA2FBB531F169E4523AD59D60C31AB777EC901EB9E0F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF89042B6504AFD313967339EB9C52D67E577495A4FF35B3DC9359B43514415AB4ECCBE4C437D6579C151C858F58AF32476448C5F9F94C96978574F4BB5CCAA9C0E48CAC7CA610320002C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuGdDMIdVs4KjFzchzR7ZZg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7697AFA6B3B0141A5C7E1C83B306908B4B284876E3DC5E244DFEBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay. 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" Sergey, thanks for the patch! LGTM On 12/11/23 18:35, Sergey Kaplun wrote: > From: Mike Pall > > Thanks to Sergey Kaplun and Peter Cawley. > > (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e) > > Assume we have the parent trace with the following IRs: > > | 0001 } tab TNEW #0 #0 > | 0002 } p32 NEWREF 0001 "key" > | 0003 } fal HSTORE 0002 false > | .... SNAP #1 [ ---- ---- 0001 ---- ] > | 0004 > num SLOAD #1 T > | .... SNAP #2 [ ---- ---- 0001 ] > | 0005 > num EQ 0004 0004 > | 0006 } tru HSTORE 0002 true > | .... SNAP #3 [ ---- ---- 0001 true ] > > The side trace for the third snapshot emits the following IRs: > > | 0001 tab TNEW #0 #0 > | 0002 p32 NEWREF 0001 "key" > | 0003 fal HSTORE 0002 false > | 0004 p32 NEWREF 0001 "key" > | 0005 tru HSTORE 0004 true > > As we can see, `NEWREF` is emitted twice. This is a violation of its > semantics, so the second store isn't noticeable. > > This patch prevents the second emitting of IR NEWREF by checking the last > one emitted NEWREF IR. There is no need to check NEWREFs beyond since > it guarantees the snapshot is taken after it, because it may cause table > rehashing, so all prior results are invalidated. > > Sergey Kaplun: > * added the description and the test for the problem > > Resolves tarantool/tarantool#7937 > Part of tarantool/tarantool#9145 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1128-double-ir-newref-on-restore-sunk > PR: https://github.com/tarantool/tarantool/pull/9466 > Related issues: > * https://github.com/LuaJIT/LuaJIT/issues/1128 > * https://github.com/tarantool/tarantool/issues/7937 > > src/lj_snap.c | 16 ++++ > ...-double-ir-newref-on-restore-sunk.test.lua | 81 +++++++++++++++++++ > 2 files changed, 97 insertions(+) > create mode 100644 test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua > > diff --git a/src/lj_snap.c b/src/lj_snap.c > index 3f0fccec..73e18e69 100644 > --- a/src/lj_snap.c > +++ b/src/lj_snap.c > @@ -583,9 +583,25 @@ void lj_snap_replay(jit_State *J, GCtrace *T) > if (irr->o == IR_HREFK || irr->o == IR_AREF) { > IRIns *irf = &T->ir[irr->op1]; > tmp = emitir(irf->ot, tmp, irf->op2); > + } else if (irr->o == IR_NEWREF) { > + IRRef allocref = tref_ref(tr); > + IRRef keyref = tref_ref(key); > + IRRef newref_ref = J->chain[IR_NEWREF]; > + IRIns *newref = &J->cur.ir[newref_ref]; > + lj_assertJ(irref_isk(keyref), > + "sunk store for parent IR %04d with bad key %04d", > + refp - REF_BIAS, keyref - REF_BIAS); > + if (newref_ref > allocref && newref->op2 == keyref) { > + lj_assertJ(newref->op1 == allocref, > + "sunk store for parent IR %04d with bad tab %04d", > + refp - REF_BIAS, allocref - REF_BIAS); > + tmp = newref_ref; > + goto skip_newref; > + } > } > } > tmp = emitir(irr->ot, tmp, key); > + skip_newref: > val = snap_pref(J, T, map, nent, seen, irs->op2); > if (val == 0) { > IRIns *irc = &T->ir[irs->op2]; > diff --git a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua > new file mode 100644 > index 00000000..a89beab6 > --- /dev/null > +++ b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua > @@ -0,0 +1,81 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT incorrect restoring of sunk > +-- tables with double usage of IR_NEWREF. > +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1128. > + > +local test = tap.test('lj-1128-double-ir-newref-on-restore-sunk'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(3) > + > +local take_side > + > +local function trace_base(num) > + local tab = {} > + tab.key = false > + -- This check can't be folded since `num` can be NaN. > + tab.key = num == num > + -- luacheck: ignore > + -- This side trace emits the following IRs: > + -- 0001 tab TNEW #0 #0 > + -- 0002 p64 NEWREF 0001 "key" > + -- 0003 fal HSTORE 0002 false > + -- 0004 p64 NEWREF 0001 "key" > + -- 0005 tru HSTORE 0004 true > + -- As we can see, `NEWREF` is emitted twice. This is a violation > + -- of its semantics, so the second store isn't noticeable. > + if take_side then end > + return tab.key > +end > + > +-- Uncompiled function to end up side trace here. > +local function trace_base_wp(num) > + return trace_base(num) > +end > +jit.off(trace_base_wp) > + > +-- Same function as above, but with two IRs NEWREF emitted. > +local function trace_2newref(num) > + local tab = {} > + tab.key = false > + -- This + op can't be folded since `num` can be -0. > + tab.key = num + 0 > + tab.key2 = false > + -- This check can't be folded since `num` can be NaN. > + tab.key2 = num == num > + -- luacheck: ignore > + if take_side then end > + return tab.key, tab.key2 > +end > + > +-- Uncompiled function to end up side trace here. > +local function trace_2newref_wp(num) > + return trace_2newref(num) > +end > +jit.off(trace_2newref_wp) > + > +jit.opt.start('hotloop=1', 'hotexit=1', 'tryside=1') > + > +-- Compile parent traces. > +trace_base_wp(0) > +trace_base_wp(0) > +trace_2newref_wp(0) > +trace_2newref_wp(0) > + > +-- Compile side traces. > +take_side = true > +trace_base_wp(0) > +trace_base_wp(0) > +trace_2newref_wp(0) > +trace_2newref_wp(0) > + > +test:is(trace_base(0), true, 'sunk value restored correctly') > + > +local arg = 0 > +local r1, r2 = trace_2newref(arg) > +test:is(r1, arg, 'sunk value restored correctly with 2 keys, first key') > +test:is(r2, true, 'sunk value restored correctly with 2 keys, second key') > + > +test:done(true)