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 A8A716EC58; Sun, 1 Aug 2021 19:16:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A8A716EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627834572; bh=+PtlaR4wxfaT9qt+65a2UaT0KIoVd8VagOdloQC2ANg=; 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=KEUlS/5QgGl7OPac/orK8k3VS5Wy1CGfYMSckqYhgEeG5Xy06Hruavd9cPX3/SaHt WKC0XCD+z558UKg3hQbagn3R6nZg7YyxPJdif5ngLlpB2zdIfkMnqJGnlkkMXh2X+K 15aS6KnzygNqql+Gg3K2Gwm8s5fIbuwfYy1DGE/U= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 113FA6EC58 for ; Sun, 1 Aug 2021 19:16:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 113FA6EC58 Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1mAE8I-0006Y2-1m; Sun, 01 Aug 2021 19:16:10 +0300 Date: Sun, 1 Aug 2021 19:14:57 +0300 To: Igor Munkin Message-ID: References: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD941C43E597735A9C351B198F4576AC7B21928AAE70459C21B182A05F53808504041F887B2EA6CBA8488434848FC8265D9ACD6A8BA674086A286CB9D3597FA8FB2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75644E22E05AA81AEB287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF23F8577A6DFFEA7C0F1463317FA92A598F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB5533756685811CA530B5049182CB2825ADC275571355DF42DF0627CAA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C317B107DEF921CE79117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A53CFDB0D796D592F4E56FD58A2829F47D80E0913658627127D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA751B940EDA0DFB0535410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34431D0341F6B74DD30AAC96201820BD6167F6C0846DC39EF360B8F9CA1A8F60459BE3A3914A99EF381D7E09C32AA3244CC3027BD463548A73A9E21F929ADDEB6A3E8609A02908F271FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMCfuYI4Pree8iDKxH8t1qg== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB47441AAA64B90D3385B492CA272CB8641AA34DFAC7EFFC4FCF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: 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, Igor! Thanks for the patch! Is there no point to simplify the test -- we have 5 different traces, when really need the only one (with RENAME between two possible jump to fallback branches with restoration from snapshot)? Feel free to ignore. Side note: also, we should test that sunk optimization still works, shouldn't we? Feel free to ignore. Otherwise, LGTM. On 24.07.21, 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. This sentence a little bit unclear to me: 1) Leads how? 2) Do you mean it in terms of recording (i. e. the reverse instrucions recording order) or not? > 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/src/lj_asm.c b/src/lj_asm.c > index c2cf5a95..9267448e 100644 > --- a/src/lj_asm.c > +++ b/src/lj_asm.c > 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') > + > +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, Sergey Kaplun