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

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

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