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

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

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