Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix ir_khash for non-string GCobj.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 6115 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits below.
 
> 
>>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 constant
>>function is used to calculate hash for the corresponding object.
>Typo: s/hash/the hash
>>This 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 by
>>routine. 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 the
>Typo: 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
> 

[-- Attachment #2: Type: text/html, Size: 8671 bytes --]