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 A377A5C2DB1; Mon, 28 Aug 2023 18:00:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A377A5C2DB1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693234810; bh=RjXm/CO+E+OKKIj+x+t3O5te5HpQC0TepDF2iCrmbtA=; 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=HNfGT162wNCe8dztRYsBPbAGaAs2cE+3tn69iQczS2NnUmYIuAB4iu2X6DQ4ZzqOE 3te6xmoowxdfHWiy0rsLXG6G8Dq+tB1LN4P+WE+NqVsM/Q6X8kA5NpqbWPizlQUHSr L6dujKCnUxYzNujvKCTllM49ft5nrR8c98wHU4Q8= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (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 414D15C2D87 for ; Mon, 28 Aug 2023 18:00:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 414D15C2D87 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1qadiq-0002Gu-29; Mon, 28 Aug 2023 18:00:08 +0300 Date: Mon, 28 Aug 2023 18:00:08 +0300 To: Sergey Kaplun Message-ID: References: <20230825150024.23247-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230825150024.23247-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9954329A9C7AF96E91A4746D44FCF77C37A10F1759BD918B9182A05F5380850402A77A6CE461396E1A32730A2083BAD4939A12063AEFD4DC33372D2AB08F15A02 X-C1DE0DAB: 0D63561A33F958A524D5C1B883F9ED5FAE7D88BDE846A80DEB92124CBCF2EAACF87CCE6106E1FC07E67D4AC08A07B9B0A816C540FC8EEC30CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527F50395B76EBB29D91D4B33DD5313FCA9985C9D70D96CD06373ED530D659B2616700E9F3DC893C157B37FD76D11AF80A0DC7AC6D68FC7F5403521EE9A1C660BD04EA455F16B58544A2557BDE0DD54B3590965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojrC6BcuK2ROzP78HrBLlphg== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE40703F6C348B207B01B72F77BB008F0C44B13232A9A0118815CD51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_TSETM. 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! Please consider my comments below. On Fri, Aug 25, 2023 at 06:00:24PM +0300, Sergey Kaplun wrote: > From: Mike Pall > > Analyzed by Sergey Kaplun. > > (cherry-picked from commit 0cc5fdfbc0810073485150eb184dc358dab507d9) > > Recording of the `BC_TSETM` bytecode may keep too optimistic JIT > maxslot. In that case, the slot above the top of the Lua stack may be Typo: s/too optimistic JIT maxslot./the JIT maxslot too optimistic./ > considered used. When any VM event handler is called before the > recording of the next instruction, this leads to an assertion failure in > `rec_check_slots()`. > > This patch sets the `ra` as a maxslot, as far as the `ra` - 1 contains a > table, which is always the highest slot after this bytecode. Also, it > adds an assertion that we check slots below the top of the Lua stack. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1025-tsetm-maxslot > Tarantool PR: https://github.com/tarantool/tarantool/pull/9040 > Issues: > * https://github.com/LuaJIT/LuaJIT/issues/1025 > * https://github.com/tarantool/tarantool/issues/8825 > > src/lj_record.c | 2 + > .../lj-1025-tsetm-maxslot.test.lua | 52 +++++++++++++++++++ > 2 files changed, 54 insertions(+) > create mode 100644 test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > > diff --git a/src/lj_record.c b/src/lj_record.c > index 34d1210a..58b040ec 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -115,6 +115,7 @@ static void rec_check_slots(jit_State *J) > cTValue *tv = &base[s]; > IRRef ref = tref_ref(tr); > IRIns *ir = NULL; /* Silence compiler. */ > + lj_assertJ(tv < J->L->top, "slot %d above top of Lua stack", s); > if (!LJ_FR2 || ref || !(tr & (TREF_FRAME | TREF_CONT))) { > lj_assertJ(ref >= J->cur.nk && ref < J->cur.nins, > "slot %d ref %04d out of range", s, ref - REF_BIAS); > @@ -2342,6 +2343,7 @@ void lj_record_ins(jit_State *J) > > case BC_TSETM: > rec_tsetm(J, ra, (BCReg)(J->L->top - J->L->base), (int32_t)rcv->u32.lo); > + J->maxslot = ra; /* The table slot at ra-1 is the highest used slot. */ > break; > > case BC_TNEW: > diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > new file mode 100644 > index 00000000..7ae0a99d > --- /dev/null > +++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > @@ -0,0 +1,52 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT incorrect recording of `TSETM` > +-- bytecode. > +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1025. > + > +local test = tap.test('lj-1025-tsetm-maxslot'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > + > +local jit_dump = require('jit.dump') > + > +local TEST_VALUE = '5' > +local TEST_IDX = 5 > + > +local function slot5() > + return nil, nil, nil, nil, TEST_VALUE > +end Why the fifth slot? Drop a comment. > + > +local storage > +local function test_tsetm(...) > + -- Usage of `TSETM` bytecode. > + storage = {slot5()} > + -- Use this function again to trick use-def analysis and avoid > + -- cleaning JIT slots, so the last JIT slot contains > + -- `TEST_VALUE`. > + return slot5(...) > +end > + > +-- Wrapper to avoid the recording of just the inner `slot5()` > +-- function. > +local function wrap() > + test_tsetm() > +end > + > +jit.opt.start('hotloop=1') > +-- We need to call the VM event handler after each recorded bytecode > +-- instruction to pollute the Lua stack and the issue > +-- becomes observable. Typo: s/and the issue becomes/and make the issue/ > +jit_dump.start('b', '/dev/null') > + > +-- Compile and execute the trace with `TSETM`. > +wrap() > +wrap() > +wrap() > + > +test:is(storage[TEST_IDX], TEST_VALUE, > + 'BC_TSETM recording with enabled jit.dump') > + > +test:done(true) > -- > 2.41.0 >