[Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.
Sergey Kaplun
skaplun at tarantool.org
Mon Mar 28 13:02:38 MSK 2022
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?
| $ ../../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> 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
More information about the Tarantool-patches
mailing list