Re: [Tarantool-patches] [PATCH luajit] Check for IR_HREF vs. IR_HREFK aliasing in non-nil store check.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
LGTM, except for two nits below.
On Wed, Apr 24, 2024 at 01:37:20PM UTC, 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
<snipped>

> 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.
Typo: s/iteration/iterations/
> +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
Typo: s/afterthe/after the/
> +    -- 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
>