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 A9626A55B81; Wed, 21 Feb 2024 15:38:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A9626A55B81 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1708519120; bh=+x/DOA0cLX3ZZrw4wUzUgZAPrjpe9+7E+lY+mIOwGi4=; 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=H+o6hDWO8QD8rIyfBtvjLUiPb155YNY1UkFfmGqpjx2WeMZQWC1XdVnWTtc9d/RrU /Tkg3UM7LhlSrcICiERpvsr8CClyArSTqk7dORxXsMek+IvsFfsemYaxosmt8TlWRS nsLmJqXq8OBbiDPktB5Ihz5oOfpOYbAp4GBGMTh0= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [95.163.41.97]) (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 503B45EF42B for ; Wed, 21 Feb 2024 15:38:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 503B45EF42B Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1rclrx-00000008rIM-2n6f; Wed, 21 Feb 2024 15:38:38 +0300 Date: Wed, 21 Feb 2024 15:34:38 +0300 To: Maxim Kokryashkin Message-ID: References: <20240216111152.152025-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240216111152.152025-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9001F8F2F6BAD2021D47CBE561CA8C01FDA47252976C9176500894C459B0CD1B99DA0602672C996DADB4A27F0417D8D3759CDAF91358BAD4B632CB751B14A3B0BAB5D286C5C76880D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7364F8074C6DFACE2C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE774A7370C81A54524EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD87188538235488E7061F899DA127CCE88E3B117241FF4B52343A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE70F3DDF2BBF19B93A9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3E478A468B35FE767117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF905409F363357F83BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B690EF0235743AE48C76E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249D082881546D93491E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6AC294AFEFA671E80089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5DB662E43AD7DE27A5002B1117B3ED696DA0AFD3C1BEA5E9D3D2BBC1EF78EDEBE823CB91A9FED034534781492E4B8EEAD0AA277257C6A5E3DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFDAAD05A9918E0AC1F72A264691745CEE4A7F315480005255B48BD6B2A60F21E2212E5196DFD6A13FF1AD97FD9824DBCD14D1D16A39698729005469E95B471D8F75FC691FEA43DD995F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojhuvRynhHMboZpGoW6acmIw== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769BD699B7689EE23CDC7A4B249DE6549FDCA7CDECBB13CB19AB7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Throw any errors before stack changes in trace stitching. 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, Maxim! Thanks for the patch! Please consider my comments below. On 16.02.24, Maxim Kokryashkin wrote: > From: Mike Pall > > Thanks to doujiang24. > > (cherry-picked from commit 3f9389edc6cdf3f78a6896d550c236860aed62b2) > > The Lua stack is changed in the `lj_record_stop`, so if the trace Actually, it is changed in the `recff_stitch()` -- the continuation frame is inserted below a function with arguments. > is aborted, for example, when the `maxsnap` is reached, and an If there are any other situations, we must handle them too: 1) AFAICS, when LuaJIT is built with -DLUAJIT_ENABLE_TABLE_BUMP, the following lines may lead to the error, and thereby the Lua stack becomes unbalanced: `lj_record_stop()`: | if (J->retryrec) | lj_trace_err(J, LJ_TRERR_RETRY); Build with the following flags: | cmake -DCMAKE_C_FLAGS="-DLUAJIT_ENABLE_TABLE_BUMP" -DLUA_USE_APICHECK=ON \ | -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug \ | -DLUAJIT_ENABLE_GC64=OFF . && make -j The following script reproduces the issue (`maxsnap` configuration option is gone): | src/luajit -e " | local modf = math.modf | jit.opt.start('hotloop=1') | | local t1 | for i = 1, 2 do | t1 = {} | t1[i] = i | -- Forcify stitch. | modf(1.2) | end | " | LuaJIT ASSERT src/lj_dispatch.c:502: lj_dispatch_call: unbalanced stack after hot instruction 2) Also, it may happen when the memory error is raised when we try to reallocate the IR buffer for the next instruction in `lj_ir_nextins()`. The reproducer for this issue is much more tricky. My suggestions here are the following: Resolve the special case in the scope of backporting this patch, and report the 2 follow-ups with suggested solutions to the upstream. As I can see, this issue should be fixed in the following way: Add the `lj_vm_cpcall()` to the `record_stop()` call and rethrow the error, if needed, after clenup. Thoughts? > error is thrown from there, the execution continues with an > unbalanced stack. > > Since the only error that is thrown from the `lj_record_stop` > with modified stack is `LJ_TRERR_SNAPOV`, this patch adds a > check for snapshot overflow to the `recff_stitch`. > > Maxim Kokryashkin: > * added the description and the test for the problem > > Part of tarantool/tarantool#9595 > --- > Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-pr-720-errors-before-stitch > PR: https://github.com/tarantool/tarantool/pull/9703 > Issues: > https://github.com/tarantool/tarantool/issues/9595 > https://github.com/LuaJIT/LuaJIT/pull/720 > > src/lj_ffrecord.c | 4 ++++ > .../lj-pr-720-errors-before-stitch.test.lua | 20 +++++++++++++++++++ > 2 files changed, 24 insertions(+) > create mode 100644 test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua > > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c > index 1b2ddb72..e3ed80fb 100644 > --- a/src/lj_ffrecord.c > +++ b/src/lj_ffrecord.c > @@ -107,6 +107,10 @@ static void recff_stitch(jit_State *J) > const BCIns *pc = frame_pc(base-1); > TValue *pframe = frame_prevl(base-1); > > + /* Check for this now. Throwing in lj_record_stop messes up the stack. */ > + if (J->cur.nsnap >= (MSize)J->param[JIT_P_maxsnap]) > + lj_trace_err(J, LJ_TRERR_SNAPOV); > + > /* Move func + args up in Lua stack and insert continuation. */ > memmove(&base[1], &base[-1-LJ_FR2], sizeof(TValue)*nslot); > setframe_ftsz(nframe, ((char *)nframe - (char *)pframe) + FRAME_CONT); > diff --git a/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua > new file mode 100644 > index 00000000..53b7b5ea > --- /dev/null > +++ b/test/tarantool-tests/lj-pr-720-errors-before-stitch.test.lua I suggest to ommit the "pr-" part of the prefix. Since PR are treated the same way by Mike we don't need to mention it specifically. Also, as Sergey mentioned, GitHub redirects users to the PR page, so we can avoid specific mentioning. Side note-reminder (top priority of mentioning at the top): * lj-dddd stands for issues from LuaJIT/LuaJIT repository. * gh-dddd stands for issues from tarantool/tarantool repository. * or-dddd stands for issues from openresty/lua-resty-core or (since lack of them) openresty/luajit2 repository. > @@ -0,0 +1,20 @@ > +local tap = require('tap') > +local test = tap.test('lj-pr-720-errors-before-stitch'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > +test:plan(1) > + > +-- `math.modf` recording is NYI. > +local modf = require('math').modf There is no need to require math library since it is already loaded. > +jit.opt.start('hotloop=1', 'maxsnap=1') > + > +-- The loop has only two iterations: the first to detect its > +-- hotness and the second to record it. The snapshot limit is > +-- set to one and is certainly reached. > +for _ = 1, 2 do > + -- Forcify stitch. > + modf(1.2) > +end > + > +test:ok(true, 'stack is balanced') > +test:done(true) > -- > 2.43.0 > -- Best regards, Sergey Kaplun