[Tarantool-patches] [PATCH luajit] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.
Maxim Kokryashkin
max.kokryashkin at gmail.com
Thu Sep 21 16:15:31 MSK 2023
From: Mike Pall <mike>
Reported by Arseny Vakhrushev.
Fix contributed by Peter Cawley.
As specified in lj_record.c:304, all loops must set `J->pc` to
the next instruction. However, the chunk of logic at
lj_trace.c:923 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
---
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc
PR: https://github.com/tarantool/tarantool/pull/9166
NB: The test for this patch triggers the assertion added in this patch,
however I had no luck making a __stable__ reproducer for the issue,
since it depends on what's in memory after the BC_JLOOP. It is easier to
achieve a consitent failures if ASLR is disabled, but it's not suitable
for the testing purposes.
src/lj_record.c | 9 +++++----
src/lj_snap.c | 3 +++
.../lj-624-jloop-snapshot-pc.test.lua | 16 ++++++++++++++++
3 files changed, 24 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..ada290ff
--- /dev/null
+++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
@@ -0,0 +1,16 @@
+local tap = require('tap')
+local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({
+ ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+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
More information about the Tarantool-patches
mailing list