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 A956969F6ED; Tue, 17 Oct 2023 17:56:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A956969F6ED DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1697554608; bh=0ydCUz0K3lFjYdFFAQT3F/pGYSSfl+mTXE/VN6iPbtY=; 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=rDrox51+YRKwpnoOt0h/nbsdiF6shxY/tGxrYvEe6FPDVLrt79kw8cIpLDa5f62rO rYA1KSbXC+pk4X5ZdPCRJCd5yTz14NabdKI2kIs9Omranq01D/2FUWlyrBxU7Cpc0j ELv57Wy/6hn1BMC3Jdlwxd/tmyzoEpoqAwiOLUmE= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [95.163.41.82]) (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 4FEBB69F6ED for ; Tue, 17 Oct 2023 17:56:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4FEBB69F6ED Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1qslV0-004rhG-0X; Tue, 17 Oct 2023 17:56:46 +0300 Date: Tue, 17 Oct 2023 17:56:45 +0300 To: Sergey Bronnikov Message-ID: References: <8d9d59bf865fe5764a3c91d5a363f7e2bc78a348.1697115768.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8d9d59bf865fe5764a3c91d5a363f7e2bc78a348.1697115768.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9A0C7FD9243F13F0A0FCA17AF7E0BADE3B13B26AE5DD983FA182A05F5380850404C228DA9ACA6FE27E2F5575241BFA27D33B7F28A456BB0A34F781A8F4C905DE89A905218BFDD3845 X-C1DE0DAB: 0D63561A33F958A518BC677442E7DAF91F763346D915C0F1C82122E601244C1BF87CCE6106E1FC07E67D4AC08A07B9B01DAA61796BF5227BCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FD73A7961AD43DBF40F0229E4F13D42FEE63576ACB78C2A406E7630EB792D79CCEF8AEA28A335898A37FD76D11AF80A0D3A324AD400823D7536A335ADA9BF6695EA455F16B58544A21C197AAF4D2E4732965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojfbpVM+RcTJDqD9VfY+JcoA== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407DAFF54B379FDBDA9FBF09A032843ADE25FC9CED9F47C053AD51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: max.kokryashkin@gmail.com, 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 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 > --- > 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). > @@ -0,0 +1,36 @@ > +local tap = require('tap') > +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames-1'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +-- GC64: Function missing in snapshot for non-base frame > +-- https://github.com/LuaJIT/LuaJIT/issues/611 > + > +test:plan(1) > + > +jit.opt.start('hotloop=1', 'hotexit=1') > + > +local inner_counter = 0 > +local SIDE_START = 1 > +-- Lower frame to return from `inner()` function side trace. > +-- TODO: Give a reason for vararg func. > +local function lower_frame(...) > + local inner = function() > + if inner_counter > SIDE_START then > + return > + end > + inner_counter = inner_counter + 1 > + end > + inner(..., inner(inner())) > +end > + > +-- Compile `inner()` function. > +lower_frame() > +lower_frame() > +-- Compile hotexit > +lower_frame() > +-- Take side exit from side trace. > +lower_frame(1) > + > +test:ok(true, 'function is present in snapshot') > +test:done(true) > diff --git a/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua > new file mode 100644 > index 00000000..7305c185 > --- /dev/null > +++ b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua > @@ -0,0 +1,45 @@ > +local tap = require('tap') > +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +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/ > +-- frame" [1], and based on reproducer described in [2]. Typo: s/based/is based/ Typo: s/reproducer/the reproducer/ > +-- > +-- [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/ > +-- not JIT-compilable and therefore triggers exit to interpreter, and then it Typo: s/to interpreter/to the interpreter > +-- resumes tracing just after the call returns - this is a trace stitching. Typo: s/is a/is/ > +-- 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/ > +-- 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. > +-- 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? > + > +test:ok(true, 'function is present in snapshot') > +test:done(true) > -- > 2.34.1 >