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

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

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
>