[Tarantool-patches] [PATCH luajit] Fix TDUP load forwarding after table rehash.

[Sergey Kaplun]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry-picked from commit c7db8255e1eb59f933fac7bc9322f0e4f8ddc6e6)

After table rehashing number keys loaded via ALOAD may be placed in the
hash part of the table. So, load forwarding analysis missed the
corresponding stores like they are never existed. In such case, either
we faced an assertion failure in `fwd_ahload()` due to values types
mismatch, either we faced an assertion failure in `rec_check_slots()`
since forwarded value by the JIT compiler isn't the same as it is in the
interpreter.

This patch adds a check that there is no any `IR_NEWREF` after table
creation, so it can't be rehashed.

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

Part of tarantool/tarantool#8825
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-980-load-fwd-after-table-rehash
PR: https://github.com/tarantool/tarantool/pull/8998
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/980
* https://github.com/tarantool/tarantool/issues/8825

 src/lj_opt_mem.c                              |   6 +
 ...j-980-load-fwd-after-table-rehash.test.lua | 166 ++++++++++++++++++
 2 files changed, 172 insertions(+)
 create mode 100644 test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua

diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
index 59fddbdd..7b610506 100644
--- a/src/lj_opt_mem.c
+++ b/src/lj_opt_mem.c
@@ -157,6 +157,7 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
     if (ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) {
       /* A NEWREF with a number key may end up pointing to the array part.
       ** But it's referenced from HSTORE and not found in the ASTORE chain.
+      ** Or a NEWREF may rehash the table and move unrelated number keys.
       ** For now simply consider this a conflict without forwarding anything.
       */
       if (xr->o == IR_AREF) {
@@ -167,6 +168,11 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
 	    goto cselim;
 	  ref2 = newref->prev;
 	}
+      } else {
+	IRIns *key = IR(xr->op2);
+	if (key->o == IR_KSLOT) key = IR(key->op1);
+	if (irt_isnum(key->t) && J->chain[IR_NEWREF] > tab)
+	  goto cselim;
       }
       /* NEWREF inhibits CSE for HREF, and dependent FLOADs from HREFK/AREF.
       ** But the above search for conflicting stores was limited by xref.
diff --git a/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua
new file mode 100644
index 00000000..a27932df
--- /dev/null
+++ b/test/tarantool-tests/lj-980-load-fwd-after-table-rehash.test.lua
@@ -0,0 +1,166 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour during load
+-- forwarding optimization for HLOAD after table rehashing.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/980.
+
+local test = tap.test('lj-980-load-fwd-after-table-rehash'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(6)
+
+jit.opt.start('hotloop=1')
+
+local result
+-- The test for TNEW load forwarding. It doesn't trigger an assert
+-- since the commit "Fix TNEW load forwarding with instable
+-- types.". But still add it to avoid regressions in future.
+for i = 6, 9 do
+  -- Need big enough table to see rehashing.
+  -- Also, to simplify logic with AREF, HREF don't use default
+  -- 1, 4 (start, stop) values here.
+  local t = {i, i, i, i, i, i, i}
+  -- Insert via ASTORE.
+  t[i] = i
+  t[1] = nil
+  t[2] = nil
+  t[3] = nil
+  t[4] = nil
+  t[5] = nil
+  -- Rehash table. Array part is empty.
+  t['1000'] = 1000
+  -- Load via HLOAD.
+  result = t[i]
+end
+
+test:is(result, 9, 'TNEW load forwarding')
+
+-- TNEW load forwarding, aliased table.
+local alias_store = {{}, {}, {}, {}, {}}
+for i = 6, 9 do
+  local t = {i, i, i, i, i, i, i}
+  alias_store[#alias_store + 1] = t
+  local alias = alias_store[i]
+  -- Insert via ASTORE.
+  alias[i] = i
+  alias[1] = nil
+  alias[2] = nil
+  alias[3] = nil
+  alias[4] = nil
+  alias[5] = nil
+  -- Rehash table. Array part is empty.
+  alias['1000'] = 1000
+  -- Load via HLOAD.
+  result = t[i]
+end
+
+test:is(result, 9, 'TNEW load forwarding, aliased table')
+
+local expected = 'result'
+
+-- TDUP different types.
+for i = 6, 9 do
+  local t = {1, 2, 3, 4, 5, 6, 7, 8}
+  t[i] = expected
+  t[i + 1] = expected
+  t[1] = nil
+  t[2] = nil
+  t[3] = nil
+  t[4] = nil
+  t[5] = nil
+  t[6] = nil
+  -- Rehash table. Array part is empty.
+  t['1000'] = 1000
+  -- Result on the recording (i == 8) iteration is 'result'.
+  -- Nevertheless, on the last (i == 9) iteration it is 8.
+  -- Just check that there is no assert failure here.
+  -- Load via HLOAD.
+  result = t[8]
+end
+
+-- Checked for assertion guard, on the last iteration we get
+-- the value on initializatoin.
+test:is(result, 8, 'TDUP load forwarding different types')
+
+-- TDUP different types, aliased table.
+alias_store = {{}, {}, {}, {}, {}}
+for i = 6, 9 do
+  local t = {1, 2, 3, 4, 5, 6, 7, 8}
+  -- Store table, to be aliased later.
+  alias_store[#alias_store + 1] = t
+  local alias = alias_store[i]
+  alias[i] = expected
+  alias[i + 1] = expected
+  alias[1] = nil
+  alias[2] = nil
+  alias[3] = nil
+  alias[4] = nil
+  alias[5] = nil
+  alias[6] = nil
+  -- Rehash table. Array part is empty.
+  alias['1000'] = 1000
+  -- Result on the recording (i == 8) iteration is 'result'.
+  -- Nevertheless, on the last (i == 9) iteration it is 8.
+  -- Just check that there is no assert failure here.
+  -- Load via HLOAD.
+  result = t[8]
+end
+
+-- Checked for assertion guard, on the last iteration we get
+-- the value on initializatoin.
+test:is(result, 8, 'TDUP load forwarding different types, aliased table')
+
+-- TDUP same type, different values.
+for i = 6, 9 do
+  local t = {1, 2, 3, 4, 5, 6, '7', '8'}
+  t[i] = expected
+  t[i + 1] = expected
+  t[1] = nil
+  t[2] = nil
+  t[3] = nil
+  t[4] = nil
+  t[5] = nil
+  t[6] = nil
+  -- Rehash table. Array part is empty.
+  t['1000'] = 1000
+  -- Result on the recording (i == 8) iteration is 'result'.
+  -- Nevertheless, on the last (i == 9) iteration it is '8'.
+  -- Just check that there is no assert failure here.
+  -- Load via HLOAD.
+  result = t[8]
+end
+
+-- Checked for assertion guard, on the last iteration we get
+-- the value on initializatoin.
+test:is(result, '8', 'TDUP load forwarding same type, different values')
+
+alias_store = {{}, {}, {}, {}, {}}
+for i = 6, 9 do
+  local t = {1, 2, 3, 4, 5, 6, '7', '8'}
+  -- Store table, to be aliased later.
+  alias_store[#alias_store + 1] = t
+  local alias = alias_store[i]
+  alias[i] = expected
+  alias[i + 1] = expected
+  alias[1] = nil
+  alias[2] = nil
+  alias[3] = nil
+  alias[4] = nil
+  alias[5] = nil
+  alias[6] = nil
+  -- Rehash table. Array part is empty.
+  alias['1000'] = 1000
+  -- Result on the recording (i == 8) iteration is 'result'.
+  -- Nevertheless, on the last (i == 9) iteration it is '8'.
+  -- Just check that there is no assert failure here.
+  -- Load via HLOAD.
+  result = t[8]
+end
+
+-- Checked for assertion guard, on the last iteration we get
+-- the value on initializatoin.
+test:is(result, '8',
+        'TDUP load forwarding same type, different values, aliased table')
+
+test:done(true)
-- 
2.41.0