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

Igor Munkin imun at tarantool.org
Tue Aug 3 23:28:23 MSK 2021


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


More information about the Tarantool-patches mailing list