[Tarantool-patches] [PATCH luajit] Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check.
Sergey Bronnikov
sergeyb at tarantool.org
Tue Jun 18 13:44:34 MSK 2024
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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20240618/b4456dfb/attachment.htm>
More information about the Tarantool-patches
mailing list