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

[Sergey Kaplun]

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