<!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 <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: <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)->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: <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 > 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)
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>