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 3D7BD68CACB; Tue, 3 Oct 2023 21:31:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3D7BD68CACB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1696357903; bh=XSai6N72wsiLh56aLoFbZEx+ohQO90GdBpH2r5cir4A=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=mTXIsAxD5CY1X8i/OH5zZgIOIf3xQJRXjt/+q34M8WWvxNbwEeVvMCjFX1jjBn+iW QMw4BiOimJPg53NRlVe5kA1c3k5CpQdWpyCqzp4S6i8yZdI6w41+qWZwJu/bDUky9Y n2VLCDNKfoTNm1js1+klkTyY1ntnMkcK5nPJHqQw= Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [95.163.41.65]) (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 099DE68A492 for ; Tue, 3 Oct 2023 21:31:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 099DE68A492 Received: by smtp42.i.mail.ru with esmtpa (envelope-from ) id 1qnkBJ-00DBpt-0t; Tue, 03 Oct 2023 21:31:41 +0300 Message-ID: <3f7d2372-b13c-16e9-9c7a-0de52be8f68a@tarantool.org> Date: Tue, 3 Oct 2023 21:31:40 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Content-Language: en-US To: Maxim Kokryashkin , tarantool-patches@dev.tarantool.org, skaplun@tarantool.org References: <20230929133843.535217-1-m.kokryashkin@tarantool.org> In-Reply-To: <20230929133843.535217-1-m.kokryashkin@tarantool.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD94A796AF4779ECFCEBD5747D70AF4B4F4CFCCBA563BF9AD4D182A05F53808504004DCC0FC2DC3D61C874984AC32569898BFE35E9325A8F293F3415A8BFD38A296 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FCFCB92DA8654BB0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371777B963B59186248638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B18D8F4F8EF768C04176EB39624AD67E117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3A703B70628EAD7BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE1E561CDFBCA1751F1B780A39BCC1DD35D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE386A7C529F68B8E5C302FCEF25BFAB345C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063752464DD08821749FEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: 4965CFDFE0519134C1FE400A9E48C5401DD40DE57556AFB266D16FC5F53507A1816E0A2A8F779BBED8D40077074E805C66D16FC5F53507A117535B0CF9F6D0C3EE9D5CB6078CC77C16263ECDFA3561850D10BAB907AB3466 X-C1DE0DAB: 0D63561A33F958A50E3D4574455C9383844E4631601BA0955CBE9943DD7EDCD6F87CCE6106E1FC07E67D4AC08A07B9B065B78C30F681404DCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD75DCE07D45A749953FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFA6E3302C478DE4C97D28662EE3C7516C329087E7B6C9F82F1F438DF8C946A5F624DE828755D94EE9CC231B5D63B17C6BA35380CBD72274B639A02C1206CC3451E48CAC7CA610320002C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojH0UbTrDsBkCcdojZWhwf/Q== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F258845855BE319DC416034A32257E3C634EB672C4647202ACCE2865282EC151BADDC1D3523A6D01B4765B2DFB59E2DDD9FE06B14FA522850F29BC30B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*. 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Max thanks for the patch. See my comments. Sergey On 9/29/23 16:38, Maxim Kokryashkin wrote: > From: Mike Pall > > Reported by Arseny Vakhrushev. > Fix contributed by Peter Cawley. Missed: "(cherry picked from commit ...)" > > As specified in the comment in `lj_record_stop`, all loops must > set `J->pc` to the next instruction. However, the chunk of logic > in `lj_trace_exit` expects it to be set to `BC_JLOOP` itself if > it used to be a `BC_RET`. This wrong pc results in the execution > of random data that goes after `BC_JLOOP` in the case of > restoration from the snapshot. > > This patch fixes that behavior by adapting the loop recording > logic to this specific case. > > Maxim Kokryashkin: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > Changes in v2: > - Fixed comments as per review by Sergey Kaplun > > Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc > PR: https://github.com/tarantool/tarantool/pull/9166 Missed a link to original issue - https://github.com/luajiT/LuaJIT/issues/624 > > src/lj_record.c | 9 ++- > src/lj_snap.c | 3 + > .../lj-624-jloop-snapshot-pc.test.lua | 81 +++++++++++++++++++ > 3 files changed, 89 insertions(+), 4 deletions(-) > create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua > > diff --git a/src/lj_record.c b/src/lj_record.c > index 48a5481b..3bdc6134 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -570,10 +570,10 @@ static LoopEvent rec_iterl(jit_State *J, const BCIns iterins) > } > > /* Record LOOP/JLOOP. Now, that was easy. */ > -static LoopEvent rec_loop(jit_State *J, BCReg ra) > +static LoopEvent rec_loop(jit_State *J, BCReg ra, int skip) > { > if (ra < J->maxslot) J->maxslot = ra; > - J->pc++; > + J->pc += skip; > return LOOPEV_ENTER; > } > > @@ -2433,7 +2433,7 @@ void lj_record_ins(jit_State *J) > rec_loop_interp(J, pc, rec_iterl(J, *pc)); > break; > case BC_LOOP: > - rec_loop_interp(J, pc, rec_loop(J, ra)); > + rec_loop_interp(J, pc, rec_loop(J, ra, 1)); > break; > > case BC_JFORL: > @@ -2443,7 +2443,8 @@ void lj_record_ins(jit_State *J) > rec_loop_jit(J, rc, rec_iterl(J, traceref(J, rc)->startins)); > break; > case BC_JLOOP: > - rec_loop_jit(J, rc, rec_loop(J, ra)); > + rec_loop_jit(J, rc, rec_loop(J, ra, > + !bc_isret(bc_op(traceref(J, rc)->startins)))); > break; > > case BC_IFORL: > diff --git a/src/lj_snap.c b/src/lj_snap.c > index 2dc281cb..b50ecfb2 100644 > --- a/src/lj_snap.c > +++ b/src/lj_snap.c > @@ -115,6 +115,9 @@ static MSize snapshot_framelinks(jit_State *J, SnapEntry *map, uint8_t *topslot) > #else > MSize f = 0; > map[f++] = SNAP_MKPC(J->pc); /* The current PC is always the first entry. */ > + lj_assertJ(!J->pt || > + (J->pc >= proto_bc(J->pt) && > + J->pc < proto_bc(J->pt) + J->pt->sizebc), "bad snapshot PC"); > #endif > while (frame > lim) { /* Backwards traversal of all frames above base. */ > if (frame_islua(frame)) { > diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua > new file mode 100644 > index 00000000..e0c1fa81 > --- /dev/null > +++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua > @@ -0,0 +1,81 @@ > +local tap = require('tap') > +local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > +-- XXX: The test case below triggers the assertion that was > +-- added in the patch if tested without the fix itself. It > +-- is hard to create a stable reproducer without turning off > +-- ASLR and VM randomizations, which is not suitable for testing. Proposed tests cannot reproduce an original problem. What if we add a test in a separate patch as a follow up? What do you think? Please add a link to the original issue - https://github.com/luaJIT/LuaJIT/issues/624 > + > +-- Reproducer below produces the following traces: > +-- ---- TRACE 1 start test.lua:2 > +-- 0001 KSHORT 1 2 > +-- 0002 ISGE 0 1 > +-- 0003 JMP 1 => 0006 > +-- 0006 UGET 1 0 ; fib > +-- 0007 SUBVN 2 0 0 ; 1 > +-- 0008 CALL 1 2 2 > +-- 0000 . FUNCF 4 ; test.lua:2 > +-- 0001 . KSHORT 1 2 > +-- 0002 . ISGE 0 1 > +-- 0003 . JMP 1 => 0006 > +-- 0006 . UGET 1 0 ; fib > +-- 0007 . SUBVN 2 0 0 ; 1 > +-- 0008 . CALL 1 2 2 > +-- 0000 . . FUNCF 4 ; test.lua:2 > +-- 0001 . . KSHORT 1 2 > +-- 0002 . . ISGE 0 1 > +-- 0003 . . JMP 1 => 0006 > +-- 0006 . . UGET 1 0 ; fib > +-- 0007 . . SUBVN 2 0 0 ; 1 > +-- 0008 . . CALL 1 2 2 > +-- 0000 . . . FUNCF 4 ; test.lua:2 > +-- ---- TRACE 1 stop -> up-recursion > +-- > +-- ---- TRACE 1 exit 1 > +-- ---- TRACE 2 start 1/1 test.lua:3 > +-- 0004 ISTC 1 0 > +-- 0005 JMP 1 => 0013 > +-- 0013 RET1 1 2 > +-- 0009 UGET 2 0 ; fib > +-- 0010 SUBVN 3 0 1 ; 2 > +-- 0011 CALL 2 2 2 > +-- 0000 . JFUNCF 4 1 ; test.lua:2 > +-- ---- TRACE 2 stop -> 1 > +-- > +-- ---- TRACE 2 exit 1 > +-- ---- TRACE 3 start 2/1 test.lua:3 > +-- 0013 RET1 1 2 > +-- 0012 ADDVV 1 1 2 > +-- 0013 RET1 1 2 > +-- ---- TRACE 3 abort test.lua:3 -- down-recursion, restarting > +-- > +-- ---- TRACE 3 start test.lua:3 > +-- 0013 RET1 1 2 > +-- 0009 UGET 2 0 ; fib > +-- 0010 SUBVN 3 0 1 ; 2 > +-- 0011 CALL 2 2 2 > +-- 0000 . JFUNCF 4 1 ; test.lua:2 > +-- ---- TRACE 3 stop -> 1 > +-- > +-- ---- TRACE 2 exit 1 > +-- ---- TRACE 4 start 2/1 test.lua:3 > +-- 0013 RET1 1 2 > +-- 0012 ADDVV 1 1 2 > +-- 0013 JLOOP 3 3 > +-- > +-- During the recording of the latter JLOOP the assertion added > +-- in the patch is triggered. > + > + > +jit.opt.start('hotloop=1', 'hotexit=1') > +local function fib(n) > + return n < 2 and n or fib(n - 1) + fib(n - 2) > +end > + > +fib(5) > + > +test:ok(true, 'snapshot pc is correct') > +test:done(true)