Hi all,    QA LGTM     -- Vitaliia Ioffe     >Вторник, 3 августа 2021, 23:52 +03:00 от Igor Munkin via 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@tarantool.org > > >> >> Sergos >> >> > On 24 Jul 2021, at 20:23, Igor Munkin < imun@tarantool.org > 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 < imun@tarantool.org > >> > --- >> > >> > 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