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 98DC06EC55; Tue, 27 Jul 2021 16:53:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 98DC06EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627393986; bh=2NXrcU1yRbk4Ckgu20kwOX0tfZSSb7BizbeTx+4INmg=; h=In-Reply-To:Date:References:To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gp3TjpsBkIx7VeBaHyryZGvX32vuT0xYzVdNtqU24orJkc6HJM+X9vBYBjivfnNY2 uQZn+w7hb4hFfTmSjfLCunXHS195ckaLDsfNdqQxzqpWkjgeTM806KisNEsjR/uWS7 dSJuDeQU9Tb1nmi1Gk6oerOeXfNzNYH7nv0Vzx8o= Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 7257A6EC55 for ; Tue, 27 Jul 2021 16:53:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7257A6EC55 Received: by smtp40.i.mail.ru with esmtpa (envelope-from ) id 1m8NW3-0000Vo-7f; Tue, 27 Jul 2021 16:53:03 +0300 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\)) In-Reply-To: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> Date: Tue, 27 Jul 2021 16:53:02 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <5fdb4899061156f0fb4c53027d55f93be3a24759.1627144350.git.imun@tarantool.org> To: Igor Munkin X-Mailer: Apple Mail (2.3654.100.0.2.22) X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3104FC76DFAAAAF7DA068FE323FAC4379182A05F53808504048BF2212BB7973A67B2B947C3FBDDC91BFD0087146D5584133B5A573544408C6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FBB2043146276655EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637279A5203CF71F5518638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C97EE675386EBC1C04C7B3492066A20F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6A1DCCEB63E2F10FB089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5B10946AB8FDB389AD50127C3E502C92ECE438E297A6ABE43D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FA7FF33AA1A4D21C410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34E318F287A436F24CEC3646DA014B9D1B7FCAAC3FF39E0F4DAD02598DDF134B4C016B1BF48E9189E21D7E09C32AA3244CEB52EE8CE43A9B60FBB802F2A3A11C4B1DD47778AE04E04DFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMEANdStWW58zzJWy5Hli1Q== X-Mailru-Sender: 3B9A0136629DC912F4AABCEFC589C81EDEA82AA33C1FD7C7DF8F43A32CB39A72BF8943611DA1D9BCAD07DD1419AC565FA614486B47F28B67C5E079CCF3B0523AED31B7EB2E253A9E112434F685709FCF0DA7A0AF5A3A8387 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 Ostanevich via Tarantool-patches Reply-To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! Just a small nit to the test. I won=E2=80=99t comment Mike=E2=80=99s = code :) LGTM Sergos > On 24 Jul 2021, at 20:23, Igor Munkin wrote: >=20 > From: Mike Pall >=20 > Reported by Igor Munkin. >=20 > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d) >=20 > 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 . >=20 > 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. >=20 > 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. >=20 > [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization >=20 > Igor Munkin: > * added the description and the test for the problem >=20 > Resolves tarantool/tarantool#5118 > Follows up tarantool/tarantool#4252 >=20 > Signed-off-by: Igor Munkin > --- >=20 > 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 >=20 > 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 >=20 > 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 > @@ -72,6 +72,7 @@ typedef struct ASMState { > IRRef snaprename; /* Rename highwater mark for snapshot check. */ > SnapNo snapno; /* Current snapshot number. */ > SnapNo loopsnapno; /* Loop snapshot number. */ > + BloomFilter snapfilt1, snapfilt2; /* Filled with snapshot refs. */ >=20 > IRRef fuseref; /* Fusion limit (loopref, 0 or FUSE_DISABLED). = */ > IRRef sectref; /* Section base reference (loopref or 0). */ > @@ -876,7 +877,10 @@ static int asm_sunk_store(ASMState *as, IRIns = *ira, IRIns *irs) > static void asm_snap_alloc1(ASMState *as, IRRef ref) > { > IRIns *ir =3D IR(ref); > - if (!irref_isk(ref) && (!(ra_used(ir) || ir->r =3D=3D RID_SUNK))) { > + if (!irref_isk(ref) && ir->r !=3D RID_SUNK) { > + bloomset(as->snapfilt1, ref); > + bloomset(as->snapfilt2, hashrot(ref, ref + HASH_BIAS)); > + if (ra_used(ir)) return; > if (ir->r =3D=3D RID_SINK) { > ir->r =3D RID_SUNK; > #if LJ_HASFFI > @@ -933,6 +937,7 @@ static void asm_snap_alloc(ASMState *as) > SnapShot *snap =3D &as->T->snap[as->snapno]; > SnapEntry *map =3D &as->T->snapmap[snap->mapofs]; > MSize n, nent =3D snap->nent; > + as->snapfilt1 =3D as->snapfilt2 =3D 0; > for (n =3D 0; n < nent; n++) { > SnapEntry sn =3D map[n]; > IRRef ref =3D snap_ref(sn); > @@ -955,18 +960,12 @@ static void asm_snap_alloc(ASMState *as) > */ > static int asm_snap_checkrename(ASMState *as, IRRef ren) > { > - SnapShot *snap =3D &as->T->snap[as->snapno]; > - SnapEntry *map =3D &as->T->snapmap[snap->mapofs]; > - MSize n, nent =3D snap->nent; > - for (n =3D 0; n < nent; n++) { > - SnapEntry sn =3D map[n]; > - IRRef ref =3D snap_ref(sn); > - if (ref =3D=3D ren || (LJ_SOFTFP && (sn & SNAP_SOFTFPNUM) && = ++ref =3D=3D ren)) { > - IRIns *ir =3D IR(ref); > - ra_spill(as, ir); /* Register renamed, so force a spill slot. = */ > - RA_DBGX((as, "snaprensp $f $s", ref, ir->s)); > - return 1; /* Found. */ > - } > + if (bloomtest(as->snapfilt1, ren) && > + bloomtest(as->snapfilt2, hashrot(ren, ren + HASH_BIAS))) { > + IRIns *ir =3D IR(ren); > + ra_spill(as, ir); /* Register renamed, so force a spill slot. */ > + RA_DBGX((as, "snaprensp $f $s", ren, ir->s)); > + return 1; /* Found. */ > } > return 0; /* Not found. */ > } > 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 =3D require('tap') > + > +local test =3D 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 =3D { > + __call =3D function(self, param, state) return self.gen(param, = state) end, > +} > + > +local wrap =3D function(gen, param, state) > + return setmetatable({ > + gen =3D gen, > + param =3D param, > + state =3D 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 =3D function(param, state, state_x, ...) > + if state_x ~=3D nil then return { state[1], state_x }, ... end > + local i =3D state[1] + 1 > + if param[3 * i - 1] =3D=3D nil then return nil end > + return chain_gen_r1(param, { i, param[3 * i] }) > +end > + > +chain_gen_r1 =3D function(param, state) > + local i, state_x =3D state[1], state[2] > + local gen_x, param_x =3D param[3 * i - 2], param[3 * i - 1] > + return chain_gen_r2(param, state, gen_x(param_x, state_x)) > +end > + > +local chain =3D function(...) > + local param =3D { } > + for i =3D 1, select('#', ...) do > + -- Put gen, param, state into param table. > + param[3 * i - 2], param[3 * i - 1], param[3 * i] > + =3D wrap(ipairs(select(i, ...))) > + end > + return wrap(chain_gen_r1, param, { 1, param[3] }) > +end > + > +----- Reproducer. ------------------------------------------------ > + > +jit.opt.start(3, 'hotloop=3D3') I don=E2=80=99t 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? > + > +xpcall(function() > + for _ =3D 1, 3 do > + local gen_x, param_x, state_x =3D chain({ 'a', 'b', 'c' }, { 'q', = 'w', 'e' }) > + while true do > + state_x =3D gen_x(param_x, state_x) > + if state_x =3D=3D 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) > --=20 > 2.25.0 >=20