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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the fixes!
LGTM, except for a few more comments below.
On Mon, Aug 28, 2023 at 06:02:12PM +0300, Sergey Kaplun wrote:
> 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
Typo: s/Use/Use a/
Typo: s/numbewr/number/
Typo: s/by VM/by the VM/
> +-- handler function.
Please also add that this is an empirical number.
>  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