Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check.
@ 2021-10-22 11:46 Sergey Kaplun via Tarantool-patches
  2022-03-25 15:51 ` sergos via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-10-22 11:46 UTC (permalink / raw)
  To: Sergey Ostanevich, Igor Munkin; +Cc: tarantool-patches

From: Mike Pall <mike>

Thanks to Yichun Zhang.

(cherry picked from commit 630ff3196a06353c6a7ccd1e9ac3958f4a8ca13c)

Before the patch, JIT compiler doesn't check slots overflow for
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, 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
---

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
+  -- 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-06-30 12:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 11:46 [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check Sergey Kaplun via Tarantool-patches
2022-03-25 15:51 ` sergos via Tarantool-patches
2022-03-28 10:02   ` Sergey Kaplun via Tarantool-patches
2022-03-30 10:02     ` sergos via Tarantool-patches
2022-04-06  7:21       ` Sergey Kaplun via Tarantool-patches
2022-06-27 19:58 ` Igor Munkin via Tarantool-patches
2022-06-28  6:39   ` Sergey Kaplun via Tarantool-patches
2022-06-30 12:08 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox