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 1BB586E674A; Mon, 20 Nov 2023 11:52:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1BB586E674A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1700470358; bh=eODF2hZ37AxwNAjnQyTYwYph9Qx9BnZ25HN/VwN5e7E=; 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=CarjeEFWRXvyUH6BLy7tGGOiLeEyFrN3M5MehbAHmsvXm0GLsbvTtSVJrg3SyN3b8 SGvmXCJRh+WvMr65Va0X1TD+ir3fcMk2CHpYO04bfr2VQ6oQ3RcxGflpZX2Uw/37nW IDPkxeY/ki3La4Y+3EcXX0FDrDkumYlZboAaXwLw= Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [95.163.41.88]) (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 B8A646E674A for ; Mon, 20 Nov 2023 11:52:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B8A646E674A Received: by smtp52.i.mail.ru with esmtpa (envelope-from ) id 1r501D-000zRF-0t; Mon, 20 Nov 2023 11:52:35 +0300 Date: Mon, 20 Nov 2023 11:48:02 +0300 To: Maxim Kokryashkin Message-ID: References: <20231116084959.24798-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9BE2AA157F04678487A33E32BE3FBF935015569BDBD2A1B54182A05F5380850404C228DA9ACA6FE272C3FEA0748D275B492FF19D1193A5B82F99B26A9DE0923F267A07E5A8898D94D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F6EE1C40B2E8BE15EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063706922F90966A37BA8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DC45184D3928B0DF1BB434ABE3DF4C36117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC974A882099E279BDA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE4B6963042765DA4B1421F57CABF51D54D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE30085B890FD2717DAC0837EA9F3D19764C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C989FD0BDF65E50FB2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F72BE6798D603635242539A7722CA490CB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5D30017BC71DBD98D2E7DAF8299F26A035CBE9943DD7EDCD6F87CCE6106E1FC07E67D4AC08A07B9B0E753FA5741D1AD02CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34BD150993ED26DDF487C90ED901F2C98F58779A3A8B7F2C24E44A99B6E30936FEEBD0066B305773F21D7E09C32AA3244C0754046481EB958AEE3DA3C66A5E3CCABBA718C7E6A9E04285A42E4C463514DC5DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmRIjd71J0y0A7x7tqJsjzw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769A98945FD5A28D90392FF19D1193A5B824AF810104A35EE5EDEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Add NaN check to 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, Maxim! Thanks for the review! Fixed your comment with question and force-pushed the branch. On 17.11.23, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, except for a single nit and a question below. > On Thu, Nov 16, 2023 at 11:49:59AM +0300, Sergey Kaplun wrote: > > From: Mike Pall > > > > Thanks to Peter Cawley. > > > > (cherry-picked from commit 7f9907b4ed0870ba64342bcc4b26cff0a94540da) > > > > When emitting IR NEWREF, there is no check for a non-NaN stored key > > value. Thus, when the NaN number value is given to trace, it may be > > stored as a key. This patch adds the corresponding check. If fold > > optimization is enabled, this IR EQ check is dropped if it references > > CONV IR from any (unsigned) integer type since NaN can be created via > > conversion from an integer. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#9145 > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1069-newref-nan-key > > Tarantool PR: https://github.com/tarantool/tarantool/pull/9374 > > Fuzzer link: https://oss-fuzz.com/testcase-detail/5251574662037504 > > Relate issues: > > * https://github.com/LuaJIT/LuaJIT/issues/1069 > > * https://github.com/tarantool/tarantool/issues/9145 > > > > src/lj_opt_fold.c | 5 +- > > src/lj_record.c | 12 +- > > .../lj-1069-newref-nan-key.test.lua | 151 ++++++++++++++++++ > > 3 files changed, 164 insertions(+), 4 deletions(-) > > create mode 100644 test/tarantool-tests/lj-1069-newref-nan-key.test.lua > > > > > diff --git a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua > > new file mode 100644 > > index 00000000..ec28b274 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua > > @@ -0,0 +1,151 @@ > > > +-- Test the constant IR NaN value on the trace. > Nit: This comment seems a bit redundant, as it duplicates the test name. Feel > free to ignore. It's also related to the second test. It's just like the section header for both of the tests. I prefer to leave it as is. > > +test:test('constant NaN on the trace', function(subtest) > > + -- Test the status and the error message. > > + subtest:plan(2) > > + local function protected() > > + local counter = 0 > > + -- Use a number key to emit NEWREF. > > + local key = 0.1 > > + > > + jit.opt.start('hotloop=1') > > + > > + while counter < 2 do > > + counter = counter + 1 > > + -- luacheck: ignore > > + local tab = {_ = 'unused'} > > + tab[key] = 'NaN' > > + -- XXX: Use the constant NaN value on the trace. > > + key = 0/0 > > + end > > + end > > + > > + local ok, err = pcall(protected) > > + subtest:ok(not ok, 'function returns an error') > > + subtest:like(err, 'table index is NaN', 'NaN index error message') > > +end) > > + > > +test:test('constant NaN on the trace and rehash of the table', function(subtest) > > + -- A little bit different test case: after rehashing the table, > > + -- the node is lost, and a hash part of the table isn't freed. > > + -- This leads to the assertion failure, which checks memory > > + -- leaks on shutdown. > > + -- XXX: The test didn't fail even before the patch. Check the > > + -- same values as in the previous test for consistency. > What do you mean by "didn't fail"? AFAICS, it leads to the assertion failure. > If you've meant no fails for a build with no assertions, then it worth > clarifying it in this comment. I mean that the checks below (`subtest:ok`, etc.) aren't fail. I've rephrased as the following: =================================================================== diff --git a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua index ec28b274..22553423 100644 --- a/test/tarantool-tests/lj-1069-newref-nan-key.test.lua +++ b/test/tarantool-tests/lj-1069-newref-nan-key.test.lua @@ -121,8 +121,8 @@ test:test('constant NaN on the trace and rehash of the table', function(subtest) -- the node is lost, and a hash part of the table isn't freed. -- This leads to the assertion failure, which checks memory -- leaks on shutdown. - -- XXX: The test didn't fail even before the patch. Check the - -- same values as in the previous test for consistency. + -- XXX: The test checks didn't fail even before the patch. Check + -- the same values as in the previous test for consistency. subtest:plan(2) local function protected() local counter = 0 =================================================================== Branch is force-pushed. > > -- Best regards, Sergey Kaplun