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 1FB1D72F6C1; Tue, 12 Dec 2023 12:44:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1FB1D72F6C1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1702374259; bh=oE/R2e5TrBebGDYdyZpHMO7G4mHj5rsxrue3MIsgxfw=; 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=a/3dQRQS1ud5UlIbHNFdMBqK5IY9Tut6fh71C0ZrIv/Zsg6ixxrDnW7CwsfVS2mnm rYfsX7iItJLob9r1/22tGgoA7adKJAuq9x8u1746LFJG3cAsh58zFBmpLwPo6AhDAh Q17yzMy30CCcRaURXGGjRic/0hikb5raDhLMaQK4= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [95.163.41.97]) (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 EEAD472F6C1 for ; Tue, 12 Dec 2023 12:44:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EEAD472F6C1 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1rCzJI-00CZA7-30; Tue, 12 Dec 2023 12:44:17 +0300 Date: Tue, 12 Dec 2023 12:44:16 +0300 To: Sergey Kaplun Message-ID: References: <20231211153520.9322-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231211153520.9322-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD97533543916A0F71AD286D97BFFE3DA5DB7E67E5414E74A44CD62213F67905E7AC27A61541CFDA0A171B4AFCCAB29DDB002A7E298E5A01895854E8B091AFA3944 X-C1DE0DAB: 0D63561A33F958A503417C85B2216D913688C2785FEA45FC8B81E981850A5325F87CCE6106E1FC07E67D4AC08A07B9B0735DFC8FA7AC1207CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FB146342DF761F756FA45AE6A00A315B4D00EBB16218D1B9E6564EAD4B50008A1E07D9F87F87EFA2637FD76D11AF80A0DB4716D79DD0BAACB0A0021011367A01DEA455F16B58544A2557BDE0DD54B3590965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojm/AwjadTR3QtkQFUfku7oQ== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4072BE1878F32EA27640BA2194DF72422C172D6B4FCE48DF648D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 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: 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 Mon, Dec 11, 2023 at 06:35:20PM +0300, Sergey Kaplun wrote: > From: Mike Pall > > Thanks to Sergey Kaplun and Peter Cawley. > > (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e) > 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) > 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. Please mention that this test cases checks situation when last NEWREF is not the same. > +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 Nit: `key` and `key2` naming seems a bit inconsistent. > + > +-- 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') These assertions pass before the patch. Is this expected behavior? If so, please drop a comment. > + > +test:done(true) > -- > 2.43.0 >