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

sergos sergos at tarantool.org
Fri Mar 25 18:51:10 MSK 2022


Hi!

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

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



More information about the Tarantool-patches mailing list