From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Maxim Kokryashkin <max.kokryashkin@gmail.com>, tarantool-patches@dev.tarantool.org, skaplun@tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*. Date: Tue, 3 Oct 2023 21:31:40 +0300 [thread overview] Message-ID: <3f7d2372-b13c-16e9-9c7a-0de52be8f68a@tarantool.org> (raw) In-Reply-To: <20230929133843.535217-1-m.kokryashkin@tarantool.org> Hi, Max thanks for the patch. See my comments. Sergey On 9/29/23 16:38, Maxim Kokryashkin wrote: > From: Mike Pall <mike> > > 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)
prev parent reply other threads:[~2023-10-03 18:31 UTC|newest] Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-09-29 13:38 Maxim Kokryashkin via Tarantool-patches 2023-10-03 18:31 ` Sergey Bronnikov via Tarantool-patches [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3f7d2372-b13c-16e9-9c7a-0de52be8f68a@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox