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

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun and Peter Cawley.

(cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e)

Assume we have the parent trace with the following IRs:

| 0001 }  tab TNEW   #0    #0
| 0002 }  p32 NEWREF 0001  "key"
| 0003 }  fal HSTORE 0002  false
| ....        SNAP   #1   [ ---- ---- 0001 ---- ]
| 0004 >  num SLOAD  #1    T
| ....        SNAP   #2   [ ---- ---- 0001 ]
| 0005 >  num EQ     0004  0004
| 0006 }  tru HSTORE 0002  true
| ....        SNAP   #3   [ ---- ---- 0001 true ]

The side trace for the third snapshot emits the following IRs:

| 0001    tab TNEW   #0    #0
| 0002    p32 NEWREF 0001  "key"
| 0003    fal HSTORE 0002  false
| 0004    p32 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.

This patch prevents the second emitting of IR NEWREF by checking the last
one emitted NEWREF IR. There is no need to check NEWREFs beyond since
it guarantees the snapshot is taken after it, because it may cause table
rehashing, so all prior results are invalidated.

Sergey Kaplun:
* added the description and the test for the problem

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)
 		if (irr->o == IR_HREFK || irr->o == IR_AREF) {
 		  IRIns *irf = &T->ir[irr->op1];
 		  tmp = emitir(irf->ot, tmp, irf->op2);
+		} else if (irr->o == IR_NEWREF) {
+		  IRRef allocref = tref_ref(tr);
+		  IRRef keyref = tref_ref(key);
+		  IRRef newref_ref = J->chain[IR_NEWREF];
+		  IRIns *newref = &J->cur.ir[newref_ref];
+		  lj_assertJ(irref_isk(keyref),
+			     "sunk store for parent IR %04d with bad key %04d",
+			     refp - REF_BIAS, keyref - REF_BIAS);
+		  if (newref_ref > allocref && newref->op2 == keyref) {
+		    lj_assertJ(newref->op1 == allocref,
+			       "sunk store for parent IR %04d with bad tab %04d",
+			       refp - REF_BIAS, allocref - REF_BIAS);
+		    tmp = newref_ref;
+		    goto skip_newref;
+		  }
 		}
 	      }
 	      tmp = emitir(irr->ot, tmp, key);
+	    skip_newref:
 	      val = snap_pref(J, T, map, nent, seen, irs->op2);
 	      if (val == 0) {
 		IRIns *irc = &T->ir[irs->op2];
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.
+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
+
+-- 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')
+
+test:done(true)
-- 
2.43.0

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

[Maxim Kokryashkin via Tarantool-patches]

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
>

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 12.12.23, Maxim Kokryashkin wrote:
> 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>

> > +-- 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.

Fixed, see the iterative patch below.

> > +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.

Fixed, thanks!

===================================================================
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
index a89beab6..77efd0f4 100644
--- 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
@@ -37,17 +37,18 @@ end
 jit.off(trace_base_wp)
 
 -- Same function as above, but with two IRs NEWREF emitted.
+-- The last NEWREF references another key.
 local function trace_2newref(num)
   local tab = {}
-  tab.key = false
+  tab.key1 = false
   -- This + op can't be folded since `num` can be -0.
-  tab.key = num + 0
+  tab.key1 = 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
+  return tab.key1, tab.key2
 end
 
 -- Uncompiled function to end up side trace here.
===================================================================

> > +
> > +-- 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.

Added:

===================================================================
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
index a89beab6..77efd0f4 100644
--- 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
@@ -75,6 +76,8 @@ test:is(trace_base(0), true, 'sunk value restored correctly')
 
 local arg = 0
 local r1, r2 = trace_2newref(arg)
+-- These tests didn't fail before the patch.
+-- They check the patch's correctness.
 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')
===================================================================

> > +
> > +test:done(true)
> > --
> > 2.43.0
> >

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM

On Tue, Dec 12, 2023 at 01:10:00PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
>
> On 12.12.23, Maxim Kokryashkin wrote:
> > 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>
>
> > > +-- 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.
>
> Fixed, see the iterative patch below.
>
> > > +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.
>
> Fixed, thanks!
>
> ===================================================================
> 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
> index a89beab6..77efd0f4 100644
> --- 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
> @@ -37,17 +37,18 @@ end
>  jit.off(trace_base_wp)
>
>  -- Same function as above, but with two IRs NEWREF emitted.
> +-- The last NEWREF references another key.
>  local function trace_2newref(num)
>    local tab = {}
> -  tab.key = false
> +  tab.key1 = false
>    -- This + op can't be folded since `num` can be -0.
> -  tab.key = num + 0
> +  tab.key1 = 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
> +  return tab.key1, tab.key2
>  end
>
>  -- Uncompiled function to end up side trace here.
> ===================================================================
>
> > > +
> > > +-- 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.
>
> Added:
>
> ===================================================================
> 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
> index a89beab6..77efd0f4 100644
> --- 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
> @@ -75,6 +76,8 @@ test:is(trace_base(0), true, 'sunk value restored correctly')
>
>  local arg = 0
>  local r1, r2 = trace_2newref(arg)
> +-- These tests didn't fail before the patch.
> +-- They check the patch's correctness.
>  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')
> ===================================================================
>
> > > +
> > > +test:done(true)
> > > --
> > > 2.43.0
> > >
>
> --
> Best regards,
> Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Sergey, thanks for the patch! LGTM

On 12/11/23 18:35, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun and Peter Cawley.
>
> (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e)
>
> Assume we have the parent trace with the following IRs:
>
> | 0001 }  tab TNEW   #0    #0
> | 0002 }  p32 NEWREF 0001  "key"
> | 0003 }  fal HSTORE 0002  false
> | ....        SNAP   #1   [ ---- ---- 0001 ---- ]
> | 0004 >  num SLOAD  #1    T
> | ....        SNAP   #2   [ ---- ---- 0001 ]
> | 0005 >  num EQ     0004  0004
> | 0006 }  tru HSTORE 0002  true
> | ....        SNAP   #3   [ ---- ---- 0001 true ]
>
> The side trace for the third snapshot emits the following IRs:
>
> | 0001    tab TNEW   #0    #0
> | 0002    p32 NEWREF 0001  "key"
> | 0003    fal HSTORE 0002  false
> | 0004    p32 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.
>
> This patch prevents the second emitting of IR NEWREF by checking the last
> one emitted NEWREF IR. There is no need to check NEWREFs beyond since
> it guarantees the snapshot is taken after it, because it may cause table
> rehashing, so all prior results are invalidated.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> 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)
>   		if (irr->o == IR_HREFK || irr->o == IR_AREF) {
>   		  IRIns *irf = &T->ir[irr->op1];
>   		  tmp = emitir(irf->ot, tmp, irf->op2);
> +		} else if (irr->o == IR_NEWREF) {
> +		  IRRef allocref = tref_ref(tr);
> +		  IRRef keyref = tref_ref(key);
> +		  IRRef newref_ref = J->chain[IR_NEWREF];
> +		  IRIns *newref = &J->cur.ir[newref_ref];
> +		  lj_assertJ(irref_isk(keyref),
> +			     "sunk store for parent IR %04d with bad key %04d",
> +			     refp - REF_BIAS, keyref - REF_BIAS);
> +		  if (newref_ref > allocref && newref->op2 == keyref) {
> +		    lj_assertJ(newref->op1 == allocref,
> +			       "sunk store for parent IR %04d with bad tab %04d",
> +			       refp - REF_BIAS, allocref - REF_BIAS);
> +		    tmp = newref_ref;
> +		    goto skip_newref;
> +		  }
>   		}
>   	      }
>   	      tmp = emitir(irr->ot, tmp, key);
> +	    skip_newref:
>   	      val = snap_pref(J, T, map, nent, seen, irs->op2);
>   	      if (val == 0) {
>   		IRIns *irc = &T->ir[irs->op2];
> 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.
> +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
> +
> +-- 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')
> +
> +test:done(true)

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/3.0 and
release/2.11.

On 11.12.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Thanks to Sergey Kaplun and Peter Cawley.
> 
> (cherry-picked from commit 1761fd2ef79ffe1778011c7e9cb03ed361b48c5e)
> 
> Assume we have the parent trace with the following IRs:
> 
> | 0001 }  tab TNEW   #0    #0
> | 0002 }  p32 NEWREF 0001  "key"
> | 0003 }  fal HSTORE 0002  false
> | ....        SNAP   #1   [ ---- ---- 0001 ---- ]
> | 0004 >  num SLOAD  #1    T
> | ....        SNAP   #2   [ ---- ---- 0001 ]
> | 0005 >  num EQ     0004  0004
> | 0006 }  tru HSTORE 0002  true
> | ....        SNAP   #3   [ ---- ---- 0001 true ]
> 
> The side trace for the third snapshot emits the following IRs:
> 
> | 0001    tab TNEW   #0    #0
> | 0002    p32 NEWREF 0001  "key"
> | 0003    fal HSTORE 0002  false
> | 0004    p32 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.
> 
> This patch prevents the second emitting of IR NEWREF by checking the last
> one emitted NEWREF IR. There is no need to check NEWREFs beyond since
> it guarantees the snapshot is taken after it, because it may cause table
> rehashing, so all prior results are invalidated.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> 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
> 

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
IM