Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check.
@ 2024-04-24 10:37 Sergey Kaplun via Tarantool-patches
  2024-05-05 12:34 ` Maxim Kokryashkin via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-04-24 10:37 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

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


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-07-09  8:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-24 10:37 [Tarantool-patches] [PATCH luajit] Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check Sergey Kaplun via Tarantool-patches
2024-05-05 12:34 ` Maxim Kokryashkin via Tarantool-patches
2024-05-13 12:24   ` Sergey Kaplun via Tarantool-patches
2024-06-18 10:44 ` Sergey Bronnikov via Tarantool-patches
2024-07-09  8:05 ` Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox