LGTM with a very small nit fix you’ve missed in the test comment.
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.| AbortedI'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 metamethodscall. 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 engineare occupied.This patch adds the corresponding check in `lj_record_call()`.Sergey Kaplun:* added the description and the test for the problemPart 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 conditionisn'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