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 7613D4F8564; Sun, 9 Jul 2023 16:19:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7613D4F8564 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1688908796; bh=mX0Uu1EcSRyViN/KTK0tHGRhqKwJjVHlPScg6XuwHqk=; 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=dKHw0AwnXQ1IUh3sZg/7k+08Okk9WSswQRWsdsd1cUueswRds+epZzrR1w1luwkeL rf2pVds7V6WfGojHF1t2cPTRo3FWgi1OyTP6IjVNs04XxNR7SEi9+vytq4M28eB+qQ BvuHSexzIWwCqJAq3ggHF1xCGEnKUod/mHudLwsk= 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 4E2C9402818 for ; Sun, 9 Jul 2023 16:19:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4E2C9402818 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1qIUKP-0001p5-IF; Sun, 09 Jul 2023 16:19:54 +0300 Date: Sun, 9 Jul 2023 16:15:32 +0300 To: Sergey Bronnikov Message-ID: References: <895991f2-93ca-0901-031b-2b39e0612a39@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <895991f2-93ca-0901-031b-2b39e0612a39@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F6142ABD4516DDC5BED4C5E4E081E9ED2DA1A5910F2B609A182A05F5380850404E909730E7E05C64047D2468EEFF1844C95748ECB579EE2B3C30AE8338D82582 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73C696014E2DCCA1EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F6EDACB2F7CB3C698638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80B8A6C3EEB5B1D7739F5D81CACFF5A77117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520599709FD55CB46A62CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269176DF2183F8FC7C088D2E8BEBF93D4B068655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CFA80D66F452D417A43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5E64667F2534DD967BD9889EB6CF9FC0DB722FB09592DFDECF87CCE6106E1FC07E67D4AC08A07B9B06A1CB4668A9CA5FACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D343DCFC3BEDFB0242D9F11E53033894827FAFFC25A9E4D77877C3D993444A2A5B0057065FFE29651E41D7E09C32AA3244CAE3639AC98BB2FCA38DDF9AD3049316B35DA7DC5AF9B58C0BAD658CF5C8AB4025DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojREMDzwhiTMaSdVXpDe5baw== X-DA7885C5: 7DCE22B6D9F37CB5F8FC64980D5AF74172EFE76D71817A8EB4EB99F6A97E350A262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986B9EC559411D0CA6EA3C70F4A746529AF90FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns. 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: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the fixes! LGTM, except a few nits and rewordings below. > Fix BC_UCLO insertion for returns. > > Contributed by XmiliaH. > > (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e) > > Patch fixes a problem when LuaJIT generates a wrong bytecode with a > missed BC_UCLO instruction. When some of BC_RET bytecode instructions are Nit: commit line length is more than 72 symbols (see rationale here [1]). > not fixup-ed, due to an early return, if UCLO is obtained before, those > leads to VM inconsistency after return from the function. > > Patch makes the following changes in bytecode (thats it, emits extra Typo: s/thats/that's/ Nit: I suggest to drop introductory phrase "thats it". > BC_UCLO instruction that closes upvalues): > > @@ -11,11 +11,12 @@ > 0006 => LOOP 1 => 0012 > 0007 ISF 0 > 0008 JMP 1 => 0010 > -0009 RET1 0 2 > +0009 UCLO 0 => 0014 > 0010 => FNEW 0 0 ; uclo.lua:56 > 0011 JMP 1 => 0006 > 0012 => UCLO 0 => 0001 > 0013 => RET0 0 1 > +0014 => RET1 0 2 Side note: like this diff very much, good idea! > > NOTE: After emitting the bytecode instruction BC_FNEW fixup is not Typo: s/NOTE: // > required, because FuncState will set a flag PROTO_CHILD that will > trigger emitting a pair of instructions BC_UCLO and BC_RET (see > ) and BC_RET will close all upvalues from a base > equal to 0. > > JIT compilation of missing_uclo() function without a patch with fix is failed: > src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed. I suppose there is no need to cite this assertion failure. So, I suggest to reword this paragraph in the following way: | JIT compilation of `missing_uclo()` function without a patch leads to | assertion failure in `rec_check_slots()` due to Lua stack inconsistency. | The root cause is still parsing misbehaviour. Nevertheless, the test | case is added to be sure that the JIT compilation isn't affected. Also, there is no need to mention me in the commit message :). > (Thanks to Sergey Kaplun for discovering this!) > Thus second testcase in a test covers a case with compilation as well. > > Sergey Bronnikov: > * added the description and the test for the problem > > Signed-off-by: Sergey Bronnikov > Co-authored-by: Sergey Kaplun > > diff --git a/src/lj_parse.c b/src/lj_parse.c > index af0dc53f..343fa797 100644 > --- a/src/lj_parse.c > +++ b/src/lj_parse.c > @@ -1546,7 +1546,7 @@ static void fs_fixup_ret(FuncState *fs) > /* Replace with UCLO plus branch. */ > fs->bcbase[pc].ins = BCINS_AD(BC_UCLO, 0, offset); > break; > - case BC_UCLO: > + case BC_FNEW: > return; /* We're done. */ > default: > break; > diff --git a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua > new file mode 100644 > index 00000000..942c22b2 > --- /dev/null > +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua > @@ -0,0 +1,115 @@ Please, restrict comment length to the 66 symbols in the test. > +local tap = require('tap') > +-- Test contains a reproducer for a problem when LuaJIT generates a wrong > +-- bytecode with a missed BC_UCLO instruction. > +local test = tap.test('lj-819-fix-missing-uclo'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(2) > + > +-- Let's take a look at listings Listing 1 and Listing 2 below with bytecode Typo: s/bytecode/the bytecode/ > +-- generated for a function missing_uclo() with and without a patch. Minor: I suggest to use `` to line-up that this is functions names in the code. Feel free to ignore. > +-- Both listings contains two BC_UCLO instructions: Typo: s/contains/contain/ > +-- - first one with id 0004 is generated for a statement 'break' inside Typo: s/first/the first/ > +-- condition, see label BC_UCLO1; > +-- - second one with id 0009 is generated for a statement 'return' inside Typo: s/second/the second/ > +-- a nested loop, see label BC_UCLO2; > +-- Both BC_UCLO's closes upvalues after leaving a function's scope. > +-- > +-- The problem is happen when fs_fixup_ret() traverses bytecode instructions in Typo: s/happen/happened/ > +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a Typo: s/first/the first/ > +-- second one (return). This leads to a wrong result produced by a function Nit: I suggest the reword this in the following way: | and misses the second one return to fixup > +-- returned by missing_uclo() function. This also explains why do we need a > +-- dead code in reproducer - without first BC_UCLO fs_fixup_ret() successfully Typo: s/first/the first/ > +-- fixup BC_UCLO and problem does not appear. Typo: s/problem/the problem/ > +-- > +-- Listing 1. Bytecode with a fix. > +-- > +-- -- BYTECODE -- uclo.lua:1-59 I suggest to use listing from the test itself, i.e. use lj-819-fix-missing-uclo.test.lua:79-97 (line numbers are used before comment rewording and reallignment) here and below in headers. > +-- 0001 => LOOP 0 => 0013 > +-- 0002 JMP 0 => 0003 > +-- 0003 => JMP 0 => 0005 > +-- 0004 UCLO 0 => 0013 > +-- 0005 => KPRI 0 0 > +-- 0006 => LOOP 1 => 0012 > +-- 0007 ISF 0 > +-- 0008 JMP 1 => 0010 > +-- 0009 UCLO 0 => 0014 > +-- 0010 => FNEW 0 0 ; uclo.lua:54 And lj-819-fix-missing-uclo.test.lua:92 here and below in listings. > +-- 0011 JMP 1 => 0006 > +-- 0012 => UCLO 0 => 0001 > +-- 0013 => RET0 0 1 > +-- 0014 => RET1 0 2 > +-- > +-- Listing 2. Bytecode without a fix. > +-- > +-- BYTECODE -- uclo.lua:1-59 > +-- 0001 => LOOP 0 => 0013 > +-- 0002 JMP 0 => 0003 > +-- 0003 => JMP 0 => 0005 > +-- 0004 UCLO 0 => 0013 > +-- 0005 => KPRI 0 0 > +-- 0006 => LOOP 1 => 0012 > +-- 0007 ISF 0 > +-- 0008 JMP 1 => 0010 > +-- 0009 UCLO 0 => 0014 > +-- 0010 => FNEW 0 0 ; uclo.lua:54 > +-- 0011 JMP 1 => 0006 > +-- 0012 => UCLO 0 => 0001 > +-- 0013 => RET0 0 1 > +-- 0014 => RET1 0 2 > +-- > +-- Listing 3. Changes in bytecode before and after a fix. > +-- > +-- @@ -11,11 +11,12 @@ > +-- 0006 => LOOP 1 => 0012 > +-- 0007 ISF 0 > +-- 0008 JMP 1 => 0010 > +-- -0009 RET1 0 2 > +-- +0009 UCLO 0 => 0014 > +-- 0010 => FNEW 0 0 ; uclo.lua:56 > +-- 0011 JMP 1 => 0006 > +-- 0012 => UCLO 0 => 0001 > +-- 0013 => RET0 0 1 > +-- +0014 => RET1 0 2 > +-- > +-- First testcase checks a correct bytecode generation by frontend Typo: s/First/The first/ Typo: s/frontend/frontend,/ > +-- and the second testcase checks consistency on a JIT compilation. > + > +local function missing_uclo() > + while true do -- luacheck: ignore > + -- Attention: it is not a dead code, it is a part of reproducer. Typo: s/reproducer/the reproducer/ I suggest the following rewording: | XXX: it is not a dead code, it is a part of the reproducer, see the | comment above. > + -- label: BC_UCLO1 > + if false then > + break > + end > + local f > + while true do > + if f then > + -- label: BC_UCLO2 > + return f > + end > + f = function() > + return f > + end > + end > + end > +end > + > +local f = missing_uclo() > +local res = f() > +-- Without a patch we don't get here a function, because upvalue isn't closed Typo: s/Without a patch/Without a patch,/ > +-- as desirable. > +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct') Code width is more than 80 symbols. Minor: s/virtual machine/VM/. Feel free to ignore. > + > +-- Make JIT compiler aggressive. I suggest to drop this comment (there is no need to comment what `hotloop=1` do, but if you want you may explain why we use 1 here (this is still common practice in our tests, so I suggest just drop the comment)). > +jit.opt.start('hotloop=1') > + > +f = missing_uclo() > +f() > +f = missing_uclo() > +local _ > +_, res = pcall(f) > +test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct') Code width is more than 80 symbols. Do we need pcall here? Also, the test isn't failed with assertion failure as declared. But the following one is: | LUA_PATH="src/?.lua;;" src/luajit -Ohotloop=1 -e ' | | local function missing_uclo() | while true do -- luacheck: ignore | local f | if false then break end | while true do | if f then | return f | end | f = function() | return f | end | end | end | end | | -- Function to pollute Lua stack. | local function ret_arg(f) return f end | | f = missing_uclo() | ret_arg(f()) | ret_arg(f()) | ' > + > +os.exit(test:check() and 0 or 1) [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message -- Best regards, Sergey Kaplun