[Tarantool-patches] [PATCH luajit] Detect inconsistent renames even in the presence of sunk values.

[Vitaliia Ioffe]

Hi all, 
 
QA LGTM
 
 
--
Vitaliia Ioffe
 
  
>Вторник, 3 августа 2021, 23:52 +03:00 от Igor Munkin via Tarantool-patches <tarantool-patches at dev.tarantool.org>:
> 
>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 at tarantool.org >
>
>>
>> Sergos
>>
>> > On 24 Jul 2021, at 20:23, Igor Munkin < imun at tarantool.org > wrote:
>> >
>> > From: Mike Pall <mike>
>> >
>> > 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 <asm_snap_checkrename>.
>> >
>> > However, the previous <asm_snap_checkrename> 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 <asm_snap_checkrename>
>> > 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 at 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
>> >
>
><snipped>
>
>> > 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, <opt_level> 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 <hotloop> 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 <gen_x> result value on line 39 is violated (nil is
>+-- returned from <ipairs_aux>) and trace execution is stopped.
>+-- 2. Next time TRACE 1 enters the field is iterating through the
>+-- second table given to <chain>. Its execution also stops at
>+-- the similar assertion but in the variant part this time.
>+-- 3. <wrap> function becomes hot enough while building new
>+-- <chain> 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 <chain> 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: <chain_gen_r1> 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 <ipairs_aux> 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
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210804/b2b6813d/attachment.htm>