From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 2C5476ECDC; Mon, 28 Mar 2022 13:04:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2C5476ECDC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1648461886; bh=IrfolNEEHvnzp0RK7d1Hu5gP0ATZhWPFlNJ27IH2kCw=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=D4o9UsBe/xc3BASzFyQ3gYW1LeDNk768fW/+odCfbE+yj8NzLlX2i/pGlHGNdPdrs LEhmyek9wUVMV0yw3xaUM/w08UpOwbMZdR62UvZBZqW5gSDd4fZKUNIJWD8Q2HEuuj fpsHVTcq/V+G+TkRHX5jCBbXs7Vo27vX727CZEr0= Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1CB686ECDC for ; Mon, 28 Mar 2022 13:04:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1CB686ECDC Received: by smtp40.i.mail.ru with esmtpa (envelope-from ) id 1nYmEt-0003Ny-64; Mon, 28 Mar 2022 13:04:43 +0300 Date: Mon, 28 Mar 2022 13:02:38 +0300 To: sergos Message-ID: References: <20211022114653.4225-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9BF7BFC6A11272BEBB40A9D11997E7DF99EC00CF38830761900894C459B0CD1B906D0C1769327E76D7ADD6E4A465BBFA510C84227987312B890F08FC34D31E8A7 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE774A7370C81A54524EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CCDA16D05354CC6E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89A056E9E21886A793E628B72DB65B5C6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE1B544F03EFBC4D57F41620B44FB51B7DD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3643FE6A0CAC512C7040F9FF01DFDA4A8C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063757B1FBEA53BC6EDBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 0D63561A33F958A542D5DBD21B609486A21FFF19F6AD5209CC463EFBC44E78B3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D9DC20663B80603F065EAA953180A3AB6202A775649FCFD0C0381D160CC50AF70CEB326640A049B01D7E09C32AA3244C469ED3A51CB74A0626372CC35A5ED5527101BF96129E4011FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojP0ngFsg3KoBGYr68jKdlew== X-Mailru-Sender: 583F1D7ACE8F49BD08E7C3B54344F62D42B7953803352AE3AD8B55A890456E302D75EF1566EC8B3F525762887713E5F1475755348978188EF9D3679FA3DE6E791CC59163FFD68303B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Add missing LJ_MAX_JSLOTS check. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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? | $ ../../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 wrote: > > > > From: Mike Pall 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