<HTML><BODY><div>Hi all, </div><div> </div><div>QA LGTM</div><div> </div><div> </div><div data-signature-widget="container"><div spellcheck="false" data-signature-widget="content"><div>--<br>Vitaliia Ioffe</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 3 августа 2021, 23:52 +03:00 от Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16280239360035764627_BODY">Sergos,<br><br>Thanks for your review!<br><br>On 27.07.21, Sergey Ostanevich wrote:<br>> Hi! Thanks for the patch!<br>><br>> Just a small nit to the test. I won’t comment Mike’s code :)<br><br>The fix is much more clearer and simpler than the test for it...<br><br>><br>> LGTM<br><br>Added your tag:<br>| Reviewed-by: Sergey Ostanevich <<a href="/compose?To=sergos@tarantool.org">sergos@tarantool.org</a>><br><br>><br>> Sergos<br>><br>> > On 24 Jul 2021, at 20:23, Igor Munkin <<a href="/compose?To=imun@tarantool.org">imun@tarantool.org</a>> wrote:<br>> ><br>> > From: Mike Pall <mike><br>> ><br>> > Reported by Igor Munkin.<br>> ><br>> > (cherry picked from commit 33e3f4badfde8cd9c202cedd1f4ed9275bc92e7d)<br>> ><br>> > Side exits with the same exitno use the same snapshot for restoring<br>> > guest stack values. This obliges all guards related to the particular<br>> > snapshot use the same RegSP mapping for the values to be restored at the<br>> > trace exit. RENAME emitted prior to the guard for the same snapshot<br>> > leads to the aforementioned invariant violation. The easy way to save<br>> > the snapshot consistency is spilling the renamed IR reference, that is<br>> > done in scope of <asm_snap_checkrename>.<br>> ><br>> > However, the previous <asm_snap_checkrename> implementation considers<br>> > only the IR references explicitly mentioned in the snapshot. E.g. if<br>> > there is a sunk[1] object to be restored at the trace exit, and the<br>> > renamed reference is a *STORE to that object, the spill slot is not<br>> > allocated. As a result an invalid value is stored while unsinking that<br>> > object at all corresponding side exits prior to the emitted renaming.<br>> ><br>> > To handle also those IR references implicitly used in the snapshot, all<br>> > non-constant and non-sunk references are added to the Bloom filter (it's<br>> > worth to mention that two hash functions are used to reduce collisions<br>> > for the cases when the number of IR references emitted between two<br>> > different snapshots exceeds the filter size). New <asm_snap_checkrename><br>> > implementation tests whether the renamed IR reference is in the filter<br>> > and forces a spill slot for it as a result.<br>> ><br>> > [1]: <a href="http://wiki.luajit.org/Allocation-Sinking-Optimization" target="_blank">http://wiki.luajit.org/Allocation-Sinking-Optimization</a><br>> ><br>> > Igor Munkin:<br>> > * added the description and the test for the problem<br>> ><br>> > Resolves tarantool/tarantool#5118<br>> > Follows up tarantool/tarantool#4252<br>> ><br>> > Signed-off-by: Igor Munkin <<a href="/compose?To=imun@tarantool.org">imun@tarantool.org</a>><br>> > ---<br>> ><br>> > Related issues:<br>> > * <a href="https://github.com/tarantool/tarantool/issues/5118" target="_blank">https://github.com/tarantool/tarantool/issues/5118</a><br>> > * <a href="https://github.com/tarantool/tarantool/issues/4252" target="_blank">https://github.com/tarantool/tarantool/issues/4252</a><br>> > * <a href="https://github.com/LuaJIT/LuaJIT/issues/584" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/584</a><br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values" target="_blank">https://github.com/tarantool/luajit/tree/imun/lj-584-bad-renames-for-sunk-values</a><br>> > CI: <a href="https://github.com/tarantool/tarantool/commit/b35e2ee" target="_blank">https://github.com/tarantool/tarantool/commit/b35e2ee</a><br>> ><br>> > src/lj_asm.c | 25 ++++---<br>> > ...j-584-bad-renames-for-sunk-values.test.lua | 69 +++++++++++++++++++<br>> > 2 files changed, 81 insertions(+), 13 deletions(-)<br>> > create mode 100644 test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua<br>> ><br><br><snipped><br><br>> > 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<br>> > new file mode 100644<br>> > index 00000000..8aad3438<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua<br>> > @@ -0,0 +1,69 @@<br>> > +local tap = require('tap')<br>> > +<br>> > +local test = tap.test('lj-584-bad-renames-for-sunk-values')<br>> > +test:plan(1)<br>> > +<br>> > +-- Test file to demonstrate LuaJIT assembler misbehaviour.<br>> > +-- For more info, proceed to the issues:<br>> > +-- * <a href="https://github.com/LuaJIT/LuaJIT/issues/584" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/584</a><br>> > +-- * <a href="https://github.com/tarantool/tarantool/issues/4252" target="_blank">https://github.com/tarantool/tarantool/issues/4252</a><br>> > +<br>> > +----- Related part of luafun.lua. --------------------------------<br>> > +<br>> > +local iterator_mt = {<br>> > + __call = function(self, param, state) return self.gen(param, state) end,<br>> > +}<br>> > +<br>> > +local wrap = function(gen, param, state)<br>> > + return setmetatable({<br>> > + gen = gen,<br>> > + param = param,<br>> > + state = state<br>> > + }, iterator_mt), param, state<br>> > +end<br>> > +<br>> > +-- These functions call each other to implement a flat iterator<br>> > +-- over the several iterable objects.<br>> > +local chain_gen_r1, chain_gen_r2<br>> > +<br>> > +chain_gen_r2 = function(param, state, state_x, ...)<br>> > + if state_x ~= nil then return { state[1], state_x }, ... end<br>> > + local i = state[1] + 1<br>> > + if param[3 * i - 1] == nil then return nil end<br>> > + return chain_gen_r1(param, { i, param[3 * i] })<br>> > +end<br>> > +<br>> > +chain_gen_r1 = function(param, state)<br>> > + local i, state_x = state[1], state[2]<br>> > + local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]<br>> > + return chain_gen_r2(param, state, gen_x(param_x, state_x))<br>> > +end<br>> > +<br>> > +local chain = function(...)<br>> > + local param = { }<br>> > + for i = 1, select('#', ...) do<br>> > + -- Put gen, param, state into param table.<br>> > + param[3 * i - 2], param[3 * i - 1], param[3 * i]<br>> > + = wrap(ipairs(select(i, ...)))<br>> > + end<br>> > + return wrap(chain_gen_r1, param, { 1, param[3] })<br>> > +end<br>> > +<br>> > +----- Reproducer. ------------------------------------------------<br>> > +<br>> > +jit.opt.start(3, 'hotloop=3')<br>><br>> I don’t like both numbers here. opt_level is 3 by default - why bother setting it?<br>> And the second one should be factored out as an argument for both opt.start and the<br>> loop below?<br><br>Oops, this is the only place, that I didn't clean up...<br><br>Yes, <opt_level> is excess: it is an artefact of juggling with options<br>for reproducing. Now it's quite clear that the issue relates to<br>allocation sinking optimization, that requires all flags to be enabled.<br><br>Regarding the <hotloop> value, I dropped a verbose comment. Hope it<br>makes the situation clearer, diff is below:<br><br>================================================================================<br><br>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<br>index 8aad3438..f037c898 100644<br>--- a/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua<br>+++ b/test/tarantool-tests/lj-584-bad-renames-for-sunk-values.test.lua<br>@@ -51,7 +51,35 @@ end<br> <br> ----- Reproducer. ------------------------------------------------<br> <br>-jit.opt.start(3, 'hotloop=3')<br>+-- XXX: Here one can find the rationale for the 'hotloop' value.<br>+-- 1. The most inner while loop on the line 86 starts recording<br>+-- for the third element (i.e. 'c') and successfully compiles<br>+-- as TRACE 1. However, its execution stops, since type guard<br>+-- for <gen_x> result value on line 39 is violated (nil is<br>+-- returned from <ipairs_aux>) and trace execution is stopped.<br>+-- 2. Next time TRACE 1 enters the field is iterating through the<br>+-- second table given to <chain>. Its execution also stops at<br>+-- the similar assertion but in the variant part this time.<br>+-- 3. <wrap> function becomes hot enough while building new<br>+-- <chain> iterator, and it is compiled as TRACE 2.<br>+-- There are also other attempts, but all of them failed.<br>+-- 4. Again, TRACE 1 reigns while iterating through the first<br>+-- table given to <chain> and finishes at the same guard the<br>+-- previous run does. Anyway, everything above is just an<br>+-- auxiliary activity preparing the JIT environment for the<br>+-- following result.<br>+-- 5. Here we finally come: <chain_gen_r1> is finally ready to be<br>+-- recorded. It successfully compiles as TRACE 3. However, the<br>+-- boundary case is recorded, so the trace execution stops<br>+-- since nil *is not* returned from <ipairs_aux> on the next<br>+-- iteration.<br>+--<br>+-- JIT fine tuning via 'hotloop' option allows to catch this<br>+-- elusive case, we achieved in a last bullet. The reason, why<br>+-- this case leads to a misbehaviour while restoring the guest<br>+-- stack at the trace exit, is described in the following LuaJIT<br>+-- issue: <a href="https://github.com/LuaJIT/LuaJIT/issues/584" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/584</a>.<br>+jit.opt.start('hotloop=3')<br> <br> xpcall(function()<br> for _ = 1, 3 do<br><br>================================================================================<br><br>If you're OK with the comment, I'll proceed with the patch.<br><br>><br>> > +<br>> > +xpcall(function()<br>> > + for _ = 1, 3 do<br>> > + local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })<br>> > + while true do<br>> > + state_x = gen_x(param_x, state_x)<br>> > + if state_x == nil then break end<br>> > + end<br>> > + end<br>> > + test:ok('All emitted RENAMEs are fine')<br>> > +end, function()<br>> > + test:fail('Invalid Lua stack has been restored')<br>> > +end)<br>> > +<br>> > +os.exit(test:check() and 0 or 1)<br>> > --<br>> > 2.25.0<br>> ><br>><br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div> </div></BODY></HTML>