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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]