Re: [Tarantool-patches] [PATCH luajit] Check frame size limit before returning to a lower frame.

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

Hi, Sergey!
Thanks for the fixes!
LGTM now.

On Wed, Mar 13, 2024 at 11:35:30AM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
> Fixed your comments below.
>
> On 12.03.24, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > LGTM, except for the single comment below.
> > On Tue, Mar 12, 2024 at 08:26:27AM +0300, Sergey Kaplun wrote:
> > > From: Mike Pall <mike>
> > >
> > > Thanks to Sergey Kaplun.
> > >
> > > (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa)
> > >
> > > When compiling a stitched (or side) trace, there is no check for the
> > > frame size of the current prototype during recording. Hence, when we
> > > return (for example, after stitching) to the lower frame with a maximum
> > > possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249)
> > > slot for GC64 mode may be used. This leads to the corresponding assertion
> > > failure in `rec_check_slots()`.
> > >
> > > This patch adds the corresponding check.
> > >
> > > Sergey Kaplun:
> > > * added the description and the test for the problem
> > >
> > > Part of tarantool/tarantool#9595
> > > ---
> > >
> > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1173-frame-limit-lower-frame
> > > Tarantool PR: https://github.com/tarantool/tarantool/pull/9791
> > > Related issues:
> > > * https://github.com/tarantool/tarantool/issues/9595
> > > * https://github.com/LuaJIT/LuaJIT/issues/1173
> > >
>
> <snipped>
>
> > > +local chunk = 'local uv = {key = 1}\n'
> > > +chunk = chunk .. 'return function()\n'
> > > +chunk = chunk .. 'local r = retf()\n'
> > Kind of a strange way to define a chunk. I believe that multiline
> > is better here.
>
> Totally agree, thanks!
>
> > > +
> > > +-- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount.
> > > +-- 1 slot is reserved (`r` variable), 1 pair is set outside the
> > > +-- cycle. 249 slots (the maximum available amount, see
> > > +-- <src/lj_parse.c>, `bcreg_bump()` for details) are occupied in
> > > +-- total.
> > > +for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do
> > > +  chunk = chunk .. ('uv.key, ')
> > > +end
> > > +chunk = chunk .. 'uv.key = nil\n'
> > > +chunk = chunk .. 'end\n'
> > Same applies here.
>
> Fixed. See the iterative patch below. Branch is force-pushed.
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> index 91e2c603..468462d2 100644
> --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> @@ -44,9 +44,11 @@ local LJ_MAX_JSLOTS = 250
>  -- `maxslot` (the first free slot) to 249. Hence, the JIT slots
>  -- are overflowing.
>
> -local chunk = 'local uv = {key = 1}\n'
> -chunk = chunk .. 'return function()\n'
> -chunk = chunk .. 'local r = retf()\n'
> +local chunk = [[
> +local uv = {key = 1}
> +return function()
> +  local r = retf()
> +  ]]
>
>  -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount.
>  -- 1 slot is reserved (`r` variable), 1 pair is set outside the
> @@ -56,8 +58,8 @@ chunk = chunk .. 'local r = retf()\n'
>  for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do
>    chunk = chunk .. ('uv.key, ')
>  end
> -chunk = chunk .. 'uv.key = nil\n'
> -chunk = chunk .. 'end\n'
> +chunk = chunk .. [[uv.key = nil
> +end]]
>
>  local get_func = assert(loadstring(chunk))
>  local function_max_framesize = get_func()
> ===================================================================
>
> <snipped>
>
> > >
>
> --
> Best regards,
> Sergey Kaplun