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 3FDAD6EC58; Tue, 3 Aug 2021 23:52:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3FDAD6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628023925; bh=spXSL9Q7xXDtuTJTBJC//ol9YRrmwj0EIa9lQJmK5WQ=; 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=YXXs7skNxu3pmFE+DkQwz9YyCaKiImfJIYh6/0Poz4KHivBhslmBDwL9QKcdZhLaL Q6fPw1NiG//SyQKhisTY2WfQMhCsK5uIWsHZ/rkAiFwkC8Nn3k2XnlHquN19p3STS6 DvgCg78JGGzX0WEb2UWiJ8RDLbYDHC3azCA/5qyg= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (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 2D73B6EC58 for ; Tue, 3 Aug 2021 23:52:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2D73B6EC58 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mB1OK-00076c-PU; Tue, 03 Aug 2021 23:52:01 +0300 Date: Tue, 3 Aug 2021 23:28:23 +0300 To: Sergey Ostanevich Message-ID: <20210803202823.GD27855@tarantool.org> References: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3FDAB68B812060C77E97CF8617D978122182A05F538085040B4E48C8E66DE7D9EA5A94D93554BC364801994123D8E64F49A1957734478ABFB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE728F774C865CF4B07EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063745B6F93C788775E78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D893FE80B82082A4693DC71547DDB6B0FF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A545B2C2BE55338D6D473B50BE6B4E1506EF830F5447CB01CBD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3438212F20E1E78D8E8C2BD2FBCE4DD86C78DEAEFE6464945F314AABDF743C1B02E3F25F5ADEF9E0781D7E09C32AA3244CF2953B0016F297E96326AE078B4DACD7408A6A02710B7304927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojeDyvyeZJDJHXT55YeHrN2w== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D911A0EB59E9FE5E9ACB457D9F3A8B7CFA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values. 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergos, Thanks for your review! On 27.07.21, Sergey Ostanevich wrote: > Hi! Thanks for the patch! > > Just a small nit to the test. I won’t comment Mike’s code :) The fix is much more clearer and simpler than the test for it... > > LGTM Added your tag: | Reviewed-by: Sergey Ostanevich > > Sergos > > > On 24 Jul 2021, at 20:23, Igor Munkin wrote: > > > > From: Mike Pall > > > > Reported by Igor Munkin. > > > > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d) > > > > Side exits with the same exitno use the same snapshot for restoring > > guest stack values. This obliges all guards related to the particular > > snapshot use the same RegSP mapping for the values to be restored at the > > trace exit. RENAME emitted prior to the guard for the same snapshot > > leads to the aforementioned invariant violation. The easy way to save > > the snapshot consistency is spilling the renamed IR reference, that is > > done in scope of . > > > > However, the previous implementation considers > > only the IR references explicitly mentioned in the snapshot. E.g. if > > there is a sunk[1] object to be restored at the trace exit, and the > > renamed reference is a *STORE to that object, the spill slot is not > > allocated. As a result an invalid value is stored while unsinking that > > object at all corresponding side exits prior to the emitted renaming. > > > > To handle also those IR references implicitly used in the snapshot, all > > non-constant and non-sunk references are added to the Bloom filter (it's > > worth to mention that two hash functions are used to reduce collisions > > for the cases when the number of IR references emitted between two > > different snapshots exceeds the filter size). New > > implementation tests whether the renamed IR reference is in the filter > > and forces a spill slot for it as a result. > > > > [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization > > > > Igor Munkin: > > * added the description and the test for the problem > > > > Resolves tarantool/tarantool#5118 > > Follows up tarantool/tarantool#4252 > > > > Signed-off-by: Igor Munkin > > --- > > > > Related issues: > > * https://github.com/tarantool/tarantool/issues/5118 > > * https://github.com/tarantool/tarantool/issues/4252 > > * https://github.com/LuaJIT/LuaJIT/issues/584 > > Branch: https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values > > CI: https://github.com/tarantool/tarantool/commit/b35e2ee > > > > src/lj_asm.c | 25 ++++--- > > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++ > > 2 files changed, 81 insertions(+), 13 deletions(-) > > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua > > > > diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua > > new file mode 100644 > > index 00000000..8aad3438 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua > > @@ -0,0 +1,69 @@ > > +local tap = require('tap') > > + > > +local test = tap.test('lj-584-bad-renames-for-sunk-values') > > +test:plan(1) > > + > > +-- Test file to demonstrate LuaJIT assembler misbehaviour. > > +-- For more info, proceed to the issues: > > +-- * https://github.com/LuaJIT/LuaJIT/issues/584 > > +-- * https://github.com/tarantool/tarantool/issues/4252 > > + > > +----- Related part of luafun.lua. -------------------------------- > > + > > +local iterator_mt = { > > + __call = function(self, param, state) return self.gen(param, state) end, > > +} > > + > > +local wrap = function(gen, param, state) > > + return setmetatable({ > > + gen = gen, > > + param = param, > > + state = state > > + }, iterator_mt), param, state > > +end > > + > > +-- These functions call each other to implement a flat iterator > > +-- over the several iterable objects. > > +local chain_gen_r1, chain_gen_r2 > > + > > +chain_gen_r2 = function(param, state, state_x, ...) > > + if state_x ~= nil then return { state[1], state_x }, ... end > > + local i = state[1] + 1 > > + if param[3 * i - 1] == nil then return nil end > > + return chain_gen_r1(param, { i, param[3 * i] }) > > +end > > + > > +chain_gen_r1 = function(param, state) > > + local i, state_x = state[1], state[2] > > + local gen_x, param_x = param[3 * i - 2], param[3 * i - 1] > > + return chain_gen_r2(param, state, gen_x(param_x, state_x)) > > +end > > + > > +local chain = function(...) > > + local param = { } > > + for i = 1, select('#', ...) do > > + -- Put gen, param, state into param table. > > + param[3 * i - 2], param[3 * i - 1], param[3 * i] > > + = wrap(ipairs(select(i, ...))) > > + end > > + return wrap(chain_gen_r1, param, { 1, param[3] }) > > +end > > + > > +----- Reproducer. ------------------------------------------------ > > + > > +jit.opt.start(3, 'hotloop=3') > > I don’t like both numbers here. opt_level is 3 by default - why bother setting it? > And the second one should be factored out as an argument for both opt.start and the > loop below? Oops, this is the only place, that I didn't clean up... Yes, is excess: it is an artefact of juggling with options for reproducing. Now it's quite clear that the issue relates to allocation sinking optimization, that requires all flags to be enabled. Regarding the value, I dropped a verbose comment. Hope it makes the situation clearer, diff is below: ================================================================================ diff --git a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua index 8aad3438..f037c898 100644 --- a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua @@ -51,7 +51,35 @@ end ----- Reproducer. ------------------------------------------------ -jit.opt.start(3, 'hotloop=3') +-- XXX: Here one can find the rationale for the 'hotloop' value. +-- 1. The most inner while loop on the line 86 starts recording +-- for the third element (i.e. 'c') and successfully compiles +-- as TRACE 1. However, its execution stops, since type guard +-- for result value on line 39 is violated (nil is +-- returned from ) and trace execution is stopped. +-- 2. Next time TRACE 1 enters the field is iterating through the +-- second table given to . Its execution also stops at +-- the similar assertion but in the variant part this time. +-- 3. function becomes hot enough while building new +-- iterator, and it is compiled as TRACE 2. +-- There are also other attempts, but all of them failed. +-- 4. Again, TRACE 1 reigns while iterating through the first +-- table given to and finishes at the same guard the +-- previous run does. Anyway, everything above is just an +-- auxiliary activity preparing the JIT environment for the +-- following result. +-- 5. Here we finally come: is finally ready to be +-- recorded. It successfully compiles as TRACE 3. However, the +-- boundary case is recorded, so the trace execution stops +-- since nil *is not* returned from on the next +-- iteration. +-- +-- JIT fine tuning via 'hotloop' option allows to catch this +-- elusive case, we achieved in a last bullet. The reason, why +-- this case leads to a misbehaviour while restoring the guest +-- stack at the trace exit, is described in the following LuaJIT +-- issue: https://github.com/LuaJIT/LuaJIT/issues/584. +jit.opt.start('hotloop=3') xpcall(function() for _ = 1, 3 do ================================================================================ If you're OK with the comment, I'll proceed with the patch. > > > + > > +xpcall(function() > > + for _ = 1, 3 do > > + local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' }) > > + while true do > > + state_x = gen_x(param_x, state_x) > > + if state_x == nil then break end > > + end > > + end > > + test:ok('All emitted RENAMEs are fine') > > +end, function() > > + test:fail('Invalid Lua stack has been restored') > > +end) > > + > > +os.exit(test:check() and 0 or 1) > > -- > > 2.25.0 > > > -- Best regards, IM