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 B42FC5C4548; Mon, 28 Aug 2023 18:16:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B42FC5C4548 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1693235807; bh=DWRspF5LIFXiFwfHN63Bjv3NkGOttr0zPWLc0MD8jcs=; 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=aaQJ1oLEHfJfB/j5J++v5USPiZW0iUGyz1YGE/Zeo8WMZLogXxoCWkMAcKbQdmS9c W26zV4zkndFyZWqqgjH9yiIPVxl+yGEPAW6/4xWzv0q9+6uL89HS0+87pOiAMV4OVO TsIygsqnF1qm3WAfrvxLxkz5UI9MBugSTCFsNH6Y= Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [95.163.41.85]) (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 C74AD57B48E for ; Mon, 28 Aug 2023 18:16:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C74AD57B48E Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1qadyv-0071q3-2r; Mon, 28 Aug 2023 18:16:46 +0300 Date: Mon, 28 Aug 2023 18:16:45 +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: X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9954329A9C7AF96E9FF7E5CBDFDEDE4BE8F45FAF46EF4763D182A05F538085040ACDB228DA40EBEDDAD23C4D77C5E6C22772E0AF019253CC6B8A3C3B5D0D2339D X-C1DE0DAB: 0D63561A33F958A5C991FE3F4F4EF7A39AB17D9518300BD662F2D3617429C4BFF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742DC8270968E61249B1004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34047E322BFAFD69BD000699C567B08C6BE9EA0F6E1EAA2DCC09CBDB08B1BA5A931D4A0FC789C9CDAA1D7E09C32AA3244C4F1CC418D984F8DC32A7569EC301701F3A76366E8A9DE7CA85A42E4C463514DC5DA084F8E80FEBD396F07DFE06A4A8314E894E437E78228B66933FA05BD8EF0CAD958392AE682691 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojrC6BcuK2ROzn6TDZG17Baw== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407C5C164ACDCBFB6D59A18FBE0C185184A5699D59FAAE899E3D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 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 fixes! LGTM, except for a few more comments below. On Mon, Aug 28, 2023 at 06:02:12PM +0300, Sergey Kaplun wrote: > Hi, Maxim! > Thanks for the review! > I've updated the patch considering your comments and force-pushed the > branch. > > On 28.08.23, Maxim Kokryashkin wrote: > > 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./ > > Fixed. > > > > 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 > > > --- > > > > > > + > > > +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. > > Fixed. See the iterative patch below. > > > > + > > > +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/ > > Fixed. > > See the iterative patch below: > > =================================================================== > diff --git a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > index 7ae0a99d..74625a79 100644 > --- a/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > +++ b/test/tarantool-tests/lj-1025-tsetm-maxslot.test.lua > @@ -15,6 +15,8 @@ local jit_dump = require('jit.dump') > local TEST_VALUE = '5' > local TEST_IDX = 5 > > +-- XXX: Use big enough slot numbewr to be overwritten by VM event Typo: s/Use/Use a/ Typo: s/numbewr/number/ Typo: s/by VM/by the VM/ > +-- handler function. Please also add that this is an empirical number. > local function slot5() > return nil, nil, nil, nil, TEST_VALUE > end > @@ -36,9 +38,9 @@ local function wrap() > 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. > +-- We need to call the VM event handler after each recorded > +-- bytecode instruction to pollute the Lua stack and make the > +-- issue observable. > jit_dump.start('b', '/dev/null') > > -- Compile and execute the trace with `TSETM`. > =================================================================== > > > > +jit_dump.start('b', '/dev/null') > > > + > > > > > > -- > > > 2.41.0 > > > > > -- > Best regards, > Sergey Kaplun