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 337464435A3; Sat, 20 May 2023 17:58:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 337464435A3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1684594731; bh=xSpsC/7mFKDQpEraD4lNBBChk+BayFYL7DVTv0iidic=; 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=CZG+puFDmhSQ5e4OGfS3B3ZgwM8dhg5O5hahRCMAsN5zwUokglLgxOEDOuzu5s79w E3HO0IWF4Qd29t5sROq/0BogkJnzXEyPkIluBJ/gi0KUbzskQQWnKbIEmrZDd6lGAT 4ZTyapVkL9ihI5ysUP+c0Jywk4MIsDNllyq6BtIY= 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 9C8934427C8 for ; Sat, 20 May 2023 17:58:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9C8934427C8 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1q0O2i-0004aB-OF; Sat, 20 May 2023 17:58:49 +0300 Date: Sat, 20 May 2023 17:54:44 +0300 To: Sergey Bronnikov Message-ID: References: <955c07a0-84f2-4598-7bc2-d7c1289546a8@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <955c07a0-84f2-4598-7bc2-d7c1289546a8@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD921E8753A900160F1284B0F69140CBAD1A760B8050B399C32182A05F53808504038E4B835A47ACAF5847874EB15189576F01468FBA101C25BF7634B27A18B9F60 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B2ADBDCD737373ABEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637013F392EFFCDE01C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B7733B1CDA4E2EF326C0AC6D9BFEA32E117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC7F48962964D238D0A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACD618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A1DCCEB63E2F10FB089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A51468F25DA28EDD3CF520C4E730CDB3409A9A553CAB912F5DF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D347130F804358653A66A2F66B01D31746506CB2567B5F924CA8A5B53CB406BFC28A26D2694E78072031D7E09C32AA3244C3A5AB6ABC49863792589C55C0EEE6F9235DA7DC5AF9B58C0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojlaZ30BpkxfLR3RE8wg2nGQ== X-DA7885C5: 0909271C23528A63CCA1503395242FF842D9E8375A5CEDD9E41A259C334C8D3C262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986BE905B5BE32805125FE51A2652AEEFE660FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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 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, Sergey! Thanks for the review! Please consider my comments below. On 16.05.23, Sergey Bronnikov wrote: > 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 Never too old to learn!:) Still, it is good for figuring out how LuaJIT works. > 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). Thanks! > > > Probably it is worth to mention that PUC Rio Lua has the same behaviour > when table has indices "-0" and "0": I suppose this isn't related to the patch itself: This is particularity of the parser. Also, nether patch nor test is about -0 vs 0 parsing as table keys. (*) > > [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. Yes, it is good to have such test, I sure. But this isn't related to the patch and its backporting. So, ignoring for now. > > Sergey > > > > 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. No, actually -- its common approach for bugfix tests: 1) We know, that this file will be executed stand-alone, without chain-evaluation with other tests, so this setting is applied only for this particular test. 2) OTOH, for we use exactly the suggested approach, since there are several JIT settings to check for different subtests. > > > + > > +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 I very strong against this approach, this is unrelated to the bug (*) and confusing for reading. > > assert(result == 2) > > This example clearly demonstrates that element with index "0" was > superseded by element with index "-0". As I said before, this is the good thing to do for checking parser or Lua semantics correctness, not this particular JIT bug. > > > > + > > +-- 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) -- Best regards, Sergey Kaplun