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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4361 bytes --]

Hi, Sergey,

thanks for the patch! LGTM

Sergey

On 11.03.2025 20:52, Sergey Kaplun wrote:
> 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 alsohttps://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)

[-- Attachment #2: Type: text/html, Size: 5164 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.3 [2], release/3.2 [3]
and release/2.11 [4].

[1]: https://github.com/tarantool/tarantool/pull/11281
[2]: https://github.com/tarantool/tarantool/pull/11282
[3]: https://github.com/tarantool/tarantool/pull/11283
[4]: https://github.com/tarantool/tarantool/pull/11284

-- 
Best regards,
Sergey Kaplun