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 6E02B6C196A; Wed, 18 Oct 2023 17:59:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6E02B6C196A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1697641179; bh=XqZMGB0EUgSEr/Sr3GWZwamIwoYLJq9gMTyuSSAbmTM=; 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=TQJsXDMgAqizHhkl4xTLrND04huDHF4F8EKKd9V6+4u4ywoBhPzbcMIsYC/89OOE4 yt2nrNEs01Jqn58w8cIt6N0eqN8k2Y7hwNJS4JjSIs21l03KjWOY920gZQrIFg3vfD UPu5b/eXpWmZkBvjl6vWB90EsFR4ANR50Y20ltB8= Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [95.163.41.81]) (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 563F26BA5BA for ; Wed, 18 Oct 2023 17:59:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 563F26BA5BA Received: by smtp40.i.mail.ru with esmtpa (envelope-from ) id 1qt81I-00Ccqz-1V; Wed, 18 Oct 2023 17:59:36 +0300 Message-ID: <42ce4b41-ac2c-4a47-bee0-18b61682ab1d@tarantool.org> Date: Wed, 18 Oct 2023 17:59:36 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Maxim Kokryashkin , Sergey Bronnikov Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org References: <8d9d59bf865fe5764a3c91d5a363f7e2bc78a348.1697115768.git.sergeyb@tarantool.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD91CE3BA418E3A67288A53B4037B9466AAA7FB66616A68686F182A05F5380850404C228DA9ACA6FE27FDC74D4C62E32EBA77111EFC8D10AB418140EBBF31C483198A521973878864A4 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EB05B7739F1E6D56EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063739E242D0556ED1DD8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B984D5284EB4872EC6D78EAF81B57381117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE9647ADFADE5905B11AB2475877E8919AD8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE34CB6874B0BCFF0B8040F9FF01DFDA4A8C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C78444BBB7636F62A2E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F9FFED5BD9FB41755A91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A50EF4D91B180770FA85FCFD29E069EB34298C00A86472F1B0F87CCE6106E1FC07E67D4AC08A07B9B0A816C540FC8EEC30CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFAE9B0D673827F4E6D4BFEC7F7AD4A00715426078DB3616C0D736B06F8D7E88ECC7834276CD0523975BB39EF8E370AEAD3E6C2B062C6E1FF5DAAE606F2EF03141A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj+FeGWQZfdZJec6bXkML0kg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769DF85608F1ACE6BB377111EFC8D10AB41EC9A243AB1C8B1BCEBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Always snapshot functions for non-base frames. 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, Max On 10/17/23 17:56, Maxim Kokryashkin via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On Thu, Oct 12, 2023 at 04:06:25PM +0300, Sergey Bronnikov via Tarantool-patches wrote: >> From: Sergey Bronnikov >> >> Reported by Arseny Vakhrushev. >> Analysis and fix contributed by Peter Cawley. >> >> (cherry picked from commit ff1e72acead01df7d8ed0fbb31efd32f57953618) >> >> The problem is GC64-specific and could be reproduced with enabled >> compiler options LUA_USE_ASSERT and LUA_USE_APICHECK. > I would prefer to see a bug description in the commit message too. >> Sergey Kaplun: >> * minimized reproducer made by fuzzing test >> >> Sergey Bronnikov: >> * added the description (see a comment in the test) >> * added tests for the problem: first one based on the original >> reproducer and second one based on a reproducer made by fuzzing test. >> >> Part of tarantool/tarantool#8825 > Should be tarantool/tarantool#9145 Fixed. >> --- >> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-611-always-snapshot-functions-for-non-base-frames >> PR: https://github.com/tarantool/tarantool/pull/9254 >> LJ issue: https://github.com/LuaJIT/LuaJIT/issues/611 >> >> src/lj_record.c | 1 + >> src/lj_snap.c | 9 +++- >> ...t-functions-for-non-base-frames-1.test.lua | 36 +++++++++++++++ >> ...hot-functions-for-non-base-frames.test.lua | 45 +++++++++++++++++++ >> 4 files changed, 89 insertions(+), 2 deletions(-) >> create mode 100644 test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua >> create mode 100644 test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua >> >> diff --git a/src/lj_record.c b/src/lj_record.c >> index 48a5481b..55785e23 100644 >> --- a/src/lj_record.c >> +++ b/src/lj_record.c >> @@ -211,6 +211,7 @@ static TRef getcurrf(jit_State *J) >> { >> if (J->base[-1-LJ_FR2]) >> return J->base[-1-LJ_FR2]; >> + /* Non-base frame functions ought to be loaded already. */ >> lj_assertJ(J->baseslot == 1+LJ_FR2, "bad baseslot"); >> return sloadt(J, -1-LJ_FR2, IRT_FUNC, IRSLOAD_READONLY); >> } >> diff --git a/src/lj_snap.c b/src/lj_snap.c >> index 6c5e5e53..06ae17eb 100644 >> --- a/src/lj_snap.c >> +++ b/src/lj_snap.c >> @@ -85,8 +85,13 @@ static MSize snapshot_slots(jit_State *J, SnapEntry *map, BCReg nslots) >> IRIns *ir = &J->cur.ir[ref]; >> if ((LJ_FR2 || !(sn & (SNAP_CONT|SNAP_FRAME))) && >> ir->o == IR_SLOAD && ir->op1 == s && ref > retf) { >> - /* No need to snapshot unmodified non-inherited slots. */ >> - if (!(ir->op2 & IRSLOAD_INHERIT)) >> + /* >> + ** No need to snapshot unmodified non-inherited slots. >> + ** But always snapshot the function below a frame in LJ_FR2 mode. >> + */ >> + if (!(ir->op2 & IRSLOAD_INHERIT) && >> + (!LJ_FR2 || s == 0 || s+1 == nslots || >> + !(J->slot[s+1] & (TREF_CONT|TREF_FRAME)))) >> continue; >> /* No need to restore readonly slots and unmodified non-parent slots. */ >> if (!(LJ_DUALNUM && (ir->op2 & IRSLOAD_CONVERT)) && >> diff --git a/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua >> new file mode 100644 >> index 00000000..759c2862 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua > I believe we should give these tests better names that -1.test.lua > and .test.lua. Maybe those names should include the original source of > the reproducer (I mean, fuzzer, and the original issue). Actually specifying source of the test in its name will say nothing to everyone who will use tests. Let's use postfix "original" for the first testcase. > >> + >> +test:plan(1) >> + >> +jit.opt.start('hotloop=1', 'hotexit=1') >> + >> +-- Test reproduces a bug "GC64: Function missing in snapshot for non-base > Typo: s/Test/The test/ > Typo: s/a bug/the bug/ Fixed. >> +-- frame" [1], and based on reproducer described in [2]. > Typo: s/based/is based/ > Typo: s/reproducer/the reproducer/ Fixed. >> +-- >> +-- [1]: https://github.com/LuaJIT/LuaJIT/issues/611 >> +-- [2]: https://github.com/LuaJIT/LuaJIT/issues/611#issuecomment-679228156 >> +-- >> +-- Function `outer` is recorded to a trace and calls a builtin function that is > Typo: s/builtin/built-in/ Fixed. >> +-- not JIT-compilable and therefore triggers exit to interpreter, and then it > Typo: s/to interpreter/to the interpreter Fixed. >> +-- resumes tracing just after the call returns - this is a trace stitching. > Typo: s/is a/is/ Fixed. >> +-- Then, within the call, we need the potential for a side trace. Finally, we need >> +-- that side exit to be taken enough for the exit to be compiled into a trace. > Typo: s/taken enough/taken enough times/ Fixed. >> +-- The loop at the bottom has enough iterations to trigger JIT compilation, and >> +-- enough more on top on trigger compilation of the not case. Compilation of > These two lines are incomprehensible. Please parahphrase them. Updated (and force-pushed):  -- --- Function `outer` is recorded to a trace and calls a builtin function that is --- not JIT-compilable and therefore triggers exit to interpreter, and then it --- resumes tracing just after the call returns - this is a trace stitching. --- Then, within the call, we need the potential for a side trace. Finally, we need --- that side exit to be taken enough for the exit to be compiled into a trace. --- The loop at the bottom has enough iterations to trigger JIT compilation, and --- enough more on top on trigger compilation of the not case. Compilation of --- this case hits the assertion failure. +-- Function `outer` is recorded to a trace and calls a built-in +-- function that is not JIT-compilable and therefore triggers +-- exit to the interpreter, and then it resumes tracing just after +-- the call returns - this is trace stitching. Then, within +-- the call, we need the potential for a side trace. Finally, +-- we need that side exit to be taken enough times for the exit +-- to be compiled into a trace. This compilation hits +-- the assertion failure.  local inner  for _ = 1, 3 do >> +-- this case hits the assertion failure. >> + >> +local inner >> +for _ = 1, 3 do >> + inner = function(_, i) >> + return i < 4 >> + end >> +end >> + >> +local function outer(i) >> + -- The function `string.gsub` is not JIT-compilable and triggers a trace >> + -- exit. For example, `string.gmatch` and `string.match` are suitable as >> + -- well. >> + -- See https://github.com/tarantool/tarantool/wiki/LuaJIT-Not-Yet-Implemented. >> + inner(string.gsub('', '', ''), i) >> +end >> + >> +for i = 1, 4 do >> + outer(i) >> +end > I like this repro much more. It is easier to understand than the first one. > What's the motivation behind having two tests for the same issue? We had a repro produced by our fuzzing test and we agreed with Sergey K. to include this repro as a test too. >> + >> +test:ok(true, 'function is present in snapshot') >> +test:done(true) >> -- >> 2.34.1 >>