[Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Yichun Zhang.

(cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)

Before the patch, JIT compiler doesn't check slots overflow for
recording of metamethods call. So the assertion in `rec_check_slots()`
checking that we don't overflow the slots limit (the limit
`LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod call
(`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots), while almost all
slots of JIT engine are occupied.

This patch adds the corresponding check in `lj_record_call()`.

Sergey Kaplun:
* added the description and the test for the problem
---

Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record

 src/lj_record.c                               |  2 +
 .../fix-slot-check-for-mm-record.test.lua     | 81 +++++++++++++++++++
 2 files changed, 83 insertions(+)
 create mode 100644 test/tarantool-tests/fix-slot-check-for-mm-record.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index 42af09e5..adf2370e 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -731,6 +731,8 @@ void lj_record_call(jit_State *J, BCReg func, ptrdiff_t nargs)
   J->framedepth++;
   J->base += func+1+LJ_FR2;
   J->baseslot += func+1+LJ_FR2;
+  if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
+    lj_trace_err(J, LJ_TRERR_STACKOV);
 }
 
 /* Record tail call. */
diff --git a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
new file mode 100644
index 00000000..e361830d
--- /dev/null
+++ b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
@@ -0,0 +1,81 @@
+local tap = require('tap')
+
+local test = tap.test('fix-slot-check-for-mm-record')
+test:plan(2)
+
+-- Before the patch, JIT compiler doesn't check slots overflow
+-- for recording of metamethods call. So the assertion checking
+-- that we don't overflow the slots limit (the limit
+-- `LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod
+-- call (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots),
+-- while almost all slots of JIT engine are occupied.
+
+-- Table with the simplest metamethod to call.
+local a0 = setmetatable({}, {
+  __add = function(t, arg1)
+    t[arg1] = arg1
+  end
+})
+_G.a0 = a0
+
+-- Fixarg function with call to metamethod.
+local function a1()
+  -- This constant is not setted as an upvalue to simplify stack
+  -- slots counting. Just remember that it is 42.
+  return a0 + 42
+end
+_G.a1 = a1
+
+-- Generate bunch of functions to call them recursively.
+-- Each function is a vararg function bumps slots on
+-- 2 (4) = 1 (2) * 2 for usual Lua frame and vararg frame
+-- recording for GC32 (GC64).
+for i = 2, 121 do
+  local f, err = load(('local r = a%d() return r'):format(i - 1))
+  assert(f, err)
+  _G['a'..i] = f
+end
+
+-- Trace is long enough, so we need to increase maxrecord.
+jit.opt.start('hotloop=1', 'maxrecord=2048')
+
+local function test_gc32()
+  -- 1 - Base slot.
+  -- 3 slots for cycle start, stop, step.
+  for _ = 1, 4 do
+    -- Occupy 1 slot for the function itself + 2 next slots will
+    -- occupied for a call to the vararg function.
+    -- Need 121 calls: 7 (baseslot after `a121()` is recorded)
+    -- + 119 * 2 + 1 (`a1` -- is not vararg function) = 246 slots.
+    -- The next call of metamethod in `a0` to record have 2 args
+    -- + 2 slots for metamethod function + 1 slot for frame.
+    -- luacheck: no global
+    a121()
+    assert(a0[42] == 42)
+    a0[42] = nil
+  end
+  return true
+end
+
+local function test_gc64()
+  -- 2 - Base slot.
+  -- 3 slots for cycle start, stop, step.
+  for _ = 1, 4 do
+    -- Occupy 1 slot for the function itself + 4 next slots will
+    -- occupied for a call to the vararg function.
+    -- Need 60 calls: 10 (baseslot after `a60()` is recorded)
+    -- + 58 * 4 + 2 (`a1` -- is not vararg function) = 244 slots.
+    -- The next call of metamethod in `a0` to record have 2 args
+    -- + 3 slots for metamethod function + 2 slots for frame.
+    -- luacheck: no global
+    a60()
+    assert(a0[42] == 42)
+    a0[42] = nil
+  end
+  return true
+end
+
+test:ok(test_gc32())
+test:ok(test_gc64())
+
+os.exit(test:check() and 0 or 1)
-- 
2.31.0

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!
Some nits in comments and I have exactly the same test result
with and without the patch:

root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit fix-slot-check-for-mm-record.test.lua 
TAP version 13
1..2
ok - nil
ok - nil

for the reference - GC64 is off:
root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit -e "print(require('ffi').abi('gc64'))"
false

Regards,
Sergos

> On 22 Oct 2021, at 14:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> Thanks to Yichun Zhang.
> 
> (cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)
> 
> Before the patch, JIT compiler doesn't check slots overflow for

Shall we put it like “before the patch”? We’re describing current behavior,
which is apparently without the patch. Then we can use simple present tense.

> recording of metamethods call. So the assertion in `rec_check_slots()`
> checking that we don't overflow the slots limit (the limit
> `LJ_MAX_JSLOTS` is 250) is failing,

Slots limit overflow assertion fails in `rec_check_slots()`

> when we record metamethod call
> (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots), while almost all
> slots of JIT engine are occupied.
> 

unlike the `lj_record_call()` (if I got it right?) during a metamethod 
recording.

> This patch adds the corresponding check in `lj_record_call()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> ---
> 
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> 
> src/lj_record.c                               |  2 +
> .../fix-slot-check-for-mm-record.test.lua     | 81 +++++++++++++++++++
> 2 files changed, 83 insertions(+)
> create mode 100644 test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> 
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 42af09e5..adf2370e 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -731,6 +731,8 @@ void lj_record_call(jit_State *J, BCReg func, ptrdiff_t nargs)
>   J->framedepth++;
>   J->base += func+1+LJ_FR2;
>   J->baseslot += func+1+LJ_FR2;
> +  if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
> +    lj_trace_err(J, LJ_TRERR_STACKOV);
> }
> 
> /* Record tail call. */
> diff --git a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> new file mode 100644
> index 00000000..e361830d
> --- /dev/null
> +++ b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> @@ -0,0 +1,81 @@
> +local tap = require('tap')
> +
> +local test = tap.test('fix-slot-check-for-mm-record')
> +test:plan(2)
> +
> +-- Before the patch, JIT compiler doesn't check slots overflow
> +-- for recording of metamethods call. So the assertion checking
> +-- that we don't overflow the slots limit (the limit
> +-- `LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod
> +-- call (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots),
> +-- while almost all slots of JIT engine are occupied.
> +
> +-- Table with the simplest metamethod to call.
> +local a0 = setmetatable({}, {
> +  __add = function(t, arg1)
> +    t[arg1] = arg1
> +  end
> +})
> +_G.a0 = a0
> +
> +-- Fixarg function with call to metamethod.
> +local function a1()
> +  -- This constant is not setted as an upvalue to simplify stack
                             ^^^^^^ set
> +  -- slots counting. Just remember that it is 42.
> +  return a0 + 42
> +end
> +_G.a1 = a1
> +
> +-- Generate bunch of functions to call them recursively.
> +-- Each function is a vararg function bumps slots on
> +-- 2 (4) = 1 (2) * 2 for usual Lua frame and vararg frame
> +-- recording for GC32 (GC64).
> +for i = 2, 121 do
> +  local f, err = load(('local r = a%d() return r'):format(i - 1))
> +  assert(f, err)
> +  _G['a'..i] = f
> +end
> +
> +-- Trace is long enough, so we need to increase maxrecord.
> +jit.opt.start('hotloop=1', 'maxrecord=2048')
> +
> +local function test_gc32()
> +  -- 1 - Base slot.
> +  -- 3 slots for cycle start, stop, step.
> +  for _ = 1, 4 do
> +    -- Occupy 1 slot for the function itself + 2 next slots will
> +    -- occupied for a call to the vararg function.
> +    -- Need 121 calls: 7 (baseslot after `a121()` is recorded)
> +    -- + 119 * 2 + 1 (`a1` -- is not vararg function) = 246 slots.
> +    -- The next call of metamethod in `a0` to record have 2 args
> +    -- + 2 slots for metamethod function + 1 slot for frame.
> +    -- luacheck: no global
> +    a121()
> +    assert(a0[42] == 42)
> +    a0[42] = nil
> +  end
> +  return true
> +end
> +
> +local function test_gc64()
> +  -- 2 - Base slot.
> +  -- 3 slots for cycle start, stop, step.
> +  for _ = 1, 4 do
> +    -- Occupy 1 slot for the function itself + 4 next slots will
> +    -- occupied for a call to the vararg function.
> +    -- Need 60 calls: 10 (baseslot after `a60()` is recorded)
> +    -- + 58 * 4 + 2 (`a1` -- is not vararg function) = 244 slots.
> +    -- The next call of metamethod in `a0` to record have 2 args
> +    -- + 3 slots for metamethod function + 2 slots for frame.
> +    -- luacheck: no global
> +    a60()
> +    assert(a0[42] == 42)
> +    a0[42] = nil
> +  end
> +  return true
> +end
> +
> +test:ok(test_gc32())
> +test:ok(test_gc64())
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.31.0
> 

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 25.03.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> Some nits in comments and I have exactly the same test result
> with and without the patch:
> 
> root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit fix-slot-check-for-mm-record.test.lua 
> TAP version 13
> 1..2
> ok - nil
> ok - nil
> 
> for the reference - GC64 is off:
> root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit -e "print(require('ffi').abi('gc64'))"
> false

Have you enabled assertions in debug build?

| $ ../../src/luajit fix-slot-check-for-mm-record.test.lua
| TAP version 13
| 1..2
| luajit: /home/burii/builds_workspace/luajit/fix-slot-check-for-mm-record/src/lj_record.c:92: rec_check_slots: Assertion `nslots < 250' failed.
| Aborted

I've done this as the following:
| $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF && make -j

> 
> Regards,
> Sergos
> 
> > On 22 Oct 2021, at 14:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>

I reformulated commit message as you suggested:

===================================================================
Add missing LJ_MAX_JSLOTS check.

Thanks to Yichun Zhang.

(cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)

JIT compiler doesn't check slots overflow for recording of metamethods
call. Slots limit (`LJ_MAX_JSLOTS` is 250) overflow assertion fails in
`rec_check_slots()` when we record metamethod call (`J->baseslot` diff +
`J->maxslot` ~ 5-8 stack slots), while almost all slots of JIT engine
are occupied.

This patch adds the corresponding check in `lj_record_call()`.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#6548
===================================================================

Branch is force pushed.

> > 
> > Thanks to Yichun Zhang.
> > 
> > (cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)
> > 
> > Before the patch, JIT compiler doesn't check slots overflow for
> 
> Shall we put it like “before the patch”? We’re describing current behavior,
> which is apparently without the patch. Then we can use simple present tense.

Fixed.

> 
> > recording of metamethods call. So the assertion in `rec_check_slots()`
> > checking that we don't overflow the slots limit (the limit
> > `LJ_MAX_JSLOTS` is 250) is failing,
> 
> Slots limit overflow assertion fails in `rec_check_slots()`

Fixed.

> 
> > when we record metamethod call
> > (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots), while almost all
> > slots of JIT engine are occupied.
> > 
> 
> unlike the `lj_record_call()` (if I got it right?) during a metamethod 
> recording.

We pass to `lj_record_call()` from any metamethod and this condition
isn't checked.

> 
> > This patch adds the corresponding check in `lj_record_call()`.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > ---
> > 
> > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> > 
> > src/lj_record.c                               |  2 +
> > .../fix-slot-check-for-mm-record.test.lua     | 81 +++++++++++++++++++
> > 2 files changed, 83 insertions(+)
> > create mode 100644 test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> > 
> > diff --git a/src/lj_record.c b/src/lj_record.c
> > index 42af09e5..adf2370e 100644
> > --- a/src/lj_record.c
> > +++ b/src/lj_record.c
> > @@ -731,6 +731,8 @@ void lj_record_call(jit_State *J, BCReg func, ptrdiff_t nargs)
> >   J->framedepth++;
> >   J->base += func+1+LJ_FR2;
> >   J->baseslot += func+1+LJ_FR2;
> > +  if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
> > +    lj_trace_err(J, LJ_TRERR_STACKOV);
> > }
> > 
> > /* Record tail call. */
> > diff --git a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> > new file mode 100644
> > index 00000000..e361830d
> > --- /dev/null
> > +++ b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> > @@ -0,0 +1,81 @@
> > +local tap = require('tap')
> > +
> > +local test = tap.test('fix-slot-check-for-mm-record')
> > +test:plan(2)
> > +
> > +-- Before the patch, JIT compiler doesn't check slots overflow
> > +-- for recording of metamethods call. So the assertion checking
> > +-- that we don't overflow the slots limit (the limit
> > +-- `LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod
> > +-- call (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots),
> > +-- while almost all slots of JIT engine are occupied.
> > +
> > +-- Table with the simplest metamethod to call.
> > +local a0 = setmetatable({}, {
> > +  __add = function(t, arg1)
> > +    t[arg1] = arg1
> > +  end
> > +})
> > +_G.a0 = a0
> > +
> > +-- Fixarg function with call to metamethod.
> > +local function a1()
> > +  -- This constant is not setted as an upvalue to simplify stack
>                              ^^^^^^ set
> > +  -- slots counting. Just remember that it is 42.
> > +  return a0 + 42
> > +end
> > +_G.a1 = a1
> > +
> > +-- Generate bunch of functions to call them recursively.
> > +-- Each function is a vararg function bumps slots on
> > +-- 2 (4) = 1 (2) * 2 for usual Lua frame and vararg frame
> > +-- recording for GC32 (GC64).
> > +for i = 2, 121 do
> > +  local f, err = load(('local r = a%d() return r'):format(i - 1))
> > +  assert(f, err)
> > +  _G['a'..i] = f
> > +end
> > +
> > +-- Trace is long enough, so we need to increase maxrecord.
> > +jit.opt.start('hotloop=1', 'maxrecord=2048')
> > +
> > +local function test_gc32()
> > +  -- 1 - Base slot.
> > +  -- 3 slots for cycle start, stop, step.
> > +  for _ = 1, 4 do
> > +    -- Occupy 1 slot for the function itself + 2 next slots will
> > +    -- occupied for a call to the vararg function.
> > +    -- Need 121 calls: 7 (baseslot after `a121()` is recorded)
> > +    -- + 119 * 2 + 1 (`a1` -- is not vararg function) = 246 slots.
> > +    -- The next call of metamethod in `a0` to record have 2 args
> > +    -- + 2 slots for metamethod function + 1 slot for frame.
> > +    -- luacheck: no global
> > +    a121()
> > +    assert(a0[42] == 42)
> > +    a0[42] = nil
> > +  end
> > +  return true
> > +end
> > +
> > +local function test_gc64()
> > +  -- 2 - Base slot.
> > +  -- 3 slots for cycle start, stop, step.
> > +  for _ = 1, 4 do
> > +    -- Occupy 1 slot for the function itself + 4 next slots will
> > +    -- occupied for a call to the vararg function.
> > +    -- Need 60 calls: 10 (baseslot after `a60()` is recorded)
> > +    -- + 58 * 4 + 2 (`a1` -- is not vararg function) = 244 slots.
> > +    -- The next call of metamethod in `a0` to record have 2 args
> > +    -- + 3 slots for metamethod function + 2 slots for frame.
> > +    -- luacheck: no global
> > +    a60()
> > +    assert(a0[42] == 42)
> > +    a0[42] = nil
> > +  end
> > +  return true
> > +end
> > +
> > +test:ok(test_gc32())
> > +test:ok(test_gc64())
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.31.0
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 7936 bytes --]

Hi! Thanks for updates!

LGTM with a very small nit fix you’ve missed in the test comment.

Sergos


> On 28 Mar 2022, at 13:02, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 25.03.22, sergos wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> Some nits in comments and I have exactly the same test result
>> with and without the patch:
>> 
>> root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit fix-slot-check-for-mm-record.test.lua 
>> TAP version 13
>> 1..2
>> ok - nil
>> ok - nil
>> 
>> for the reference - GC64 is off:
>> root@dev1:/workspaces/t.sergos/third_party/luajit/test/tarantool-tests# ../../src/luajit -e "print(require('ffi').abi('gc64'))"
>> false
> 
> Have you enabled assertions in debug build?
> 
My bad, now it fails w/o patch. thanks!

> | $ ../../src/luajit fix-slot-check-for-mm-record.test.lua
> | TAP version 13
> | 1..2
> | luajit: /home/burii/builds_workspace/luajit/fix-slot-check-for-mm-record/src/lj_record.c:92: rec_check_slots: Assertion `nslots < 250' failed.
> | Aborted
> 
> I've done this as the following:
> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF && make -j
> 
>> 
>> Regards,
>> Sergos
>> 
>>> On 22 Oct 2021, at 14:46, Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>> wrote:
>>> 
>>> From: Mike Pall <mike>
> 
> I reformulated commit message as you suggested:
> 
> ===================================================================
> Add missing LJ_MAX_JSLOTS check.
> 
> Thanks to Yichun Zhang.
> 
> (cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)
> 
> JIT compiler doesn't check slots overflow for recording of metamethods
> call. Slots limit (`LJ_MAX_JSLOTS` is 250) overflow assertion fails in
> `rec_check_slots()` when we record metamethod call (`J->baseslot` diff +
> `J->maxslot` ~ 5-8 stack slots), while almost all slots of JIT engine
> are occupied.
> 
> This patch adds the corresponding check in `lj_record_call()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#6548
> ===================================================================
> 
> Branch is force pushed.
> 
>>> 
>>> Thanks to Yichun Zhang.
>>> 
>>> (cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)
>>> 
>>> Before the patch, JIT compiler doesn't check slots overflow for
>> 
>> Shall we put it like “before the patch”? We’re describing current behavior,
>> which is apparently without the patch. Then we can use simple present tense.
> 
> Fixed.
> 
>> 
>>> recording of metamethods call. So the assertion in `rec_check_slots()`
>>> checking that we don't overflow the slots limit (the limit
>>> `LJ_MAX_JSLOTS` is 250) is failing,
>> 
>> Slots limit overflow assertion fails in `rec_check_slots()`
> 
> Fixed.
> 
>> 
>>> when we record metamethod call
>>> (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots), while almost all
>>> slots of JIT engine are occupied.
>>> 
>> 
>> unlike the `lj_record_call()` (if I got it right?) during a metamethod 
>> recording.
> 
> We pass to `lj_record_call()` from any metamethod and this condition
> isn't checked.
> 
>> 
>>> This patch adds the corresponding check in `lj_record_call()`.
>>> 
>>> Sergey Kaplun:
>>> * added the description and the test for the problem
>>> ---
>>> 
>>> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
>>> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
>>> 
>>> src/lj_record.c                               |  2 +
>>> .../fix-slot-check-for-mm-record.test.lua     | 81 +++++++++++++++++++
>>> 2 files changed, 83 insertions(+)
>>> create mode 100644 test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
>>> 
>>> diff --git a/src/lj_record.c b/src/lj_record.c
>>> index 42af09e5..adf2370e 100644
>>> --- a/src/lj_record.c
>>> +++ b/src/lj_record.c
>>> @@ -731,6 +731,8 @@ void lj_record_call(jit_State *J, BCReg func, ptrdiff_t nargs)
>>>  J->framedepth++;
>>>  J->base += func+1+LJ_FR2;
>>>  J->baseslot += func+1+LJ_FR2;
>>> +  if (J->baseslot + J->maxslot >= LJ_MAX_JSLOTS)
>>> +    lj_trace_err(J, LJ_TRERR_STACKOV);
>>> }
>>> 
>>> /* Record tail call. */
>>> diff --git a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
>>> new file mode 100644
>>> index 00000000..e361830d
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
>>> @@ -0,0 +1,81 @@
>>> +local tap = require('tap')
>>> +
>>> +local test = tap.test('fix-slot-check-for-mm-record')
>>> +test:plan(2)
>>> +
>>> +-- Before the patch, JIT compiler doesn't check slots overflow
>>> +-- for recording of metamethods call. So the assertion checking
>>> +-- that we don't overflow the slots limit (the limit
>>> +-- `LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod
>>> +-- call (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots),
>>> +-- while almost all slots of JIT engine are occupied.
>>> +
>>> +-- Table with the simplest metamethod to call.
>>> +local a0 = setmetatable({}, {
>>> +  __add = function(t, arg1)
>>> +    t[arg1] = arg1
>>> +  end
>>> +})
>>> +_G.a0 = a0
>>> +
>>> +-- Fixarg function with call to metamethod.
>>> +local function a1()
>>> +  -- This constant is not setted as an upvalue to simplify stack
>>                             ^^^^^^ set

Didn’t mention in the update. Please, proceed.

>>> +  -- slots counting. Just remember that it is 42.
>>> +  return a0 + 42
>>> +end
>>> +_G.a1 = a1
>>> +
>>> +-- Generate bunch of functions to call them recursively.
>>> +-- Each function is a vararg function bumps slots on
>>> +-- 2 (4) = 1 (2) * 2 for usual Lua frame and vararg frame
>>> +-- recording for GC32 (GC64).
>>> +for i = 2, 121 do
>>> +  local f, err = load(('local r = a%d() return r'):format(i - 1))
>>> +  assert(f, err)
>>> +  _G['a'..i] = f
>>> +end
>>> +
>>> +-- Trace is long enough, so we need to increase maxrecord.
>>> +jit.opt.start('hotloop=1', 'maxrecord=2048')
>>> +
>>> +local function test_gc32()
>>> +  -- 1 - Base slot.
>>> +  -- 3 slots for cycle start, stop, step.
>>> +  for _ = 1, 4 do
>>> +    -- Occupy 1 slot for the function itself + 2 next slots will
>>> +    -- occupied for a call to the vararg function.
>>> +    -- Need 121 calls: 7 (baseslot after `a121()` is recorded)
>>> +    -- + 119 * 2 + 1 (`a1` -- is not vararg function) = 246 slots.
>>> +    -- The next call of metamethod in `a0` to record have 2 args
>>> +    -- + 2 slots for metamethod function + 1 slot for frame.
>>> +    -- luacheck: no global
>>> +    a121()
>>> +    assert(a0[42] == 42)
>>> +    a0[42] = nil
>>> +  end
>>> +  return true
>>> +end
>>> +
>>> +local function test_gc64()
>>> +  -- 2 - Base slot.
>>> +  -- 3 slots for cycle start, stop, step.
>>> +  for _ = 1, 4 do
>>> +    -- Occupy 1 slot for the function itself + 4 next slots will
>>> +    -- occupied for a call to the vararg function.
>>> +    -- Need 60 calls: 10 (baseslot after `a60()` is recorded)
>>> +    -- + 58 * 4 + 2 (`a1` -- is not vararg function) = 244 slots.
>>> +    -- The next call of metamethod in `a0` to record have 2 args
>>> +    -- + 3 slots for metamethod function + 2 slots for frame.
>>> +    -- luacheck: no global
>>> +    a60()
>>> +    assert(a0[42] == 42)
>>> +    a0[42] = nil
>>> +  end
>>> +  return true
>>> +end
>>> +
>>> +test:ok(test_gc32())
>>> +test:ok(test_gc64())
>>> +
>>> +os.exit(test:check() and 0 or 1)
>>> -- 
>>> 2.31.0
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 46241 bytes --]

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Sergey Kaplun via Tarantool-patches]

Hi!

Thanks for the review!

On 30.03.22, sergos wrote:
> Hi! Thanks for updates!
> 
> LGTM with a very small nit fix you’ve missed in the test comment.

Fixed. Branch is force pushed.

===================================================================
diff --git a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
index e361830d..94a27d18 100644
--- a/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
+++ b/test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
@@ -20,7 +20,7 @@ _G.a0 = a0

 -- Fixarg function with call to metamethod.
 local function a1()
-  -- This constant is not setted as an upvalue to simplify stack
+  -- This constant is not set as an upvalue to simplify stack
   -- slots counting. Just remember that it is 42.
   return a0 + 42
 end
===================================================================

> 
> Sergos
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, but I've rewritten the test a bit to not run
both tests for a single build. You can find the changes within this
commit[1] and I proceed with the patch if you're OK with it.

[1]: https://github.com/tarantool/luajit/commit/fe6d486

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Sergey Kaplun via Tarantool-patches]

Igor,

Thanks for the review and fixup, it LGTM.

On 27.06.22, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! LGTM, but I've rewritten the test a bit to not run
> both tests for a single build. You can find the changes within this
> commit[1] and I proceed with the patch if you're OK with it.
> 
> [1]: https://github.com/tarantool/luajit/commit/fe6d486
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 22.10.21, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Thanks to Yichun Zhang.
> 
> (cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)
> 
> Before the patch, JIT compiler doesn't check slots overflow for
> recording of metamethods call. So the assertion in `rec_check_slots()`
> checking that we don't overflow the slots limit (the limit
> `LJ_MAX_JSLOTS` is 250) is failing, when we record metamethod call
> (`J->baseslot` diff + `J->maxslot` ~ 5-8 stack slots), while almost all
> slots of JIT engine are occupied.
> 
> This patch adds the corresponding check in `lj_record_call()`.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> ---
> 
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-slot-check-for-mm-record
> 
>  src/lj_record.c                               |  2 +
>  .../fix-slot-check-for-mm-record.test.lua     | 81 +++++++++++++++++++
>  2 files changed, 83 insertions(+)
>  create mode 100644 test/tarantool-tests/fix-slot-check-for-mm-record.test.lua
> 

<snipped>

> -- 
> 2.31.0
> 

-- 
Best regards,
IM