Re: [Tarantool-patches] [PATCH luajit] Emit sunk IR_NEWREF only once per key on snapshot replay.

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

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Mon, Dec 11, 2023 at 06:35:20PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun and Peter Cawley.
>
> (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e)
<snipped>
> Resolves tarantool/tarantool#7937
> Part of tarantool/tarantool#9145
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1128-double-ir-newref-on-restore-sunk
> PR: https://github.com/tarantool/tarantool/pull/9466
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1128
> * https://github.com/tarantool/tarantool/issues/7937
>
>  src/lj_snap.c                                 | 16 ++++
>  ...-double-ir-newref-on-restore-sunk.test.lua | 81 +++++++++++++++++++
>  2 files changed, 97 insertions(+)
>  create mode 100644 test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
>
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 3f0fccec..73e18e69 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -583,9 +583,25 @@ void lj_snap_replay(jit_State *J, GCtrace *T)
<snipped>

> diff --git a/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
> new file mode 100644
> index 00000000..a89beab6
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1128-double-ir-newref-on-restore-sunk.test.lua
> @@ -0,0 +1,81 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT incorrect restoring of sunk
> +-- tables with double usage of IR_NEWREF.
> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1128.
> +
> +local test = tap.test('lj-1128-double-ir-newref-on-restore-sunk'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(3)
> +
> +local take_side
> +
> +local function trace_base(num)
> +  local tab = {}
> +  tab.key = false
> +  -- This check can't be folded since `num` can be NaN.
> +  tab.key = num == num
> +  -- luacheck: ignore
> +  -- This side trace emits the following IRs:
> +  -- 0001    tab TNEW   #0    #0
> +  -- 0002    p64 NEWREF 0001  "key"
> +  -- 0003    fal HSTORE 0002  false
> +  -- 0004    p64 NEWREF 0001  "key"
> +  -- 0005    tru HSTORE 0004  true
> +  -- As we can see, `NEWREF` is emitted twice. This is a violation
> +  -- of its semantics, so the second store isn't noticeable.
> +  if take_side then end
> +  return tab.key
> +end
> +
> +-- Uncompiled function to end up side trace here.
> +local function trace_base_wp(num)
> +  return trace_base(num)
> +end
> +jit.off(trace_base_wp)
> +
> +-- Same function as above, but with two IRs NEWREF emitted.
Please mention that this test cases checks situation when last NEWREF
is not the same.
> +local function trace_2newref(num)
> +  local tab = {}
> +  tab.key = false
> +  -- This + op can't be folded since `num` can be -0.
> +  tab.key = num + 0
> +  tab.key2 = false
> +  -- This check can't be folded since `num` can be NaN.
> +  tab.key2 = num == num
> +  -- luacheck: ignore
> +  if take_side then end
> +  return tab.key, tab.key2
> +end
Nit: `key` and `key2` naming seems a bit inconsistent.
> +
> +-- Uncompiled function to end up side trace here.
> +local function trace_2newref_wp(num)
> +  return trace_2newref(num)
> +end
> +jit.off(trace_2newref_wp)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1', 'tryside=1')
> +
> +-- Compile parent traces.
> +trace_base_wp(0)
> +trace_base_wp(0)
> +trace_2newref_wp(0)
> +trace_2newref_wp(0)
> +
> +-- Compile side traces.
> +take_side = true
> +trace_base_wp(0)
> +trace_base_wp(0)
> +trace_2newref_wp(0)
> +trace_2newref_wp(0)
> +
> +test:is(trace_base(0), true, 'sunk value restored correctly')
> +
> +local arg = 0
> +local r1, r2 = trace_2newref(arg)
> +test:is(r1, arg, 'sunk value restored correctly with 2 keys, first key')
> +test:is(r2, true, 'sunk value restored correctly with 2 keys, second key')
These assertions pass before the patch. Is this expected behavior? If
so, please drop a comment.
> +
> +test:done(true)
> --
> 2.43.0
>