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 A6E3E6755B6; Mon, 23 Oct 2023 18:01:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A6E3E6755B6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1698073292; bh=3EkfFjj/dcBBPtpYZOwWfvMCEkNlEQ7Qo1fUe+MOZRM=; 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=RlyaRPq7U9L9/KTRlV/mAJKFI2ZXujVc+eaV9gIh54/ajzhafeKc1clC6+11TBaYG uuxStJ6pVYgivAbQB5y/NXW4eM4xxuj7N+BwyyF0tuhoRPHLOiKe7nvWBGMgrcHOgq xw1hH3GpLuvlmXHxhkzRgSKN/5yKjm1c5bV6hgiw= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 746726755B6 for ; Mon, 23 Oct 2023 18:01:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 746726755B6 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1quwQs-0007j9-Pm; Mon, 23 Oct 2023 18:01:31 +0300 Date: Mon, 23 Oct 2023 17:57:01 +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-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D4445358DD2C13E2729A38C26529782BDA0C7D983A9B0819182A05F538085040DDD28ED4139A3F2CB0969EBC30F08946D1862F59CC5BEE5D35D79EED2D08C2FC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BF6702EC5472AA0FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371D26D2A8652661258638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89F67D8C67E064EE2139C84D72F661831117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C0406C186E56A1B26068655334FD4449CB33AC447995A7AD186D8C47C27EEC5E9FD5E8D9A59859A8B699FC562817B5DAD0089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A58003E87DF8D3B11B8F123EA4B452E39FF40911B9918A5D80F87CCE6106E1FC07E67D4AC08A07B9B0E753FA5741D1AD02CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF4CDF59C900D8634497E2E96F6912E209851F8B005662011D662DE911A12F6A73182FB9261F6240E8A8B50B9382ADB12A6F69771324DE8B755177FE65F32FAFECA74DFFEFA5DC0E7F02C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojqlVu258LHAHey8CXILAlYg== X-DA7885C5: 7481110147A4E15ABBDF5A895D568FB2B0D6029718CC2444FD8A34745CA3EF93262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B5CB08EBFA48537640F0885F6CF6D3F3E0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun 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 12.10.23, Sergey Bronnikov 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. > > 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 > --- > 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 > 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 > @@ -0,0 +1,36 @@ > +local tap = require('tap') > +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames-1'):skipcond({ I suppose, that we can choose smaller name, because the code width is more than 80 symbols. :) I suggest 'lj-611-gc64-inherit-frame-slot' for both tests and filenames. Feel free to change it as you wish:). > + ['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. I see this TODO, but don't see an explanation. If you want to observe the behaviour compare the code flow (with the fixed-arg function) in the gdb. > +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 Missed dot at the end of the sentence. > +lower_frame() > +-- Take side exit from side trace. > +lower_frame(1) > + > +test:ok(true, 'function is present in snapshot') Typo: s/snapshot/the 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({ I suppose, that we can choose smaller name, because the code width is more than 80 symbols. :) I suggest 'lj-611-gc64-inherit-frame-slot-orig' for both tests and filenames. Feel free to change it as you wish:). > + ['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 > +-- frame" [1], and based on reproducer described in [2]. > +-- > +-- [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 > +-- 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. > + > +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. Comment width is more than 66 symbols. > + inner(string.gsub('', '', ''), i) > +end > + > +for i = 1, 4 do > + outer(i) > +end > + > +test:ok(true, 'function is present in snapshot') Typo: s/snapshot/the snapshot/ > +test:done(true) > -- > 2.34.1 > -- Best regards, Sergey Kaplun