* [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.
@ 2023-09-29 13:38 Maxim Kokryashkin via Tarantool-patches
2023-10-03 18:31 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 2+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-09-29 13:38 UTC (permalink / raw)
To: tarantool-patches, skaplun, sergeyb
From: Mike Pall <mike>
Reported by Arseny Vakhrushev.
Fix contributed by Peter Cawley.
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
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.
+
+-- 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)
--
2.42.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.
2023-09-29 13:38 [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET* Maxim Kokryashkin via Tarantool-patches
@ 2023-10-03 18:31 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 2+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-10-03 18:31 UTC (permalink / raw)
To: Maxim Kokryashkin, tarantool-patches, skaplun
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)
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-10-03 18:31 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-29 13:38 [Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET* Maxim Kokryashkin via Tarantool-patches
2023-10-03 18:31 ` Sergey Bronnikov via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox