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 4AFF86ECE3; Tue, 28 Jun 2022 09:59:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4AFF86ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1656399566; bh=ArjNsFRzFScVL8dEzRQAzUd1Jkh6IppoMQ1/GoyBIks=; 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=wbRJjQUltIADXcy8FcVy6sM+gxCm6AQt0gscdwiMobB6MyXTm9B3ejxSEBu5OyVMy 71oVx11+EYz0TnbhklB891hbUWIQki+fHCWeSvFGg1XJz1sf7bDHtfS6Lf1otAU5ZF yR4K0/sXtq+Z+eMkOFPEIH4EUK1Ib2yB3vTr2Muc= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 BAED16ECE3 for ; Tue, 28 Jun 2022 09:59:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BAED16ECE3 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1o65Bz-0001u4-Sd; Tue, 28 Jun 2022 09:59:24 +0300 Date: Tue, 28 Jun 2022 09:57:07 +0300 To: sergos Message-ID: References: <20220127115346.22800-1-skaplun@tarantool.org> <60369E65-251D-4C39-B8BB-0EE3E291DC16@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <60369E65-251D-4C39-B8BB-0EE3E291DC16@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD921FF253C2DCA6432F205BFE23685B2C747BEBA2DF010ABD500894C459B0CD1B9ACE1406DF9E87F33042170ACCBCC289573C4A3A29EFBC24F0B100F9AFB84C8EA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75DF2B1F23425CAE5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378F586D843116CFB2EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B8859CA687ABA27BA7AF8DB8BD4D5E5AA2C544F51ABDFF86ACC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8744B801E316CB65FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B9F5955FECEF5819E75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 0D63561A33F958A5A1514197C48E4380CDD15F1FEDD2D4863721A9BE965652A0D59269BC5F550898D99A6476B3ADF6B4886A5961035A09600383DAD389E261318FB05168BE4CE3AF X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3467D08F30473A5842A4D32481539A94361A20F13390E71878B069D45B9BBA93B6162B6E906E3856D01D7E09C32AA3244C93153C708B8C7666BF47EB61FCB3E294D9ADFF0C0BDB8D1FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj8L65zF8kk4dlI6f9rvc3vw== X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284EFAADE2C532B3970053BFF445B8CF58350FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix bytecode dump unpatching. 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: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergos! Thanks for the review! On 27.06.22, sergos wrote: > Hi! > > Thanks for the patch! > Just some nits in changelog, LGTM I've updated commit message to the following: =================================================================== Fix bytecode dump unpatching. Reported by Christopher Oliver. (cherry picked from commit 20ac817a747cf8cab044ae81b09c08d23e34342b) RET bytecodes are patched to JLOOP bytecode in a function with up-recursion. During dump those bytecodes they should be unpatched to the original one. It is done by restoring the opcode by subtraction the diff between JLOOP and ILOOP bytecodes. The restore was done by the erroneous opcode subtraction, that led to a LOOP bytecode in place of the RET one. This patch fixes the bytecode unpatching via copy of the original instruction, that was patched. Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#6548 =================================================================== > > Sergos > > > On 27 Jan 2022, at 14:53, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > Reported by Christopher Oliver. > > > > (cherry picked from commit 20ac817a747cf8cab044ae81b09c08d23e34342b) > > > > When a compiled function with up-recursion RET bytecodes are patched to > > JLOOP bytecode. > > If I got it right? > “RET bytecodes are patched to JLOOP bytecode in a function with up-recursion." > > > During dump of those bytecodes they should be unpatched > ^^^ ^^^^^ remove 2 words > > > to the original one. > > It is done by restoring the opcode by subtraction > > > the diff between JLOOP and ILOOP bytecodes. That gives the LOOP > > bytecodes instead RET as expected. > > The restore was done by the erroneous opcode subtraction, that led to a LOOP > bytecode in place of the RET one. > > > This patch fixes the bytecode unpatching via copy the original start > of ???? > > instruction, that was patched. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#6548 > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-wrong-bc-ret > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-noticket-wrong-bc-ret-full-ci > > Related issue: https://github.com/tarantool/tarantool/issues/6548 > > > > src/lj_bcwrite.c | 5 +---- > > .../bc-jit-unpatching.test.lua | 22 +++++++++++++++++++ > > 2 files changed, 23 insertions(+), 4 deletions(-) > > create mode 100644 test/tarantool-tests/bc-jit-unpatching.test.lua > > > > diff --git a/src/lj_bcwrite.c b/src/lj_bcwrite.c > > index 5e05caea..a86d6d00 100644 > > --- a/src/lj_bcwrite.c > > +++ b/src/lj_bcwrite.c > > @@ -219,10 +219,7 @@ static char *bcwrite_bytecode(BCWriteCtx *ctx, char *p, GCproto *pt) > > q[LJ_ENDIAN_SELECT(0, 3)] = (uint8_t)(op-BC_IFORL+BC_FORL); > > } else if (op == BC_JFORL || op == BC_JITERL || op == BC_JLOOP) { > > BCReg rd = q[LJ_ENDIAN_SELECT(2, 1)] + (q[LJ_ENDIAN_SELECT(3, 0)] << 8); > > - BCIns ins = traceref(J, rd)->startins; > > - q[LJ_ENDIAN_SELECT(0, 3)] = (uint8_t)(op-BC_JFORL+BC_FORL); > > - q[LJ_ENDIAN_SELECT(2, 1)] = bc_c(ins); > > - q[LJ_ENDIAN_SELECT(3, 0)] = bc_b(ins); > > + memcpy(q, &traceref(J, rd)->startins, 4); > > } > > } > > } > > diff --git a/test/tarantool-tests/bc-jit-unpatching.test.lua b/test/tarantool-tests/bc-jit-unpatching.test.lua > > new file mode 100644 > > index 00000000..9f9cb390 > > --- /dev/null > > +++ b/test/tarantool-tests/bc-jit-unpatching.test.lua > > @@ -0,0 +1,22 @@ > > +local tap = require('tap') > > +local utils = require('utils') > > + > > +local test = tap.test('bc-jit-unpatching') > > +test:plan(1) > > + > > +-- Function with up-recursion. > > +local function f(n) > > + return n < 2 and n or f(n - 1) + f(n - 2) > > +end > > + > > +local ret1bc = 'RET1%s*1%s*2' > > +-- Check that this bytecode still persists. > > +assert(utils.hasbc(loadstring(string.dump(f)), ret1bc)) > > + > > +-- Compile function to get JLOOP bytecode in recursion. > > Do you need any jit.opt.start(‘hotloop=1’) here? Yes, we can reduce amount of recursion calls with it: =================================================================== diff --git a/test/tarantool-tests/bc-jit-unpatching.test.lua b/test/tarantool-tests/bc-jit-unpatching.test.lua index 9f9cb390..6a5bed52 100644 --- a/test/tarantool-tests/bc-jit-unpatching.test.lua +++ b/test/tarantool-tests/bc-jit-unpatching.test.lua @@ -13,8 +13,9 @@ local ret1bc = 'RET1%s*1%s*2' -- Check that this bytecode still persists. assert(utils.hasbc(loadstring(string.dump(f)), ret1bc)) +jit.opt.start('hotloop=1', 'hotexit=1') -- Compile function to get JLOOP bytecode in recursion. -f(10) +f(5) test:ok(utils.hasbc(loadstring(string.dump(f)), ret1bc), 'bytecode unpached correctly') =================================================================== > > > +f(10) > > + > > +test:ok(utils.hasbc(loadstring(string.dump(f)), ret1bc), > > + 'bytecode unpached correctly') > > + > > +os.exit(test:check() and 0 or 1) > > -- > > 2.34.1 > > > -- Best regards, Sergey Kaplun