Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!
Thanks for the review!
I've updated the patch considering your comments and force-pushed the
branch.

On 28.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Fri, Aug 25, 2023 at 06:00:24PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Analyzed by Sergey Kaplun.
> > 
> > (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9)
> > 
> > Recording of the `BC_TSETM` bytecode may keep too optimistic JIT
> > maxslot. In that case, the slot above the top of the Lua stack may be
> Typo: s/too optimistic JIT maxslot./the JIT maxslot too optimistic./

Fixed.

> > considered used. When any VM event handler is called before the
> > recording of the next instruction, this leads to an assertion failure in
> > `rec_check_slots()`.
> > 
> > This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a
> > table, which is always the highest slot after this bytecode. Also, it
> > adds an assertion that we check slots below the top of the Lua stack.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#8825
> > ---

<snipped>

> > +
> > +local TEST_VALUE = '5'
> > +local TEST_IDX = 5
> > +
> > +local function slot5()
> > +  return nil, nil, nil, nil, TEST_VALUE
> > +end
> Why the fifth slot? Drop a comment.

Fixed. See the iterative patch below.

> > +
> > +local storage
> > +local function test_tsetm(...)
> > +  -- Usage of `TSETM` bytecode.
> > +  storage = {slot5()}
> > +  -- Use this function again to trick use-def analysis and avoid
> > +  -- cleaning JIT slots, so the last JIT slot contains
> > +  -- `TEST_VALUE`.
> > +  return slot5(...)
> > +end
> > +
> > +-- Wrapper to avoid the recording of just the inner `slot5()`
> > +-- function.
> > +local function wrap()
> > +  test_tsetm()
> > +end
> > +
> > +jit.opt.start('hotloop=1')
> > +-- We need to call the VM event handler after each recorded bytecode
> > +-- instruction to pollute the Lua stack and the issue
> > +-- becomes observable.
> Typo: s/and the issue becomes/and make the issue/

Fixed.

See the iterative patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
index 7ae0a99d..74625a79 100644
--- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua
@@ -15,6 +15,8 @@ local jit_dump = require('jit.dump')
 local TEST_VALUE = '5'
 local TEST_IDX = 5
 
+-- XXX: Use big enough slot numbewr to be overwritten by VM event
+-- handler function.
 local function slot5()
   return nil, nil, nil, nil, TEST_VALUE
 end
@@ -36,9 +38,9 @@ local function wrap()
 end
 
 jit.opt.start('hotloop=1')
--- We need to call the VM event handler after each recorded bytecode
--- instruction to pollute the Lua stack and the issue
--- becomes observable.
+-- We need to call the VM event handler after each recorded
+-- bytecode instruction to pollute the Lua stack and make the
+-- issue observable.
 jit_dump.start('b', '/dev/null')
 
 -- Compile and execute the trace with `TSETM`.
===================================================================

> > +jit_dump.start('b', '/dev/null')
> > +

<snipped>

> > -- 
> > 2.41.0
> > 

-- 
Best regards,
Sergey Kaplun