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

[Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

From: Mike Pall <mike>

Reported by Arseny Vakhrushev.
Fix contributed by Peter Cawley.

(cherry-picked from commit 5c46f47736f7609be407c88d531ecd1689d40a79)

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.

NOTICE: This patch is only a part of the original commit,
and the other part is backported in the previous commit. The
patch was split into two, so the test case becomes easier to
implement since it can now depend on this assertion instead
of memory layout.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#9145
---
 src/lj_record.c                               |  9 +-
 .../lj-624-jloop-snapshot-pc.test.lua         | 84 +++++++++++++++++++
 2 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/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..726b2efa
--- /dev/null
+++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
@@ -0,0 +1,84 @@
+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.
+--
+-- See also:
+-- https://github.com/luaJIT/LuaJIT/issues/624
+
+
+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.39.3 (Apple Git-145)