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 5DDAD12E1AB1; Thu, 20 Mar 2025 15:22:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5DDAD12E1AB1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1742473369; bh=l/ELnfco7D1qn6dwL95qUp6MMJvUCbTLVvXw2Ln739k=; 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=nVF0fv3uUDGdM4G9bl1O8+eke9NM6zPB7wjfRdjDj/pQXaHXk+UQdwPCND0bVQwBo VhKI/Nr9B6OW89flakkeejJ8DJZafku3ZdlU8dcrhKL7yyse7nIV88TWRfZu4mTHF/ 4wmh7QCla5zHHeQCOe4bWcFNC2rdrzMA1GfHuRJw= Received: from send60.i.mail.ru (send60.i.mail.ru [89.221.237.155]) (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 05FCD7746BD for ; Thu, 20 Mar 2025 15:22:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 05FCD7746BD Received: by exim-smtp-75f69ddc6c-6vk5l with esmtpa (envelope-from ) id 1tvEv8-000000003HN-3l9m; Thu, 20 Mar 2025 15:22:47 +0300 Content-Type: multipart/alternative; boundary="------------00hvefqzrvER2pXB9mz0E9lW" Message-ID: Date: Thu, 20 Mar 2025 15:22:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20250227160013.2040-1-skaplun@tarantool.org> Content-Language: en-US In-Reply-To: <20250227160013.2040-1-skaplun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92A53477891B58793DCF94335131206D40D1B920360E5D1A3182A05F5380850403308492DF9AEBE453DE06ABAFEAF6705F4738C6CCBD3050FC93F2E030BC6C174357F9E8F5AEEDED5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70CB15FA6C489297DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB55337566C835A30D5A7E169F2014787C1E854AAD06D0D423A2AA9D7F163885A536272C2A389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B636DA1BED736F9328CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249D9EEDE0C260548BB76E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B13AA280BE71E98733AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B1D471462564A2E1935872C767BF85DA227C277FBC8AE2E8BA56E11165BA017C7EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A500397C34A8001D765002B1117B3ED6969CD7319E0350F3C933EE06AFCD964888823CB91A9FED034534781492E4B8EEADAE4FDBF11360AC9BBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D3407FE5477D6A8AF0813C3469DEA4C7EDBC264D6E2676DF6C57202A0F0E293CF0A51D5ED049125D59B1D7E09C32AA3244C39FCA29E560483A377DD89D51EBB7742C561E321CAC8483AEA455F16B58544A2E30DDF7C44BCB90DA5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVRxnlmV4XzQlRqiMlVjla5g= X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140CBB4CB980EAFAB11A6D5EE0DB6E1EC8D583EE38F97A5BB0D0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix IR_ABC hoisting. 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. --------------00hvefqzrvER2pXB9mz0E9lW Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hi, Sergey! thanks for the patch! LGTM with minor comments that can be ignored. Sergey On 27.02.2025 19:00, Sergey Kaplun wrote: > From: Mike Pall > > Reported by pwnhacker0x18. Fixed by Peter Cawley. > > (cherry picked from commit 7369eff67d46d7f5fac9ee064e3fbf97a15458de) > > The `IR_ABC` has 2 different types: > * `IRT_INT` for generic condition check. > * `IRT_P32` as a marker for the invariant checks for upper and lower > boundaries of the loop index. These checks may be dropped during the > folding optimization in recording the variant part of the loop. > The checks may be dropped if the instruction is not PHI, the first > operand is invariant, and the upper bound of the array part of the table > isn't changed, or the table itself isn't changed on the trace. > > The checks in the `abc_invar` fold rule assume that the constant values > of the first operand of ABC may be dropped. But when this constant value > comes from the folding chain that does CSE, the check is still necessary > if the original instruction (not resulting after loop optimization > substitution and the folding chain) isn't a constant. > > Hence, this patch additionally singularizes the `IRT_U32` type for the > `IR_ABC` in the case when the asize is a constant on the trace so it can > be simply dropped from the invariant part of the loop. For `IRT_P32` > type, this patch adds a check that the first operand isn't a constant to > be sure that it isn't a result of the fold chain. > > 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-1194-abc-hoisting > Related issues: > *https://github.com/LuaJIT/LuaJIT/issues/1194 > *https://github.com/tarantool/tarantool/issues/11055 > > src/lj_opt_fold.c | 5 +- > src/lj_record.c | 5 +- > .../lj-1194-abc-hoisting.test.lua | 77 +++++++++++++++++++ > 3 files changed, 83 insertions(+), 4 deletions(-) > create mode 100644 test/tarantool-tests/lj-1194-abc-hoisting.test.lua > > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c > index cd4395bb..83105816 100644 > --- a/src/lj_opt_fold.c > +++ b/src/lj_opt_fold.c > @@ -1906,9 +1906,10 @@ LJFOLDF(abc_k) > LJFOLD(ABC any any) > LJFOLDF(abc_invar) > { > - /* Invariant ABC marked as PTR. Drop if op1 is invariant, too. */ > + /* Invariant ABC marked as P32 or U32. Drop if op1 is invariant too. */ > if (!irt_isint(fins->t) && fins->op1 < J->chain[IR_LOOP] && > - !irt_isphi(IR(fins->op1)->t)) > + (irt_isu32(fins->t) || > + (!irref_isk(fins->op1) && !irt_isphi(IR(fins->op1)->t)))) > return DROPFOLD; > return NEXTFOLD; > } > diff --git a/src/lj_record.c b/src/lj_record.c > index 5345fa63..3dc6e840 100644 > --- a/src/lj_record.c > +++ b/src/lj_record.c > @@ -1315,12 +1315,13 @@ static void rec_idx_abc(jit_State *J, TRef asizeref, TRef ikey, uint32_t asize) > /* Runtime value for stop of loop is within bounds? */ > if ((uint64_t)stop + ofs < (uint64_t)asize) { > /* Emit invariant bounds check for stop. */ > - emitir(IRTG(IR_ABC, IRT_P32), asizeref, ofs == 0 ? J->scev.stop : > + uint32_t abc = IRTG(IR_ABC, tref_isk(asizeref) ? IRT_U32 : IRT_P32); > + emitir(abc, asizeref, ofs == 0 ? J->scev.stop : > emitir(IRTI(IR_ADD), J->scev.stop, ofsref)); > /* Emit invariant bounds check for start, if not const or negative. */ > if (!(J->scev.dir && J->scev.start && > (int64_t)IR(J->scev.start)->i + ofs >= 0)) > - emitir(IRTG(IR_ABC, IRT_P32), asizeref, ikey); > + emitir(abc, asizeref, ikey); > return; > } > } > diff --git a/test/tarantool-tests/lj-1194-abc-hoisting.test.lua b/test/tarantool-tests/lj-1194-abc-hoisting.test.lua > new file mode 100644 > index 00000000..3a78c34e > --- /dev/null > +++ b/test/tarantool-tests/lj-1194-abc-hoisting.test.lua > @@ -0,0 +1,77 @@ > +local tap = require('tap') > + > +-- Test file to demonstrate LuaJIT's incorrect hoisting out of the > +-- loop for Array Bound Check. > +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/1194. > + > +local test = tap.test('lj-1194-abc-hoisting'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > +}) > + > +test:plan(1) > + > +local table_new = require('table.new') > + > +-- Before the patch, the `for` cycle in the `test_func()` below > +-- produces the following trace: > +-- > +-- | 0006 int FLOAD 0005 tab.asize > +-- | 0007 > p32 ABC 0006 0001 > +-- | 0008 p32 FLOAD 0005 tab.array > +-- | 0009 p32 AREF 0008 0003 > +-- | 0010 tab FLOAD 0005 tab.meta > +-- | 0011 > tab EQ 0010 NULL > +-- | 0012 nil ASTORE 0009 nil > +-- | 0013 >+ tab TNEW #0 #0 > +-- | 0014 + int ADD 0003 +1 > +-- | 0015 > int LE 0014 0001 > +-- | 0016 ------ LOOP ------------ > +-- | 0017 > int NE 0014 +0 > +-- | 0018 p32 FLOAD 0013 tab.array > +-- | 0019 p32 AREF 0018 0014 > +-- | 0020 nil ASTORE 0019 nil > +-- > +-- As you can see, the `0007 ABC` instruction is dropped from the > +-- variant part of the loop. > + > +-- Disable fusing to simplify the reproducer it now will be crash > +-- on loading of an address from the `t->array`. > +jit.opt.start('hotloop=1', '-fuse') > + > +local function test_func() > + -- 1 iteration for hotcount warm-up. 2 and 3 to record invariant As usually we care about readers, I propose to make text more human-firendly: s/1/The first/ s/2 and 3/second and third/ > + -- and variant parts of the loop. The trace is run via an > + -- additional call to this function. > + local limit = 3 > + -- Create a table with a fixed array size (`limit` + 1), so all > + -- access to it fits into the array part. > + local s = table_new(limit, 0) > + for i = 1, limit do > + -- Don't change the table on hotcount warm-up. > + if i ~= 1 then > + -- `0020 ASTORE` causes the SegFault on the trace on the > + -- last (3rd) iteration, since the table (set on the first > + -- iteration) has `asize == 0`, and t->array == NULL`. > + -- luacheck: no unused > + s[i] = nil > + s = {} > + end > + end > +end > + > +-- Compile the `for` trace inside `test_func()`. > +-- The invariant part of this trace contains the ABC check, while > +-- the invariant does not. So, first compile the trace, then use > +-- the second call to run it from the beginning with all guards > +-- passing in the invariant part. > +test_func() > + > +-- Avoid compilation of any other traces. > +jit.off() > + > +-- Run that trace. > +test_func() > + > +test:ok(true, 'no crash on the trace due to incorrect ABC hoisting') > + > +test:done(true) --------------00hvefqzrvER2pXB9mz0E9lW Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit

Hi, Sergey!

thanks for the patch! LGTM with minor comments that can be ignored.

Sergey

On 27.02.2025 19:00, Sergey Kaplun wrote:
From: Mike Pall <mike>

Reported by pwnhacker0x18. Fixed by Peter Cawley.

(cherry picked from commit 7369eff67d46d7f5fac9ee064e3fbf97a15458de)

The `IR_ABC` has 2 different types:
* `IRT_INT` for generic condition check.
* `IRT_P32` as a marker for the invariant checks for upper and lower
  boundaries of the loop index. These checks may be dropped during the
  folding optimization in recording the variant part of the loop.
The checks may be dropped if the instruction is not PHI, the first
operand is invariant, and the upper bound of the array part of the table
isn't changed, or the table itself isn't changed on the trace.

The checks in the `abc_invar` fold rule assume that the constant values
of the first operand of ABC may be dropped. But when this constant value
comes from the folding chain that does CSE, the check is still necessary
if the original instruction (not resulting after loop optimization
substitution and the folding chain) isn't a constant.

Hence, this patch additionally singularizes the `IRT_U32` type for the
`IR_ABC` in the case when the asize is a constant on the trace so it can
be simply dropped from the invariant part of the loop. For `IRT_P32`
type, this patch adds a check that the first operand isn't a constant to
be sure that it isn't a result of the fold chain.

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-1194-abc-hoisting
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1194
* https://github.com/tarantool/tarantool/issues/11055

 src/lj_opt_fold.c                             |  5 +-
 src/lj_record.c                               |  5 +-
 .../lj-1194-abc-hoisting.test.lua             | 77 +++++++++++++++++++
 3 files changed, 83 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1194-abc-hoisting.test.lua

diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index cd4395bb..83105816 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -1906,9 +1906,10 @@ LJFOLDF(abc_k)
 LJFOLD(ABC any any)
 LJFOLDF(abc_invar)
 {
-  /* Invariant ABC marked as PTR. Drop if op1 is invariant, too. */
+  /* Invariant ABC marked as P32 or U32. Drop if op1 is invariant too. */
   if (!irt_isint(fins->t) && fins->op1 < J->chain[IR_LOOP] &&
-      !irt_isphi(IR(fins->op1)->t))
+      (irt_isu32(fins->t) ||
+       (!irref_isk(fins->op1) && !irt_isphi(IR(fins->op1)->t))))
     return DROPFOLD;
   return NEXTFOLD;
 }
diff --git a/src/lj_record.c b/src/lj_record.c
index 5345fa63..3dc6e840 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -1315,12 +1315,13 @@ static void rec_idx_abc(jit_State *J, TRef asizeref, TRef ikey, uint32_t asize)
       /* Runtime value for stop of loop is within bounds? */
       if ((uint64_t)stop + ofs < (uint64_t)asize) {
 	/* Emit invariant bounds check for stop. */
-	emitir(IRTG(IR_ABC, IRT_P32), asizeref, ofs == 0 ? J->scev.stop :
+	uint32_t abc = IRTG(IR_ABC, tref_isk(asizeref) ? IRT_U32 : IRT_P32);
+	emitir(abc, asizeref, ofs == 0 ? J->scev.stop :
 	       emitir(IRTI(IR_ADD), J->scev.stop, ofsref));
 	/* Emit invariant bounds check for start, if not const or negative. */
 	if (!(J->scev.dir && J->scev.start &&
 	      (int64_t)IR(J->scev.start)->i + ofs >= 0))
-	  emitir(IRTG(IR_ABC, IRT_P32), asizeref, ikey);
+	  emitir(abc, asizeref, ikey);
 	return;
       }
     }
diff --git a/test/tarantool-tests/lj-1194-abc-hoisting.test.lua b/test/tarantool-tests/lj-1194-abc-hoisting.test.lua
new file mode 100644
index 00000000..3a78c34e
--- /dev/null
+++ b/test/tarantool-tests/lj-1194-abc-hoisting.test.lua
@@ -0,0 +1,77 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's incorrect hoisting out of the
+-- loop for Array Bound Check.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1194.
+
+local test = tap.test('lj-1194-abc-hoisting'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local table_new = require('table.new')
+
+-- Before the patch, the `for` cycle in the `test_func()` below
+-- produces the following trace:
+--
+-- | 0006    int FLOAD  0005  tab.asize
+-- | 0007 >  p32 ABC    0006  0001
+-- | 0008    p32 FLOAD  0005  tab.array
+-- | 0009    p32 AREF   0008  0003
+-- | 0010    tab FLOAD  0005  tab.meta
+-- | 0011 >  tab EQ     0010  NULL
+-- | 0012    nil ASTORE 0009  nil
+-- | 0013 >+ tab TNEW   #0    #0
+-- | 0014  + int ADD    0003  +1
+-- | 0015 >  int LE     0014  0001
+-- | 0016 ------ LOOP ------------
+-- | 0017 >  int NE     0014  +0
+-- | 0018    p32 FLOAD  0013  tab.array
+-- | 0019    p32 AREF   0018  0014
+-- | 0020    nil ASTORE 0019  nil
+--
+-- As you can see, the `0007 ABC` instruction is dropped from the
+-- variant part of the loop.
+
+-- Disable fusing to simplify the reproducer it now will be crash
+-- on loading of an address from the `t->array`.
+jit.opt.start('hotloop=1', '-fuse')
+
+local function test_func()
+  -- 1 iteration for hotcount warm-up. 2 and 3 to record invariant

As usually we care about readers, I propose to make text more human-firendly:

s/1/The first/

s/2 and 3/second and third/

+  -- and variant parts of the loop. The trace is run via an
+  -- additional call to this function.
+  local limit = 3
+  -- Create a table with a fixed array size (`limit` + 1), so all
+  -- access to it fits into the array part.
+  local s = table_new(limit, 0)
+  for i = 1, limit do
+    -- Don't change the table on hotcount warm-up.
+    if i ~= 1 then
+      -- `0020 ASTORE` causes the SegFault on the trace on the
+      -- last (3rd) iteration, since the table (set on the first
+      -- iteration) has `asize == 0`, and t->array == NULL`.
+      -- luacheck: no unused
+      s[i] = nil
+      s = {}
+    end
+  end
+end
+
+-- Compile the `for` trace inside `test_func()`.
+-- The invariant part of this trace contains the ABC check, while
+-- the invariant does not. So, first compile the trace, then use
+-- the second call to run it from the beginning with all guards
+-- passing in the invariant part.
+test_func()
+
+-- Avoid compilation of any other traces.
+jit.off()
+
+-- Run that trace.
+test_func()
+
+test:ok(true, 'no crash on the trace due to incorrect ABC hoisting')
+
+test:done(true)
--------------00hvefqzrvER2pXB9mz0E9lW--