From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
Received: from [87.239.111.99] (localhost [127.0.0.1])
	by dev.tarantool.org (Postfix) with ESMTP id D6C65D82141;
	Tue, 18 Jun 2024 13:44:36 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D6C65D82141
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev;
	t=1718707477; bh=QI7SS+J1mTy2arfJLTj9nfmlCWbQxfbDQf7OzP1Z33A=;
	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=Z4cCah3UKhvUH30+bdIDCnjnMbfmLM7M4Yxn04AtGtCY0XqMJa8R4k9zi8eS8GPOs
	 IwfXFjSYqc1Q9b8TlqA8V1yeAro3ftKQUJBRnNSmmRfhNP5K4Km7ohTRwJZRGH2aPD
	 A7QQHZ82Xbfxi3y6xQ4bKcu//232ktEYcC6vpVUs=
Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [95.163.41.65])
 (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 54F96D82141
 for <tarantool-patches@dev.tarantool.org>;
 Tue, 18 Jun 2024 13:44:35 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 54F96D82141
Received: by exim-smtp-dbbd7b44b-7mms2 with esmtpa (envelope-from
 <sergeyb@tarantool.org>)
 id 1sJWKI-00000000MBZ-1pyI; Tue, 18 Jun 2024 13:44:34 +0300
Content-Type: multipart/alternative;
 boundary="------------J1AMuSKa0mQHW2o0cGYkQP7B"
Message-ID: <3d7669c8-0ad9-4714-9093-5a8213442124@tarantool.org>
Date: Tue, 18 Jun 2024 13:44:34 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Sergey Kaplun <skaplun@tarantool.org>,
 Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
References: <20240424103720.9464-1-skaplun@tarantool.org>
In-Reply-To: <20240424103720.9464-1-skaplun@tarantool.org>
X-Mailru-Src: smtp
X-4EC0790: 10
X-7564579A: 646B95376F6C166E
X-77F55803: 4F1203BC0FB41BD9EB52AADBDB11C14927CFFAF1C6212F6AB55D068E3B3A9FE4182A05F53808504025D77450BD84A5CE3DE06ABAFEAF6705298650EA9C3A7C78E86B39BB833B052C209524BCEF20D7BA
X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE746D93DAA4671895CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372521E7C1CE72986C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89E5830A6CD46D20C687B43258BEF969F7D62C68954EC0B29CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F924B32C592EA89F389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C2D01283D1ACF37BAC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C36E36DCD5FF651F90BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7AEA1580DED4E70E3731C566533BA786AA5CC5B56E945C8DA
X-C1DE0DAB: 0D63561A33F958A56198F92094C375CF5002B1117B3ED6968E4843F94AE410A4A13BD6A4B0E00B96823CB91A9FED034534781492E4B8EEAD220496FFA5CD4785BDAD6C7F3747799A
X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFEDDB41815C847BE8274E475DE122FA7A682BFA313192C5FACBC8ABE51650C671110A92F5D16EB67F1521D91D49C923C7EB2B3DB2ECEBFC710DCC4675CE65115BBC32F018DFB5CBD85F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203
X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJvJpr60SBvLb4jlVc97XpA==
X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140B6D03DEB629BE434EAE04967732FC4582272F5BDF971F09D0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367
X-Mras: Ok
Subject: Re: [Tarantool-patches] [PATCH luajit] Check for IR_HREF vs.
 IR_HREFK aliasing in non-nil store check.
X-BeenThere: tarantool-patches@dev.tarantool.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
From: Sergey Bronnikov via Tarantool-patches
 <tarantool-patches@dev.tarantool.org>
Reply-To: Sergey Bronnikov <sergeyb@tarantool.org>
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@dev.tarantool.org>

This is a multi-part message in MIME format.
--------------J1AMuSKa0mQHW2o0cGYkQP7B
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi, Sergey

thanks for the patch! LGTM

On 24.04.2024 13:37, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit 658530562c2ac7ffa8e4ca5d18856857471244e9)
>
> The `lj_opt_fwd_wasnonnil()` skips the check for HREF and HREFK that may
> alias. Hence, the guard for the non-nil value may be skipped, and the
> `__newindex` metamethod call is omitted too.
>
> This patch adds the aforementioned check for different reference types
> (HREF vs. HREFK), which were not detected by the previous analysis.
> Also, the helper macro `irt_isp32()` is introduced to check that the IR
> type is `IRT_P32` (KSLOT type).
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias
> Related issues:
> *https://github.com/tarantool/tarantool/issues/9924
> *https://github.com/LuaJIT/LuaJIT/issues/1133
>
>   src/lj_ir.h                                   |  1 +
>   src/lj_opt_mem.c                              |  2 +
>   .../lj-1133-fwd-href-hrefk-alias.test.lua     | 94 +++++++++++++++++++
>   3 files changed, 97 insertions(+)
>   create mode 100644 test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua
>
> diff --git a/src/lj_ir.h b/src/lj_ir.h
> index e9b8328e..27c66f63 100644
> --- a/src/lj_ir.h
> +++ b/src/lj_ir.h
> @@ -367,6 +367,7 @@ typedef struct IRType1 { uint8_t irt; } IRType1;
>   #define irt_isu32(t)		(irt_type(t) == IRT_U32)
>   #define irt_isi64(t)		(irt_type(t) == IRT_I64)
>   #define irt_isu64(t)		(irt_type(t) == IRT_U64)
> +#define irt_isp32(t)		(irt_type(t) == IRT_P32)
>   
>   #define irt_isfp(t)		(irt_isnum(t) || irt_isfloat(t))
>   #define irt_isinteger(t)	(irt_typerange((t), IRT_I8, IRT_INT))
> diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
> index 9044f09a..c9f1216c 100644
> --- a/src/lj_opt_mem.c
> +++ b/src/lj_opt_mem.c
> @@ -916,6 +916,8 @@ int lj_opt_fwd_wasnonnil(jit_State *J, IROpT loadop, IRRef xref)
>   	if (skref == xkref || !irref_isk(skref) || !irref_isk(xkref))
>   	  return 0;  /* A nil store with same const key or var key MAY alias. */
>   	/* Different const keys CANNOT alias. */
> +      } else if (irt_isp32(IR(skref)->t) != irt_isp32(IR(xkref)->t)) {
> +	return 0;  /* HREF and HREFK MAY alias. */
>         }  /* Different key types CANNOT alias. */
>       }  /* Other non-nil stores MAY alias. */
>       ref = store->prev;
> diff --git a/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua
> new file mode 100644
> index 00000000..6b72c97a
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua
> @@ -0,0 +1,94 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate the LuaJIT's incorrect aliasing check
> +-- for HREFK and HREF IRs during the non-nil check.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1133.
> +
> +local test = tap.test('lj-1133-fwd-href-hrefk-alias'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +test:plan(1)
> +
> +local rawset = rawset
> +
> +-- The maximum value that can be stored in a 16-bit `op2`
> +-- field in HREFK IR.
> +local HASH_NODES = 65535
> +
> +-- Amount of iteration to compile and execute the trace.
> +local LOOP_LIMIT = 4
> +
> +-- Function to be called twice to emit the trace and take the side
> +-- exit.
> +local function trace_aliased_tables(t1, t2)
> +  -- The criteria is the number of new index creations.
> +  local count = 0
> +  local mt = {__newindex = function(t, k, v)
> +    count = count + 1
> +    rawset(t, k, v)
> +  end}
> +  setmetatable(t1, mt)
> +  setmetatable(t2, mt)
> +
> +  for _ = 1, LOOP_LIMIT do
> +    -- XXX: Keys values have no special meaning here, just be sure
> +    -- that they are HREF/HREFK and not in the array table part.
> +    -- `t1` is empty, emitting HREFK.
> +    t1[10] = 1
> +    -- `t2` on recording has more than `HASH_NODES` table nodes,
> +    -- so this emits HREF.
> +    t2[10] = nil
> +    -- Resolve `__newindex` if t1 == t2.
> +    -- `lj_opt_fwd_wasnonnil()` missed the check that HREFK and
> +    -- HREF may alias before the patch, so the guarded HLOAD IR
> +    -- with the corresponding snapshot is skipped.
> +    -- The difference in the emitted IR before and afterthe patch
> +    -- is the following:
> +    -- |  0004 >  tab SLOAD  #1    T
> +    -- |              ...
> +    -- |  0009    p32 FLOAD  0004  tab.node
> +    -- |  0010 >  p32 HREFK  0009  +10  @0
> +    -- |  0011 >  num HLOAD  0010
> +    -- |  0012    num HSTORE 0010  +1
> +    -- |  ....        SNAP   #1
> +    -- |  0013 >  tab SLOAD  #2    T
> +    -- |  0014    int FLOAD  0013  tab.asize
> +    -- |  0015 >  int ULE    0014  +10
> +    -- |  0016    p32 HREF   0013  +10
> +    -- |  0017 >  p32 NE     0016  [0x415554e8]
> +    -- |  0018 >  num HLOAD  0016
> +    -- |  0019    nil HSTORE 0016  nil
> +    -- | -0020    num HSTORE 0010  +30
> +    -- |  ....        SNAP   #2
> +    -- | +0020 >  num HLOAD  0010
> +    -- | +0021    num HSTORE 0010  +30
> +    -- | +....        SNAP   #3
> +    --
> +    -- Hence, the taken exit is not resolving `__newindex` before
> +    -- the patch.
> +    t1[10] = 1
> +    -- The exit 2 of the trace is here.
> +    -- Resolve `__newindex` if t1 ~= t2.
> +    t2[10] = 1
> +  end
> +  -- `__newindex` is called twice on the first iteration and once
> +  -- on each other.
> +  return count == LOOP_LIMIT + 1
> +end
> +
> +-- Create a big table to emit HREF IR (not HREFK) to trick
> +-- the alias checking.
> +local bigt = {}
> +for i = 1, HASH_NODES + 1 do
> +  bigt[-i] = true
> +end
> +
> +jit.opt.start('hotloop=1')
> +
> +trace_aliased_tables({}, bigt)
> +
> +-- Now use tables that are aliased.
> +local smallt = {}
> +test:ok(trace_aliased_tables(smallt, smallt), 'aliasing check is correct')
> +
> +test:done(true)
--------------J1AMuSKa0mQHW2o0cGYkQP7B
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<!DOCTYPE html>
<html data-lt-installed="true">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body style="padding-bottom: 1px;">
    <p>Hi, Sergey</p>
    <p>thanks for the patch! LGTM<br>
    </p>
    <div class="moz-cite-prefix">On 24.04.2024 13:37, Sergey Kaplun
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20240424103720.9464-1-skaplun@tarantool.org">
      <pre class="moz-quote-pre" wrap="">From: Mike Pall &lt;mike&gt;

Thanks to Peter Cawley.

(cherry picked from commit 658530562c2ac7ffa8e4ca5d18856857471244e9)

The `lj_opt_fwd_wasnonnil()` skips the check for HREF and HREFK that may
alias. Hence, the guard for the non-nil value may be skipped, and the
`__newindex` metamethod call is omitted too.

This patch adds the aforementioned check for different reference types
(HREF vs. HREFK), which were not detected by the previous analysis.
Also, the helper macro `irt_isp32()` is introduced to check that the IR
type is `IRT_P32` (KSLOT type).

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

Part of tarantool/tarantool#9924
---

Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias">https://github.com/tarantool/luajit/tree/skaplun/lj-1133-fwd-href-hrefk-alias</a>
Related issues:
* <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/9924">https://github.com/tarantool/tarantool/issues/9924</a>
* <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1133">https://github.com/LuaJIT/LuaJIT/issues/1133</a>

 src/lj_ir.h                                   |  1 +
 src/lj_opt_mem.c                              |  2 +
 .../lj-1133-fwd-href-hrefk-alias.test.lua     | 94 +++++++++++++++++++
 3 files changed, 97 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua

diff --git a/src/lj_ir.h b/src/lj_ir.h
index e9b8328e..27c66f63 100644
--- a/src/lj_ir.h
+++ b/src/lj_ir.h
@@ -367,6 +367,7 @@ typedef struct IRType1 { uint8_t irt; } IRType1;
 #define irt_isu32(t)		(irt_type(t) == IRT_U32)
 #define irt_isi64(t)		(irt_type(t) == IRT_I64)
 #define irt_isu64(t)		(irt_type(t) == IRT_U64)
+#define irt_isp32(t)		(irt_type(t) == IRT_P32)
 
 #define irt_isfp(t)		(irt_isnum(t) || irt_isfloat(t))
 #define irt_isinteger(t)	(irt_typerange((t), IRT_I8, IRT_INT))
diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
index 9044f09a..c9f1216c 100644
--- a/src/lj_opt_mem.c
+++ b/src/lj_opt_mem.c
@@ -916,6 +916,8 @@ int lj_opt_fwd_wasnonnil(jit_State *J, IROpT loadop, IRRef xref)
 	if (skref == xkref || !irref_isk(skref) || !irref_isk(xkref))
 	  return 0;  /* A nil store with same const key or var key MAY alias. */
 	/* Different const keys CANNOT alias. */
+      } else if (irt_isp32(IR(skref)-&gt;t) != irt_isp32(IR(xkref)-&gt;t)) {
+	return 0;  /* HREF and HREFK MAY alias. */
       }  /* Different key types CANNOT alias. */
     }  /* Other non-nil stores MAY alias. */
     ref = store-&gt;prev;
diff --git a/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua
new file mode 100644
index 00000000..6b72c97a
--- /dev/null
+++ b/test/tarantool-tests/lj-1133-fwd-href-hrefk-alias.test.lua
@@ -0,0 +1,94 @@
+local tap = require('tap')
+
+-- Test file to demonstrate the LuaJIT's incorrect aliasing check
+-- for HREFK and HREF IRs during the non-nil check.
+-- See also: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1133">https://github.com/LuaJIT/LuaJIT/issues/1133</a>.
+
+local test = tap.test('lj-1133-fwd-href-hrefk-alias'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+test:plan(1)
+
+local rawset = rawset
+
+-- The maximum value that can be stored in a 16-bit `op2`
+-- field in HREFK IR.
+local HASH_NODES = 65535
+
+-- Amount of iteration to compile and execute the trace.
+local LOOP_LIMIT = 4
+
+-- Function to be called twice to emit the trace and take the side
+-- exit.
+local function trace_aliased_tables(t1, t2)
+  -- The criteria is the number of new index creations.
+  local count = 0
+  local mt = {__newindex = function(t, k, v)
+    count = count + 1
+    rawset(t, k, v)
+  end}
+  setmetatable(t1, mt)
+  setmetatable(t2, mt)
+
+  for _ = 1, LOOP_LIMIT do
+    -- XXX: Keys values have no special meaning here, just be sure
+    -- that they are HREF/HREFK and not in the array table part.
+    -- `t1` is empty, emitting HREFK.
+    t1[10] = 1
+    -- `t2` on recording has more than `HASH_NODES` table nodes,
+    -- so this emits HREF.
+    t2[10] = nil
+    -- Resolve `__newindex` if t1 == t2.
+    -- `lj_opt_fwd_wasnonnil()` missed the check that HREFK and
+    -- HREF may alias before the patch, so the guarded HLOAD IR
+    -- with the corresponding snapshot is skipped.
+    -- The difference in the emitted IR before and afterthe patch
+    -- is the following:
+    -- |  0004 &gt;  tab SLOAD  #1    T
+    -- |              ...
+    -- |  0009    p32 FLOAD  0004  tab.node
+    -- |  0010 &gt;  p32 HREFK  0009  +10  @0
+    -- |  0011 &gt;  num HLOAD  0010
+    -- |  0012    num HSTORE 0010  +1
+    -- |  ....        SNAP   #1
+    -- |  0013 &gt;  tab SLOAD  #2    T
+    -- |  0014    int FLOAD  0013  tab.asize
+    -- |  0015 &gt;  int ULE    0014  +10
+    -- |  0016    p32 HREF   0013  +10
+    -- |  0017 &gt;  p32 NE     0016  [0x415554e8]
+    -- |  0018 &gt;  num HLOAD  0016
+    -- |  0019    nil HSTORE 0016  nil
+    -- | -0020    num HSTORE 0010  +30
+    -- |  ....        SNAP   #2
+    -- | +0020 &gt;  num HLOAD  0010
+    -- | +0021    num HSTORE 0010  +30
+    -- | +....        SNAP   #3
+    --
+    -- Hence, the taken exit is not resolving `__newindex` before
+    -- the patch.
+    t1[10] = 1
+    -- The exit 2 of the trace is here.
+    -- Resolve `__newindex` if t1 ~= t2.
+    t2[10] = 1
+  end
+  -- `__newindex` is called twice on the first iteration and once
+  -- on each other.
+  return count == LOOP_LIMIT + 1
+end
+
+-- Create a big table to emit HREF IR (not HREFK) to trick
+-- the alias checking.
+local bigt = {}
+for i = 1, HASH_NODES + 1 do
+  bigt[-i] = true
+end
+
+jit.opt.start('hotloop=1')
+
+trace_aliased_tables({}, bigt)
+
+-- Now use tables that are aliased.
+local smallt = {}
+test:ok(trace_aliased_tables(smallt, smallt), 'aliasing check is correct')
+
+test:done(true)
</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>

--------------J1AMuSKa0mQHW2o0cGYkQP7B--