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