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 E620A51F737; Mon, 10 Jul 2023 17:53:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E620A51F737 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1689000822; bh=t/nvcHzK2Y+I/QlOXcyKl8lAPPqKs+a61AGuj7G3nTs=; 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=T23xao3+rQlnkwZgy8m1rY08lfU3HNElg050cOWnPDk8es4EtT/wSeOCQefyqm+I9 rJqJhA9C9KMA1rpHteY62UGEKG++37CO4E0nCx6LFLdhzvtmnXkA2uLn2f8v68Doyr mPGYSxseYSTGdaj7+cWr6EVRN7GuM8HPTFzoDOfU= Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [95.163.41.81]) (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 DDB384F855C for ; Mon, 10 Jul 2023 17:53:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DDB384F855C Received: by smtp40.i.mail.ru with esmtpa (envelope-from ) id 1qIsGh-008Tp5-Sd; Mon, 10 Jul 2023 17:53:40 +0300 Message-ID: <61340557-2a53-e6e3-ac6f-280ff30f1d66@tarantool.org> Date: Mon, 10 Jul 2023 17:53:39 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 To: Sergey Kaplun References: <895991f2-93ca-0901-031b-2b39e0612a39@tarantool.org> Content-Language: en-US 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: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9F6142ABD4516DDC5081FADD37C191A8C18BC49CE86EDB7AF182A05F538085040426C0BF5CC70E65B4897348AC09A7FAA330954A0243CC3FDC361CC1D880D4B78 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE706EA9E10470DC775EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006373103A56A89D9083FEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BE5CCB53A13BC8DBAEE14A1E8435A58C66997FB68A1BD7F83CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F79006376C1CC96B391BF954389733CBF5DBD5E9B5C8C57E37DE458BD9DD9810294C998ED8FC6C240DEA76428AA50765F79006371191FFCD31C1A256D81D268191BDAD3DBD4B6F7A4D31EC0BE2F48590F00D11D6D81D268191BDAD3D78DA827A17800CE7B078B4E09E491CEEEC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3C9BE88FFEDFA497A35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A57E710B199E419D80BBC18B802CC58A755BD923491A7CF8A8F87CCE6106E1FC07E67D4AC08A07B9B0A6C7FFFE744CA7FBCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3463DBE2ADA183F62F84BEBD7B6DDC5D8A1B9A17197D0CAE67A28C6A37F87AF3ED9B321F3788F920F91D7E09C32AA3244CF88A5A51EF4DDF1CE7AEAB538C8696DBD9ADFF0C0BDB8D1FBAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj7en6wDTORtMlyGGG8+rffA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769F4ADED89808EEFD44897348AC09A7FAAC9F871340C5A6C40EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! thanks for review! On 7/9/23 16:15, Sergey Kaplun wrote: > 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]). Fixed! > >> 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". Dropped. > >> 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: // Dropped. > >> 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 :). Replaced with suggested paragraph. Thanks. > >> (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. Updated. +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua @@ -1,31 +1,33 @@  local tap = require('tap') --- Test contains a reproducer for a problem when LuaJIT generates a wrong --- bytecode with a missed BC_UCLO instruction. +-- 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(),  }) >> +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/ Fixed. > >> +-- 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. Updated. > >> +-- Both listings contains two BC_UCLO instructions: > Typo: s/contains/contain/ Updated. > >> +-- - first one with id 0004 is generated for a statement 'break' inside > Typo: s/first/the first/ Fixed. > >> +-- condition, see label BC_UCLO1; >> +-- - second one with id 0009 is generated for a statement 'return' inside > Typo: s/second/the second/ Fixed. > >> +-- 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/ Fixed. > >> +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a > Typo: s/first/the first/ Fixed. > >> +-- 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/ Fixed. > >> +-- fixup BC_UCLO and problem does not appear. > Typo: s/problem/the problem/ Fixed. > >> +-- >> +-- 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. Fixed here and below. > >> +-- 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,/ Fixed. >> +-- 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/ Fixed. > > I suggest the following rewording: > | XXX: it is not a dead code, it is a part of the reproducer, see the > | comment above. Replaced. > >> + -- 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,/ Fixed. > >> +-- as desirable. >> +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct') > Code width is more than 80 symbols. Fixed. @@ -98,11 +102,11 @@ end  local f = missing_uclo()  local res = f() --- Without a patch we don't get here a function, because upvalue isn't closed --- as desirable. -test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct') +-- Without a patch, we don't get here a function, because upvalue +-- isn't closed as desirable. +test:ok(type(res) == 'function', +        'VM consistency: type of returned value is correct') > Minor: s/virtual machine/VM/. Feel free to ignore. Fixed. > >> + >> +-- 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)). Dropped. >> +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. Fixed. @@ -110,6 +114,7 @@ f()  f = missing_uclo()  local _  _, res = pcall(f) -test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct') +test:ok(type(res) == 'function', +        'consistency on compilation: type of returned value is correct')  os.exit(test:check() and 0 or 1) > Do we need pcall here? I would use it to avoid breaking test due to assert. Without a pcall: TAP version 13 1..2 not ok - VM consistency: type of returned value is correct     filename:   eval     line:       -1     frame #1       line:     0       source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua       filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua       what:     main       namewhat:       src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua     frame #2       line:     -1       source:   =[C]       filename: eval       what:     C       namewhat:       src:      [C] luajit: /home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed. Aborted (core dumped) With pcall: TAP version 13 1..2 not ok - VM consistency: type of returned value is correct    filename:   eval     line:       -1     frame #1       line: 0       source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua       filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua       what:     main       namewhat:       src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua     frame #2       line:     -1       source:   =[C]       filename: eval       what:     C       namewhat:       src:      [C] not ok - consistency on compilation: type of returned value is correct     filename:   eval     line:       -1     frame #1       line:     0 I like second output more. > > 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 >