[Tarantool-patches] [PATCH luajit] Avoid unpatching bytecode twice after a trace flush.

[Sergey Kaplun]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry picked from commit 85c3f2fb6f59276ebf07312859a69d6d5a897f62)

When flushing the already flushed trace, it is possible that another
trace is recorded for the same bytecode as the first trace. In that
case, the assertion fails in `trace_unpatch()`.

This patch fixes it by unpatching the trace only in the case if it
persists in the trace chain. Also, it deletes the dead code for the
trace unpatching logic, since the root trace can't start from `BC_JMP`
and the child traces are handled differently.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#11055
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1345-flushing-trace-twice

Note: CI is red due to problems with the integration testing.
See also: https://github.com/tarantool/tarantool/pull/11220

Related issues:
* https://github.com/tarantool/tarantool/issues/11055
* https://github.com/LuaJIT/LuaJIT/issues/1345

 src/lj_trace.c                                | 15 +++------
 .../lj-1345-flushing-trace-twice.test.lua     | 31 +++++++++++++++++++
 2 files changed, 35 insertions(+), 11 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1345-flushing-trace-twice.test.lua

diff --git a/src/lj_trace.c b/src/lj_trace.c
index 6b97cc13..f9b8ff00 100644
--- a/src/lj_trace.c
+++ b/src/lj_trace.c
@@ -235,14 +235,6 @@ static void trace_unpatch(jit_State *J, GCtrace *T)
 	       "bad original bytecode %d", op);
     *pc = T->startins;
     break;
-  case BC_JMP:
-    lj_assertJ(op == BC_ITERL, "bad original bytecode %d", op);
-    pc += bc_j(*pc)+2;
-    if (bc_op(*pc) == BC_JITERL) {
-      lj_assertJ(traceref(J, bc_d(*pc)) == T, "JITERL references other trace");
-      *pc = T->startins;
-    }
-    break;
   case BC_JFUNCF:
     lj_assertJ(op == BC_FUNCF, "bad original bytecode %d", op);
     *pc = T->startins;
@@ -258,18 +250,19 @@ static void trace_flushroot(jit_State *J, GCtrace *T)
   GCproto *pt = &gcref(T->startpt)->pt;
   lj_assertJ(T->root == 0, "not a root trace");
   lj_assertJ(pt != NULL, "trace has no prototype");
-  /* First unpatch any modified bytecode. */
-  trace_unpatch(J, T);
   /* Unlink root trace from chain anchored in prototype. */
   if (pt->trace == T->traceno) {  /* Trace is first in chain. Easy. */
     pt->trace = T->nextroot;
+unpatch:
+    /* Unpatch modified bytecode only if the trace has not been flushed. */
+    trace_unpatch(J, T);
   } else if (pt->trace) {  /* Otherwise search in chain of root traces. */
     GCtrace *T2 = traceref(J, pt->trace);
     if (T2) {
       for (; T2->nextroot; T2 = traceref(J, T2->nextroot))
 	if (T2->nextroot == T->traceno) {
 	  T2->nextroot = T->nextroot;  /* Unlink from chain. */
-	  break;
+	  goto unpatch;
 	}
     }
   }
diff --git a/test/tarantool-tests/lj-1345-flushing-trace-twice.test.lua b/test/tarantool-tests/lj-1345-flushing-trace-twice.test.lua
new file mode 100644
index 00000000..d5345227
--- /dev/null
+++ b/test/tarantool-tests/lj-1345-flushing-trace-twice.test.lua
@@ -0,0 +1,31 @@
+local tap = require('tap')
+
+-- Test file to demonstrate incorrect behaviour when LuaJIT
+-- flushes the trace twice when another trace for the same
+-- starting bytecode was recorded.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1345.
+local test = tap.test('lj-1345-flushing-trace-twice'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- Reset JIT.
+jit.flush()
+collectgarbage()
+
+jit.opt.start('hotloop=1')
+
+for _ = 1, 3  do
+  -- Nothing to flush on the first iteration. On the second
+  -- iteration, flushing the trace for the loop below (from the
+  -- first iteration). On the third iteration, another trace (from
+  -- the second iteration) is recorded for that loop.
+  -- This leads to the assertion failure before this patch.
+  jit.flush(1)
+  -- Record the loop with a trace.
+  for _ = 1, 4 do end
+end
+
+test:ok(true, 'no assertion failure during trace flushing')
+test:done(true)
-- 
2.48.1