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 24D8C9B9035; Tue, 12 Mar 2024 11:01:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 24D8C9B9035 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1710230504; bh=0MLWiNrsqrk4V2/YmzFthAntynymn4emN8dUH0zN70Q=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=zIwRlqk/G/o0SAAMu1FaUCC0rucmslQw/FFcKYMrbgeoF0OZFC2WCDeFigz4R4myl uB341cEbuTHUEeGwX7TFnaRlQxWef7XehuYAYUvKw/UhfcvxDt25p6isU5ApA+e6+p xtNYV/GfdBmz8KraR8SjkOcgjrm3s4uDU3SuPe6U= Received: from smtp3.i.mail.ru (smtp3.i.mail.ru [95.163.41.67]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E46529B9004 for ; Tue, 12 Mar 2024 11:01:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E46529B9004 Received: by smtp3.i.mail.ru with esmtpa (envelope-from ) id 1rjx4v-0000000H6Pn-3Pdh; Tue, 12 Mar 2024 11:01:42 +0300 Message-ID: Date: Tue, 12 Mar 2024 11:01:41 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org References: <20240312052627.21222-1-skaplun@tarantool.org> In-Reply-To: <20240312052627.21222-1-skaplun@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD987C0EE6E7F0A597D8D64E6F18B2E2E3258EF6A9BB0633F3C182A05F53808504073688C4DFEC4405089CEA9C911D09995C821D87C8CDD33A38B26E6E41588ACA4955D7F540D105623 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73C696014E2DCCA1EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A9545EB8D2325CAFEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD871E55263888A078C703F946947D79B4AF3629EC3CBE8A80CDCA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3ED8438A78DFE0A9E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C1D471462564A2E192E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F0A35B161A8BF67C1A91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5B20B0A1F0EFF1D515002B1117B3ED69659A007539E7F9E2C8B25839F35DFE037823CB91A9FED034534781492E4B8EEAD17AEC49845D0B908 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF22BD0125971207E292EE38FA1B1D58E4DF671D3A617C0CD2F6AC8497C2B9648631E81B06003A5D53748ED927723DE83F324AF77DBB6F23801163614F41252DB062A3DCC2F8A444DA5F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojWDseZ7hBpngVL+Z8b65CGQ== X-DA7885C5: 956806EEB6CCAEEFF255D290C0D534F905753A06FB772A1B37FF7CC8F88C27D67C64746FBAFE2F0C5B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE336BE0403E8DDD09D41A6AB7D240CFEA2EB2486F920520D90BEF86D5F70DA33880E41E8EF7A07863ECB274557F927329BE2DDF8182D28ACDB545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Check frame size limit before returning to a lower frame. 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey thanks for the patch! On 3/12/24 08:26, Sergey Kaplun wrote: > From: Mike Pall > > Thanks to Sergey Kaplun. > > (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa) > > When compiling a stitched (or side) trace, there is no check for the > frame size of the current prototype during recording. Hence, when we > return (for example, after stitching) to the lower frame with a maximum > possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249) > slot for GC64 mode may be used. This leads to the corresponding assertion > failure in `rec_check_slots()`. > > This patch adds the corresponding check. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#9595 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1173-frame-limit-lower-frame > Tarantool PR: https://github.com/tarantool/tarantool/pull/9791 > Related issues: > * https://github.com/tarantool/tarantool/issues/9595 > * https://github.com/LuaJIT/LuaJIT/issues/1173 > > src/lj_record.c | 2 + > .../lj-1173-frame-limit-lower-frame.test.lua | 83 +++++++++++++++++++ > 2 files changed, 85 insertions(+) > create mode 100644 test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > > diff --git a/src/lj_record.c b/src/lj_record.c > index c01c1f0b..e3590b1a 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -886,6 +886,8 @@ void lj_record_ret(jit_State *J, BCReg rbase, ptrdiff_t gotresults) > lj_trace_err(J, LJ_TRERR_LLEAVE); > } else if (J->needsnap) { /* Tailcalled to ff with side-effects. */ > lj_trace_err(J, LJ_TRERR_NYIRETL); /* No way to insert snapshot here. */ > + } else if (1 + pt->framesize >= LJ_MAX_JSLOTS) { > + lj_trace_err(J, LJ_TRERR_STACKOV); > } else { /* Return to lower frame. Guard for the target we return to. */ > TRef trpt = lj_ir_kgc(J, obj2gco(pt), IRT_PROTO); > TRef trpc = lj_ir_kptr(J, (void *)frame_pc(frame)); > diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > new file mode 100644 > index 00000000..91e2c603 > --- /dev/null > +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > @@ -0,0 +1,83 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT assertion failure during > +-- recording of side trace in GC64 mode with return to lower > +-- frame, which has the maximum possible frame size. > +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1173. > + > +local test = tap.test('lj-1173-frame-limit-lower-frame'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > + > +-- Parent trace with stitching and returning to a lower frame. > +local function retf() > + math.modf(1) > +end > +_G.retf = retf > + > +local LJ_MAX_JSLOTS = 250 I would say in a comment that constant is from . Your test depends on this constant (if it will be changed the test will test nothing), how to make sure that LJ_MAX_JSLOTS is equal to LJ_MAX_JSLOTS in ? > + > +-- Generate the following function: > +-- | local uv = {key = 1} > +-- | return function() > +-- | local r = retf() > +-- | uv.key, uv.key, --[[124 times in total ...]] uv.key = nil > +-- | end > +-- It will have the following bytecode: > +-- | 0001 GGET 0 0 ; "retf" > +-- | 0002 CALL 0 2 1 > +-- | 0003 UGET 1 0 ; uv > +-- | ... > +-- | 0126 UGET 124 0 ; uv > +-- | 0127 KNIL 125 248 > +-- | 0128 TSETS 248 124 1 ; "key" > +-- | ... > +-- | 0251 TSETS 125 1 1 ; "key" > +-- | 0252 RET0 0 1 > +-- As you can see, the 249 slots (from 0 to 248) are occupied in > +-- total. > +-- When we return to the lower frame for the side trace, we may > +-- hit the slot limit mentioned above: 2 slots are occupied > +-- by the frame (`baseslot`) and `KNIL` bytecode recording sets > +-- `maxslot` (the first free slot) to 249. Hence, the JIT slots > +-- are overflowing. > + > +local chunk = 'local uv = {key = 1}\n' > +chunk = chunk .. 'return function()\n' > +chunk = chunk .. 'local r = retf()\n' > + > +-- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. > +-- 1 slot is reserved (`r` variable), 1 pair is set outside the > +-- cycle. 249 slots (the maximum available amount, see > +-- , `bcreg_bump()` for details) are occupied in > +-- total. > +for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do > + chunk = chunk .. ('uv.key, ') > +end > +chunk = chunk .. 'uv.key = nil\n' > +chunk = chunk .. 'end\n' Why not to use multiline here and after the loop? > +local get_func = assert(loadstring(chunk)) > +local function_max_framesize = get_func() > + > +jit.opt.start('hotloop=1', 'hotexit=1') > + > +-- Compile the parent trace first. > +retf() > +retf() > + > +-- Try to compile the side trace with a return to a lower frame > +-- with a huge frame size. > +function_max_framesize() > +function_max_framesize() > + > +-- XXX: The limit check is OK with default defines for non-GC64 > +-- mode, the trace is compiled for it. The test fails only with > +-- GC64 mode enabled. Still run the test for non-GC64 mode to > +-- avoid regressions. > + > +test:ok(true, 'no assertion failure during recording') > + > +test:done(true)