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 D493C4369ED; Tue, 16 May 2023 15:17:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D493C4369ED DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684239441; bh=XsnMcG5dWK2SpvZhRLC7LYIyNi9dope1rQAYxfj4ABk=; 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=dPxIKUjTNRbrlqeD+iUqqws9OoUHpnB4J7xtjFmgmljKI8v/TC6X4iUOM9Bm/Eeu2 doto/uaXnDxiAyFAxqCz7/hWqlcMQGk7MKkYJ3byGE5XH6sH9uK6SigMnJrbvWq2I4 pq4Vu6TPSk+S/gxZXdNZVeWSiB/68adjlKvnKDq8= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 7F82C4369ED for ; Tue, 16 May 2023 15:17:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7F82C4369ED Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1pytcF-00ADad-FR; Tue, 16 May 2023 15:17:19 +0300 Message-ID: <955c07a0-84f2-4598-7bc2-d7c1289546a8@tarantool.org> Date: Tue, 16 May 2023 15:17:19 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: In-Reply-To: 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: 4F1203BC0FB41BD92B2351A05AA37E8C9D8A6526EF3E652374DF841EC52E5C7700894C459B0CD1B946701F7D909D5E5C4237316E01189AC9A7530E36AF9AAA984F5BE5F2D6B2B162 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE768BD42809A772657EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A900FA3C0C04B8698638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88BFA800C9F1920ED48EBDC9D0E482221117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC7F48962964D238D0A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACD2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE41BF15D38FB6CB3AE2071C6999E77799D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3E3786DD2C77EBDAA040F9FF01DFDA4A8C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637C69D4C8FA58DAC3AEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5C4145493E548B358D8D737EEAE4E4968FA34DBAAA459B824F87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34197948450CB5442A1FCC3336B87FE4FF14B000972A3517036EF6FE9A10AB427D61B38EE1CC15E4DA1D7E09C32AA3244C238F830FCFB2B27D3A5A0919DCFDA111A95CA90A1D8AC565FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbsH3UG36S2ZRHUaUkKXmCg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7694C3BA24747F6F272DCB2F11EF51D46A1F1FEA02A07AA46D6EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Fix canonicalization of +-0.0 keys for IR_NEWREF. 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" Hello, Sergey! Thanks for the patch! To be honest I'm not proficient enough for reviewing such patches, but I don't have objections, only a couple of minor comments. See below. To give more confidence with fix I have started fuzzer for luaL_loadbuffer with applied patch (no crashes for a about 5 hours). Probably it is worth to mention that PUC Rio Lua has the same behaviour when table has indices "-0" and "0": [0] ~/sources/MRG/tarantool/third_party/luajit$ lua Lua 5.2.4  Copyright (C) 1994-2015 Lua.org, PUC-Rio > a = {[0] = 1, [2] = 4, [-0] = 7} > a[0] 7 > I believe it is important because you will say that Lua semantics will not broken after your patch. Sergey On 5/10/23 15:34, Sergey Kaplun wrote: > From: Mike Pall > > Reported by Sergey Kaplun. > > (cherry picked from commit 96fc114a7a3be3fd2c227d5a0ac53aa50cfb85d1) > > This commit is a follow-up for the commit > f067cf638cf8987ab3b6db372d609a5982e458b5 ("Fix narrowing of unary > minus."). Since this commit -0 IR constant is stored as well as +0 > constant on the trace. Since IR NEWREF keys don't canonicalizied for -0 > opposed of IR HREFK, it may lead to inconsistencies during trace > recording. > > In particular, since -0 and 0 are different IR constants, alias analysis > declares that they can't be aliased during folding optimization. > Therefore: > 1) For the IR TNEW we have non-nil value to lookup from the table via > HLOAD, when only nil lookup is expected due to alias analysis. > 2) For the IR TDUP we have non-nil value to lookup from the table via > HLOAD, but the template table has no 0 field initiated as far as -0 > isn't folding to 0 during parsing (see `bcemit_unop()` in > ). > These cases lead to the assertion failures in `fwd_ahload()`. > > This patch adds the aforementioned canonicalization. > > Sergey Bronnikov: > * reported the original issue for the TDUP IR > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#8516 > --- > > Side note: I don't mention the 981 issue by intend -- I don't want to > bother Mike with force pushes:). I suppose Igor should add this line (if > he wants) went this commit will be cherry-picked to our master branch > (a.k.a. tarantool). > > src/lj_record.c | 2 + > .../tarantool-tests/lj-981-folding-0.test.lua | 57 +++++++++++++++++++ > 2 files changed, 59 insertions(+) > create mode 100644 test/tarantool-tests/lj-981-folding-0.test.lua > > diff --git a/src/lj_record.c b/src/lj_record.c > index 9e2e1d9e..cc44db8d 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -1474,6 +1474,8 @@ TRef lj_record_idx(jit_State *J, RecordIndex *ix) > TRef key = ix->key; > if (tref_isinteger(key)) /* NEWREF needs a TValue as a key. */ > key = emitir(IRTN(IR_CONV), key, IRCONV_NUM_INT); > + else if (tref_isnumber(key) && tref_isk(key) && tvismzero(&ix->keyv)) > + key = lj_ir_knum_zero(J); /* Canonicalize -0.0 to +0.0. */ > xref = emitir(IRT(IR_NEWREF, IRT_PGC), ix->tab, key); > keybarrier = 0; /* NEWREF already takes care of the key barrier. */ > #ifdef LUAJIT_ENABLE_TABLE_BUMP > diff --git a/test/tarantool-tests/lj-981-folding-0.test.lua b/test/tarantool-tests/lj-981-folding-0.test.lua > new file mode 100644 > index 00000000..251da24d > --- /dev/null > +++ b/test/tarantool-tests/lj-981-folding-0.test.lua > @@ -0,0 +1,57 @@ > +local tap = require('tap') > +local test = tap.test('lj-981-folding-0'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', > +}) > + > +-- Test file to demonstrate LuaJIT misbehaviour on load forwarding > +-- for -0 IR constant as table index. > +-- See also, https://github.com/LuaJIT/LuaJIT/issues/981. > + > +local jparse = require('utils.jit_parse') > + > +jit.opt.start('hotloop=1') You changed global JIT settings, it is a good habit to put everything back when test is finished. > + > +test:plan(4) > + > +-- Reset traces. > +jit.flush() > + > +jparse.start('i') > +local result > +local expected = 'result' > +-- TNEW: > +-- -0 isn't folded during parsing, so it will be set with KSHORT, > +-- UNM bytecodes. See and bytecode listing > +-- for details. > +-- Because of it, empty table is created via TNEW. > +for _ = 1, 4 do > + result = ({[-0] = expected})[0] > +end > + > +local traces = jparse.finish() > + > +-- Test that there is no any assertion failure. > +test:ok(result == expected, 'TNEW and -0 folding') > +-- Test that there is no NEWREF -0 IR. > +test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TNEW tab') > + > +-- Reset traces. > +jit.flush() > + > +jparse.start('i') > +-- TDUP: > +-- Now just add a constant field for the table to use TDUP with > +-- template table instead TNEW before -0 is set. > +for _ = 1, 4 do > + result = ({[-0] = expected, [1] = 1})[0] > +end > + > +traces = jparse.finish() To be honest I think that chosen tables in tests are not representative. I propose to take this one: local expected = 1 local result for _ = 1, 4 do   result = ({[0] = 1, [-0] = 2})[0] end assert(result == 2) This example clearly demonstrates that element with index "0" was superseded by element with index "-0". > + > +-- Test that there is no any assertion failure. > +test:ok(result == expected, 'TDUP and -0 folding') > +-- Test that there is no NEWREF -0 IR. > +test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TDUP tab') > + > +os.exit(test:check() and 0 or 1)