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

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

Sergey,

LGTM with minor comment below.

On 3/13/24 12:37, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
>
> On 12.03.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>>
>> thanks for the patch!
>>
>> On 3/12/24 08:26, 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
> Updated commit message to the following, see the comment below.
>
> | Check frame size limit before returning to a lower frame.
> |
> | 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. The `LJ_MAX_JSLOTS` and
> | `LJ_MAX_SLOTS` are equal by default, but their values may be manually
> | changed for some custom builds. Hence, the check is not enabled only for
> | `LJ_GC64` mode.
> |
> | Sergey Kaplun:
> | * added the description and the test for the problem
> |
> | Part of tarantool/tarantool#9595
Thanks!
>>> ---
>>>
>>> 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>
>
>>> 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
>>> new file mode 100644
>>> index 00000000..91e2c603
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> <snipped>
>
>>> +
>>> +local LJ_MAX_JSLOTS = 250
>> I would say in a comment that constant is from <src/lj_def.h>.
>>
>> Your test depends on this constant (if it will be changed the test will
>> test nothing),
>>
>> how to make sure that LJ_MAX_JSLOTS is equal to LJ_MAX_JSLOTS in
>> <src/lj_def.h>?
> Honestly, I don't know any good way to do it now. This limit is
> "hard-defined", but considering Mike's comment [1] may be changed by
> hand by someone manually for their installation. For now, I suppose Is
> should just leave a comment here. Also, by grepping, anyone who applies
> the patch that changes the `LJ_MAX_JSLOTS` limit should see the
> following definition and adjust it too.

It is not perfect, but I don't know how to link macros in C and constant 
in Lua better.

Thanks for the fix.

>
> See the iterative patch below.
>
> ===================================================================
> 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 468462d2..b454003e 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
> @@ -17,6 +17,9 @@ local function retf()
>   end
>   _G.retf = retf
>   
> +-- The maximum number of stack slots for a trace. Defined in the
> +-- <src/lj_def.h>. Also, it equals `LJ_MAX_SLOTS` -- the maximum
> +-- number of slots in a Lua function.
>   local LJ_MAX_JSLOTS = 250
>   
>   -- Generate the following function:
> ===================================================================
>
>>> +
>>> +-- Generate the following function:
> <snipped>
>
>>> +
>>> +local chunk = 'local uv = {key = 1}\n'
>>> +chunk = chunk .. 'return function()\n'
>>> +chunk = chunk .. 'local r = retf()\n'
>>> +
>>> +-- 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'
>> Why not to use multiline here and after the loop?
> Good idea, thanks!
> 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()
> +  ]]

here brackets are on a new line with indentation and below brackets on 
the same line with code.

looks inconsistently.

>   
>   -- 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()
> ===================================================================
>
>>> +local get_func = assert(loadstring(chunk))
> <snipped>
>
>>> +test:done(true)
> [1]: https://github.com/LuaJIT/LuaJIT/issues/1173#issuecomment-1987290608
>