From: Mike Pall <mike>
Contributed by Peter Cawley.
(cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
When emitting `IR_HREF` for constant value to lookup the `ir_khash()`Typo: s/for constant/for a constantfunction is used to calculate hash for the corresponding object.Typo: s/hash/the hashThis calculation must be the same as in the corresponding `hashkey()`
function from <lj_tab.c>.
Hash calculating via passing two arguments `lo`, and `hi` to `hashrot()`Typo: s/calculating via/is calculated byroutine. For non-string GC objects the first `lo` argument is the same
for GC64 and not GC64 mode -- lower 32 bits of the object address. For
GC64 mode `hi` argument is upper 32 bits of the object address,
including specific type NaN-tag. This `hi` argument in `ir_khash()`
function is miscalculated in GC64 using non-GC64 value (`lo` +
`HASH_BIAS`). As a result, the hash for the GC object is miscalculated
on trace and we exit from trace due to assertion guard on the type or
value check.
This patch fixes calculation of hash value on trace for GC64 mode by
making it consistent with `hashkey()`.Typo: s/of hash/of the hash
Sergey Kaplun:
* added the description and the test for the problem
Part of tarantool/tarantool#7230
---
Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-356-ir-khash-non-string-obj-full-ci
Issue/PR:
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/LuaJIT/LuaJIT/pull/356
Tarantool PR: https://github.com/tarantool/tarantool/pull/8020
Side note: Problems with red fuzzer jobs look irrelevant to the patch.
src/lj_asm.c | 4 +
.../lj-356-ir-khash-non-string-obj.test.lua | 90 +++++++++++++++++++
2 files changed, 94 insertions(+)
create mode 100644 test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
diff --git a/src/lj_asm.c b/src/lj_asm.c
index 1a7fb0c8..a154547b 100644
--- a/src/lj_asm.c
+++ b/src/lj_asm.c
@@ -1016,7 +1016,11 @@ static uint32_t ir_khash(IRIns *ir)
} else {
lua_assert(irt_isgcv(ir->t));
lo = u32ptr(ir_kgc(ir));
+#if LJ_GC64
+ hi = (uint32_t)(u64ptr(ir_kgc(ir)) >> 32) | (irt_toitype(ir->t) << 15);
+#else
hi = lo + HASH_BIAS;
+#endif
}
return hashrot(lo, hi);
}
diff --git a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
new file mode 100644
index 00000000..fff0b1a5
--- /dev/null
+++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
@@ -0,0 +1,90 @@
+local tap = require('tap')
+local traceinfo = require('jit.util').traceinfo
+local table_new = require('table.new')
+
+-- Test file to demonstrate the incorrect GC64 JIT behaviour
+-- for `IR_HREF` for on-trace-constant key lookup.
+-- See also https://github.com/LuaJIT/LuaJIT/pull/356.
+local test = tap.test('lj-356-ir-khash-non-string-obj')
+local N_ITERATIONS = 4
+
+-- Amount of iteration for trace compilation and execution and
+-- additional check, that there is no new trace compiled.
+test:plan(N_ITERATIONS + 1)
+
+-- To reproduce the issue we need to compile a trace with
+-- `IR_HREF`, with a lookup of constant hash key GC value. To
+-- prevent `IR_HREFK` to be emitted instead, we need a table with
+-- a huge hash part. Delta of address between the start of the
+-- hash part of the table and the current node to lookup must be
+-- more than `(1024 * 64 - 1) * sizeof(Node)`.
+-- See <src/lj_record.c>, for details.
+-- XXX: This constant is well suited to prevent test to be flaky,
+-- because the aforementioned delta is always large enough.
+local N_HASH_FIELDS = 1024 * 1024 * 8
+local MAGIC = 42
+
+local filled_tab = table_new(0, N_HASH_FIELDS + 1)
+
+-- The function returns constant cdata pinned to `GCproto` to be
+-- used as a key for table lookup.+local function get_const_cdata()
+ return 0LL
+end
+
+-- XXX: don't set `hotexit` to prevent compilation of trace after
+-- exiting the main test cycle.
+jit.opt.start('hotloop=1')
+
+-- Prevent `get_const_cdata()` become hot and be compiled before
+-- the main test cycle.Typo: s/become hot and be/from becoming hot and being+jit.off()
+
+filled_tab[get_const_cdata()] = MAGIC
+
+-- Speed up table filling-up.
+jit.on()
+
+-- Filling-up the table with GC values to minimize the amount of
+-- hash collisions and increases delta between the start of theTypo: s/increases/increase+-- hash part of the table and currently stored node.
+for i = 1, N_HASH_FI%MCEPASTEBIN%ELDS do
+ filled_tab[1LL] = i
+end
+
+-- Prevent JIT misbehaviour before the main test chunk.
+jit.off()
+
+-- Allocate a table with exact array part to be sure that there
+-- is no side exit from the trace, due to table reallocation.
+local result_tab = table_new(N_ITERATIONS, 0)
+
+jit.flush()
+
+assert(not traceinfo(1), 'no traces compiled after flush')
+
+jit.on()
+
+for _ = 1, N_ITERATIONS do
+ -- If the hash for table lookup is miscalculated, then we get
+ -- `nil` (most possibly) value from the table and the side exit
+ -- will be taken and we continue execution from the call to
+ -- `get_const_cdata()`, this function is already hot after the
+ -- first cycle iteration, and the new trace is recorded.
+ table.insert(result_tab, filled_tab[get_const_cdata()])
+end
+
+jit.off()
+
+test:ok(not traceinfo(2), 'the second trace should not be compiled')
+
+-- No more need to prevent trace compilation.
+jit.on()
+
+for i = 1, N_ITERATIONS do
+ -- Check that that all lookups are correct and there is no
+ -- value from other cdata stored in the table.
+ test:ok(result_tab[i] == MAGIC, 'correct hash lookup from the table')
+end
+
+os.exit(test:check() and 0 or 1)
--
2.34.1--
Best regards,Maxim Kokryashkin