[Tarantool-patches] [PATCH luajit v2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

[Maxim Kokryashkin]

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