[Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.
sergos
sergos at tarantool.org
Wed Mar 30 13:02:08 MSK 2022
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 at 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 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
>
> 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 at tarantool.org <mailto:skaplun at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20220330/dbd4dc18/attachment.htm>
More information about the Tarantool-patches
mailing list