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

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

[Sergey Kaplun via Tarantool-patches]

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

 src/lj_record.c                               |  2 +
 .../lj-1173-frame-limit-lower-frame.test.lua  | 83 +++++++++++++++++++
 2 files changed, 85 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index c01c1f0b..e3590b1a 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -886,6 +886,8 @@ void lj_record_ret(jit_State *J, BCReg rbase, ptrdiff_t gotresults)
       lj_trace_err(J, LJ_TRERR_LLEAVE);
     } else if (J->needsnap) {  /* Tailcalled to ff with side-effects. */
       lj_trace_err(J, LJ_TRERR_NYIRETL);  /* No way to insert snapshot here. */
+    } else if (1 + pt->framesize >= LJ_MAX_JSLOTS) {
+      lj_trace_err(J, LJ_TRERR_STACKOV);
     } else {  /* Return to lower frame. Guard for the target we return to. */
       TRef trpt = lj_ir_kgc(J, obj2gco(pt), IRT_PROTO);
       TRef trpc = lj_ir_kptr(J, (void *)frame_pc(frame));
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
@@ -0,0 +1,83 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT assertion failure during
+-- recording of side trace in GC64 mode with return to lower
+-- frame, which has the maximum possible frame size.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1173.
+
+local test = tap.test('lj-1173-frame-limit-lower-frame'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- Parent trace with stitching and returning to a lower frame.
+local function retf()
+  math.modf(1)
+end
+_G.retf = retf
+
+local LJ_MAX_JSLOTS = 250
+
+-- Generate the following function:
+-- | local uv = {key = 1}
+-- | return function()
+-- |   local r = retf()
+-- |   uv.key, uv.key, --[[124 times in total ...]] uv.key = nil
+-- | end
+-- It will have the following bytecode:
+-- | 0001    GGET     0   0      ; "retf"
+-- | 0002    CALL     0   2   1
+-- | 0003    UGET     1   0      ; uv
+-- | ...
+-- | 0126    UGET   124   0      ; uv
+-- | 0127    KNIL   125 248
+-- | 0128    TSETS  248 124   1  ; "key"
+-- | ...
+-- | 0251    TSETS  125   1   1  ; "key"
+-- | 0252    RET0     0   1
+-- As you can see, the 249 slots (from 0 to 248) are occupied in
+-- total.
+-- When we return to the lower frame for the side trace, we may
+-- hit the slot limit mentioned above: 2 slots are occupied
+-- by the frame (`baseslot`) and `KNIL` bytecode recording sets
+-- `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'
+
+-- 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'
+
+local get_func = assert(loadstring(chunk))
+local function_max_framesize = get_func()
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+-- Compile the parent trace first.
+retf()
+retf()
+
+-- Try to compile the side trace with a return to a lower frame
+-- with a huge frame size.
+function_max_framesize()
+function_max_framesize()
+
+-- XXX: The limit check is OK with default defines for non-GC64
+-- mode, the trace is compiled for it. The test fails only with
+-- GC64 mode enabled. Still run the test for non-GC64 mode to
+-- avoid regressions.
+
+test:ok(true, 'no assertion failure during recording')
+
+test:done(true)
-- 
2.44.0

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

[Sergey Bronnikov via Tarantool-patches]

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
> ---
>
> 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
>
>   src/lj_record.c                               |  2 +
>   .../lj-1173-frame-limit-lower-frame.test.lua  | 83 +++++++++++++++++++
>   2 files changed, 85 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
>
> diff --git a/src/lj_record.c b/src/lj_record.c
> index c01c1f0b..e3590b1a 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -886,6 +886,8 @@ void lj_record_ret(jit_State *J, BCReg rbase, ptrdiff_t gotresults)
>         lj_trace_err(J, LJ_TRERR_LLEAVE);
>       } else if (J->needsnap) {  /* Tailcalled to ff with side-effects. */
>         lj_trace_err(J, LJ_TRERR_NYIRETL);  /* No way to insert snapshot here. */
> +    } else if (1 + pt->framesize >= LJ_MAX_JSLOTS) {
> +      lj_trace_err(J, LJ_TRERR_STACKOV);
>       } else {  /* Return to lower frame. Guard for the target we return to. */
>         TRef trpt = lj_ir_kgc(J, obj2gco(pt), IRT_PROTO);
>         TRef trpc = lj_ir_kptr(J, (void *)frame_pc(frame));
> 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
> @@ -0,0 +1,83 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT assertion failure during
> +-- recording of side trace in GC64 mode with return to lower
> +-- frame, which has the maximum possible frame size.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1173.
> +
> +local test = tap.test('lj-1173-frame-limit-lower-frame'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +-- Parent trace with stitching and returning to a lower frame.
> +local function retf()
> +  math.modf(1)
> +end
> +_G.retf = retf
> +
> +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>?

> +
> +-- Generate the following function:
> +-- | local uv = {key = 1}
> +-- | return function()
> +-- |   local r = retf()
> +-- |   uv.key, uv.key, --[[124 times in total ...]] uv.key = nil
> +-- | end
> +-- It will have the following bytecode:
> +-- | 0001    GGET     0   0      ; "retf"
> +-- | 0002    CALL     0   2   1
> +-- | 0003    UGET     1   0      ; uv
> +-- | ...
> +-- | 0126    UGET   124   0      ; uv
> +-- | 0127    KNIL   125 248
> +-- | 0128    TSETS  248 124   1  ; "key"
> +-- | ...
> +-- | 0251    TSETS  125   1   1  ; "key"
> +-- | 0252    RET0     0   1
> +-- As you can see, the 249 slots (from 0 to 248) are occupied in
> +-- total.
> +-- When we return to the lower frame for the side trace, we may
> +-- hit the slot limit mentioned above: 2 slots are occupied
> +-- by the frame (`baseslot`) and `KNIL` bytecode recording sets
> +-- `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'
> +
> +-- 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?
> +local get_func = assert(loadstring(chunk))
> +local function_max_framesize = get_func()
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +-- Compile the parent trace first.
> +retf()
> +retf()
> +
> +-- Try to compile the side trace with a return to a lower frame
> +-- with a huge frame size.
> +function_max_framesize()
> +function_max_framesize()
> +
> +-- XXX: The limit check is OK with default defines for non-GC64
> +-- mode, the trace is compiled for it. The test fails only with
> +-- GC64 mode enabled. Still run the test for non-GC64 mode to
> +-- avoid regressions.
> +
> +test:ok(true, 'no assertion failure during recording')
> +
> +test:done(true)

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

[Sergey Kaplun via Tarantool-patches]

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

> > ---
> >
> > 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.

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()
+  ]]
 
 -- 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

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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
>

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

[Sergey Kaplun via Tarantool-patches]

Sergey,
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 13.03.24, Sergey Bronnikov wrote:
> Sergey,
> 
> LGTM with minor comment below.
> 
> On 3/13/24 12:37, Sergey Kaplun wrote:

<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
> > 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.

I've used such indentation to create a human-readable chunk:
| return function()
|   local r = retf()
|   uv.key, ..., uv_key = nil
| end
Instead of:
| return function()
|   local r = retf()
| uv.key, ..., uv_key = nil
| end

Since nobody wants to read this chunk as is, I've removed 2 spaces to
avoid confusion. 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 b454003e..cfaecbce 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
@@ -51,7 +51,7 @@ 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
===================================================================

> 
> >   
> >   -- 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

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

[Sergey Bronnikov via Tarantool-patches]

Sergey,

thanks for the fixes! LGTM

On 3/13/24 15:35, Sergey Kaplun wrote:
> Sergey,
> Thanks for the review!
> Fixed your comment and force-pushed the branch.
>
> On 13.03.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>> LGTM with minor comment below.
>>
>> On 3/13/24 12:37, Sergey Kaplun wrote:
> <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
>>> 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.
> I've used such indentation to create a human-readable chunk:
> | return function()
> |   local r = retf()
> |   uv.key, ..., uv_key = nil
> | end
> Instead of:
> | return function()
> |   local r = retf()
> | uv.key, ..., uv_key = nil
> | end
>
> Since nobody wants to read this chunk as is, I've removed 2 spaces to
> avoid confusion. 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 b454003e..cfaecbce 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
> @@ -51,7 +51,7 @@ 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
> ===================================================================
>
>>>    
>>>    -- 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>
>

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

[Maxim Kokryashkin via Tarantool-patches]

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
>
>  src/lj_record.c                               |  2 +
>  .../lj-1173-frame-limit-lower-frame.test.lua  | 83 +++++++++++++++++++
>  2 files changed, 85 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
>
> diff --git a/src/lj_record.c b/src/lj_record.c
> index c01c1f0b..e3590b1a 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
<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
> @@ -0,0 +1,83 @@
<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.
> +
> +-- 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.
> +
> +local get_func = assert(loadstring(chunk))
> +local function_max_framesize = get_func()
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +-- Compile the parent trace first.
> +retf()
> +retf()
> +
> +-- Try to compile the side trace with a return to a lower frame
> +-- with a huge frame size.
> +function_max_framesize()
> +function_max_framesize()
> +
> +-- XXX: The limit check is OK with default defines for non-GC64
> +-- mode, the trace is compiled for it. The test fails only with
> +-- GC64 mode enabled. Still run the test for non-GC64 mode to
> +-- avoid regressions.
> +
> +test:ok(true, 'no assertion failure during recording')
> +
> +test:done(true)
> --
> 2.44.0
>

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

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

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

[Sergey Kaplun via Tarantool-patches]

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.0 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/9832
[2]: https://github.com/tarantool/tarantool/pull/9833
[3]: https://github.com/tarantool/tarantool/pull/9834

-- 
Best regards,
Sergey Kaplun