From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id B3F3212D3427; Wed, 12 Mar 2025 15:55:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B3F3212D3427 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1741784104; bh=t3fhaFLmU5+v4O1TADcjApoY4XmN6ecc0C47slF+h6k=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=hh/G6jHm9R30MvLnHYcTNwwa8Rx6YifZRsmuSVqfWAzdMnSguyxjsi1+YM5zeD4ah 6fRP9HQph+T1ZDd4IXGWVpglKqn63MPQvEeR8bXV9kI97BJcB5d33nhSi/5McaqMm3 7Du81ztGjee4uenSuVv1JCiBnmSGKceQTeYCg2k8= Received: from send126.i.mail.ru (send126.i.mail.ru [89.221.237.221]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2774612D3420 for ; Wed, 12 Mar 2025 15:55:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2774612D3420 Received: by exim-smtp-69cc44787d-jk6tv with esmtpa (envelope-from ) id 1tsLbz-00000000ChE-0RZ8; Wed, 12 Mar 2025 15:55:03 +0300 Content-Type: multipart/alternative; boundary="------------zVV768U4VGCCmXEV6n1jMJt6" Message-ID: <4aa05c55-caee-43ee-8d38-359aee9b3cb6@tarantool.org> Date: Wed, 12 Mar 2025 15:55:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250311175226.7707-1-skaplun@tarantool.org> In-Reply-To: <20250311175226.7707-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9CB00722163A93FBB39214A33DDD1A8CB1424F4E1C3FDCD45CD62213F67905E7A670A94B9B7AAE3D31B71F2AEB6796A6EF14AAA3D5FE6C2720AE283FE3EEDFB0608D917D6130B1AFB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A179494B5629353BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566A1031DB47C569B6074D07D7D8E098AF5B80179F0094AC2D58C1DF64C9A06B642389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C091DAD9F922AA71188941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6A70DDFFB3186CBC5CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249D9EEDE0C260548BB76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B13AA280BE71E98733AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735D028CC0B556B22BCC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5667FE9BD0D52836B5002B1117B3ED696553B0B634A9EADB4957033528158102E823CB91A9FED034534781492E4B8EEAD5B606B10FC07407CBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34F1257DC9690AEBA135298972CCE87D7EDC2F9C1CC46B13ED5DF59FDFE463B28839D579E7E3546EA91D7E09C32AA3244CF2E8155BFC20BD4E77DD89D51EBB7742CB71424153CBCCF6EA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQiWK+2I7Y2sd2P4EYJcq0w= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61401B0E50FC05B02D190578E6996F3834134745B8C80272880D0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Avoid unpatching bytecode twice after a trace flush. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------zVV768U4VGCCmXEV6n1jMJt6 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey, thanks for the patch! LGTM Sergey On 11.03.2025 20:52, Sergey Kaplun wrote: > From: Mike Pall > > 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) --------------zVV768U4VGCCmXEV6n1jMJt6 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

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 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)
--------------zVV768U4VGCCmXEV6n1jMJt6--