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

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

[Sergey Kaplun via Tarantool-patches]

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()`
function is used to calculate hash for the corresponding object.
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()`
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()`.

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.
+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
+-- hash part of the table and currently stored node.
+for i = 1, N_HASH_FIELDS 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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- 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 --]

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

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!

Some addition to Max’s comments. And a question on the test.

Sergos

> On 8 Dec 2022, at 08:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> 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()`
               an                           ^^^ 
                         perhaps just ‘for a constant value lokup’?

> function is used to calculate hash for the corresponding object.
> 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()`
                                                             the

> 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()`
           a

> function is miscalculated in GC64 using non-GC64 value (`lo` +
                                 mode    a

> `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
                          the          an
> value check.
> 
> This patch fixes calculation of hash value on trace for GC64 mode by
> making it consistent with `hashkey()`.
                          the
> 
> 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.
      of an           an
> +-- 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
             an `IR_HREFK` emission

> +-- 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.
> +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
> +-- hash part of the table and currently stored node.
> +for i = 1, N_HASH_FIELDS 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')

That’s not quite clear to me: a second trace generation is a side-effect
of the incorrect hash calculation. Is it always leads to the trace
generation? 

> +
> +-- 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')

And this one checks what then? The hash is calculated correctly, but the value
read from the `filled_tab` is incorrect - what can lead to this?

> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

Fixed your comments, branch is force pushed.

On 12.12.22, Maxim Kokryashkin wrote:
> 
> 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

Fixed. New commit message is:

| LJ_GC64: Fix ir_khash for non-string GCobj.
|
| Contributed by Peter Cawley.
|
| (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
|
| When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`
| function is used to calculate the hash for the corresponding object.
| This calculation must be the same as in the corresponding `hashkey()`
| function from <lj_tab.c>.
|
| Hash is calculated by passing two arguments `lo`, and `hi` to the
| `hashrot()` 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 a specific type NaN-tag. This `hi` argument in
| `ir_khash()` function is miscalculated in GC64 mode using a non-GC64
| value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is
| miscalculated on trace and we exit from the trace due to an assertion
| guard on the type or value check.
|
| This patch fixes calculation of the hash value on trace for GC64 mode by
| making it consistent with the `hashkey()`.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#7230


> >>
> >>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.

<snipped>

> >>+
> >>+-- 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

<snipped>

> >>--

Iterative patch with the fixes:
===================================================================
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
index fff0b1a5..7f304183 100644
--- 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
@@ -3,7 +3,7 @@ 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.
+-- of an `IR_HREF` for the 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
@@ -14,10 +14,10 @@ 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)`.
+-- prevent an `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.
@@ -36,8 +36,8 @@ end
 -- exiting the main test cycle.
 jit.opt.start('hotloop=1')
 
--- Prevent `get_const_cdata()` become hot and be compiled before
--- the main test cycle.
+-- Prevent `get_const_cdata()` from becoming hot and being
+-- compiled before the main test cycle.
 jit.off()
 
 filled_tab[get_const_cdata()] = MAGIC
@@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC
 jit.on()
 
 -- Filling-up the table with GC values to minimize the amount of
--- hash collisions and increases delta between the start of the
+-- hash collisions and increase delta between the start of the
 -- hash part of the table and currently stored node.
-for i = 1, N_HASH_FIELDS do
-  filled_tab[1LL] = i
+for _ = 1, N_HASH_FIELDS do
+  filled_tab[1LL] = 1
 end
 
 -- Prevent JIT misbehaviour before the main test chunk.
===================================================================

> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 14.12.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Some addition to Max’s comments. And a question on the test.
> 
> Sergos
> 
> > On 8 Dec 2022, at 08:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > 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()`
>                an                           ^^^ 
>                          perhaps just ‘for a constant value lokup’?
> 
> > function is used to calculate hash for the corresponding object.
> > 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()`
>                                                              the
> 
> > 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()`
>            a
> 
> > function is miscalculated in GC64 using non-GC64 value (`lo` +
>                                  mode    a
> 
> > `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
>                           the          an
> > value check.
> > 
> > This patch fixes calculation of hash value on trace for GC64 mode by
> > making it consistent with `hashkey()`.
>                           the
> > 

Fixed your comments.
The new commit message is the following:

| LJ_GC64: Fix ir_khash for non-string GCobj.
|
| Contributed by Peter Cawley.
|
| (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
|
| When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`
| function is used to calculate the hash for the corresponding object.
| This calculation must be the same as in the corresponding `hashkey()`
| function from <lj_tab.c>.
|
| Hash is calculated by passing two arguments `lo`, and `hi` to the
| `hashrot()` 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 a specific type NaN-tag. This `hi` argument in
| `ir_khash()` function is miscalculated in GC64 mode using a non-GC64
| value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is
| miscalculated on trace and we exit from the trace due to an assertion
| guard on the type or value check.
|
| This patch fixes calculation of the hash value on trace for GC64 mode by
| making it consistent with the `hashkey()`.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#7230


> > 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.

<snipped>

> > 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.
>       of an           an
> > +-- 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
>              an `IR_HREFK` emission

Side note: I'm not sure about "emission" corectness here, so ignoring
this part.

I've fixed the rest of your comments, see the iterative patch below.

===================================================================
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
index fff0b1a5..7f304183 100644
--- 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
@@ -3,7 +3,7 @@ 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.
+-- of an `IR_HREF` for the 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
@@ -14,10 +14,10 @@ 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)`.
+-- prevent an `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.
@@ -36,8 +36,8 @@ end
 -- exiting the main test cycle.
 jit.opt.start('hotloop=1')
 
--- Prevent `get_const_cdata()` become hot and be compiled before
--- the main test cycle.
+-- Prevent `get_const_cdata()` from becoming hot and being
+-- compiled before the main test cycle.
 jit.off()
 
 filled_tab[get_const_cdata()] = MAGIC
@@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC
 jit.on()
 
 -- Filling-up the table with GC values to minimize the amount of
--- hash collisions and increases delta between the start of the
+-- hash collisions and increase delta between the start of the
 -- hash part of the table and currently stored node.
-for i = 1, N_HASH_FIELDS do
-  filled_tab[1LL] = i
+for _ = 1, N_HASH_FIELDS do
+  filled_tab[1LL] = 1
 end
 
 -- Prevent JIT misbehaviour before the main test chunk.
===================================================================

> 
> > +-- 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

<snipped>

> > +
> > +test:ok(not traceinfo(2), 'the second trace should not be compiled')
> 
> That’s not quite clear to me: a second trace generation is a side-effect
> of the incorrect hash calculation. Is it always leads to the trace
> generation? 

How I see this for now. There are two possibilities, when the
aforementioned hash is miscalculated:

1) We got `nil` value on a trace to lookup and we exit from the trace by
assertion guard on the field type (the most possible one, AFAIKS).
2) We got a value for some existing cdata after hash lookup, so we don't
exit from a trace, but got an incorrect value by the given key. NB: I've
updated the generation of the table content to avoid clashing with
`MAGIC` value on the 42nd iteration :).

So this test should cover both cases.

> 
> > +
> > +-- 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')
> 
> And this one checks what then? The hash is calculated correctly, but the value
> read from the `filled_tab` is incorrect - what can lead to this?
> 
> > +end
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 15 декабря 2022, 13:16 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Sergos!
>
>Thanks for the review!
>
>On 14.12.22, sergos wrote:
>> Hi!
>>
>> Thanks for the patch!
>>
>> Some addition to Max’s comments. And a question on the test.
>>
>> Sergos
>>
>> > On 8 Dec 2022, at 08:46, Sergey Kaplun < skaplun@tarantool.org > wrote:
>> >
>> > 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()`
>> an ^^^
>> perhaps just ‘for a constant value lokup’?
>>
>> > function is used to calculate hash for the corresponding object.
>> > 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()`
>> the
>>
>> > 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()`
>> a
>>
>> > function is miscalculated in GC64 using non-GC64 value (`lo` +
>> mode a
>>
>> > `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
>> the an
>> > value check.
>> >
>> > This patch fixes calculation of hash value on trace for GC64 mode by
>> > making it consistent with `hashkey()`.
>> the
>> >
>
>Fixed your comments.
>The new commit message is the following:
>
>| LJ_GC64: Fix ir_khash for non-string GCobj.
>|
>| Contributed by Peter Cawley.
>|
>| (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
>|
>| When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`
>| function is used to calculate the hash for the corresponding object.
>| This calculation must be the same as in the corresponding `hashkey()`
>| function from <lj_tab.c>.
>|
>| Hash is calculated by passing two arguments `lo`, and `hi` to the
>| `hashrot()` 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 a specific type NaN-tag. This `hi` argument in
>| `ir_khash()` function is miscalculated in GC64 mode using a non-GC64
>| value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is
>| miscalculated on trace and we exit from the trace due to an assertion
>| guard on the type or value check.
>|
>| This patch fixes calculation of the hash value on trace for GC64 mode by
>| making it consistent with the `hashkey()`.
>|
>| Sergey Kaplun:
>| * added the description and the test for the problem
>|
>| Part of tarantool/tarantool#7230
>
>
>> > 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.
>
><snipped>
>
>> > 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.
>> of an an
>> > +-- 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
>> an `IR_HREFK` emission
>
>Side note: I'm not sure about "emission" corectness here, so ignoring
>this part.
>
>I've fixed the rest of your comments, see the iterative patch below.
>
>===================================================================
>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
>index fff0b1a5..7f304183 100644
>--- 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
>@@ -3,7 +3,7 @@ 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.
>+-- of an `IR_HREF` for the 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
>@@ -14,10 +14,10 @@ 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)`.
>+-- prevent an `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.
>@@ -36,8 +36,8 @@ end
> -- exiting the main test cycle.
> jit.opt.start('hotloop=1')
> 
>--- Prevent `get_const_cdata()` become hot and be compiled before
>--- the main test cycle.
>+-- Prevent `get_const_cdata()` from becoming hot and being
>+-- compiled before the main test cycle.
> jit.off()
> 
> filled_tab[get_const_cdata()] = MAGIC
>@@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC
> jit.on()
> 
> -- Filling-up the table with GC values to minimize the amount of
>--- hash collisions and increases delta between the start of the
>+-- hash collisions and increase delta between the start of the
> -- hash part of the table and currently stored node.
>-for i = 1, N_HASH_FIELDS do
>- filled_tab[1LL] = i
>+for _ = 1, N_HASH_FIELDS do
>+ filled_tab[1LL] = 1
> end
> 
> -- Prevent JIT misbehaviour before the main test chunk.
>===================================================================
>
>>
>> > +-- 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
>
><snipped>
>
>> > +
>> > +test:ok(not traceinfo(2), 'the second trace should not be compiled')
>>
>> That’s not quite clear to me: a second trace generation is a side-effect
>> of the incorrect hash calculation. Is it always leads to the trace
>> generation?
>
>How I see this for now. There are two possibilities, when the
>aforementioned hash is miscalculated:
>
>1) We got `nil` value on a trace to lookup and we exit from the trace by
>assertion guard on the field type (the most possible one, AFAIKS).
>2) We got a value for some existing cdata after hash lookup, so we don't
>exit from a trace, but got an incorrect value by the given key. NB: I've
>updated the generation of the table content to avoid clashing with
>`MAGIC` value on the 42nd iteration :).
>
>So this test should cover both cases.
>
>>
>> > +
>> > +-- 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')
>>
>> And this one checks what then? The hash is calculated correctly, but the value
>> read from the `filled_tab` is incorrect - what can lead to this?
>>
>> > +end
>> > +
>> > +os.exit(test:check() and 0 or 1)
>> > --
>> > 2.34.1
>> >
>>
>
>--
>Best regards,
>Sergey Kaplun
 

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

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

[sergos via Tarantool-patches]

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

Hi!

Thanks for replies! 

LGTM.
Sergos

> On 15 Dec 2022, at 13:13, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Sergos!
> 
> Thanks for the review!
> 
> On 14.12.22, sergos wrote:
>> Hi!
>> 
>> Thanks for the patch!
>> 
>> Some addition to Max’s comments. And a question on the test.
>> 
>> Sergos
>> 
>>> On 8 Dec 2022, at 08:46, Sergey Kaplun <skaplun@tarantool.org> wrote:
>>> 
>>> 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()`
>>               an                           ^^^ 
>>                         perhaps just ‘for a constant value lokup’?
>> 
>>> function is used to calculate hash for the corresponding object.
>>> 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()`
>>                                                             the
>> 
>>> 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()`
>>           a
>> 
>>> function is miscalculated in GC64 using non-GC64 value (`lo` +
>>                                 mode    a
>> 
>>> `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
>>                          the          an
>>> value check.
>>> 
>>> This patch fixes calculation of hash value on trace for GC64 mode by
>>> making it consistent with `hashkey()`.
>>                          the
>>> 
> 
> Fixed your comments.
> The new commit message is the following:
> 
> | LJ_GC64: Fix ir_khash for non-string GCobj.
> |
> | Contributed by Peter Cawley.
> |
> | (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)
> |
> | When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`
> | function is used to calculate the hash for the corresponding object.
> | This calculation must be the same as in the corresponding `hashkey()`
> | function from <lj_tab.c>.
> |
> | Hash is calculated by passing two arguments `lo`, and `hi` to the
> | `hashrot()` 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 a specific type NaN-tag. This `hi` argument in
> | `ir_khash()` function is miscalculated in GC64 mode using a non-GC64
> | value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is
> | miscalculated on trace and we exit from the trace due to an assertion
> | guard on the type or value check.
> |
> | This patch fixes calculation of the hash value on trace for GC64 mode by
> | making it consistent with the `hashkey()`.
> |
> | Sergey Kaplun:
> | * added the description and the test for the problem
> |
> | Part of tarantool/tarantool#7230
> 
> 
>>> 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.
> 
> <snipped>
> 
>>> 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.
>>      of an           an
>>> +-- 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
>>             an `IR_HREFK` emission
> 
> Side note: I'm not sure about "emission" corectness here, so ignoring
> this part.
> 
> I've fixed the rest of your comments, see the iterative patch below.
> 
> ===================================================================
> 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
> index fff0b1a5..7f304183 100644
> --- 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
> @@ -3,7 +3,7 @@ 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.
> +-- of an `IR_HREF` for the 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
> @@ -14,10 +14,10 @@ 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)`.
> +-- prevent an `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.
> @@ -36,8 +36,8 @@ end
> -- exiting the main test cycle.
> jit.opt.start('hotloop=1')
> 
> --- Prevent `get_const_cdata()` become hot and be compiled before
> --- the main test cycle.
> +-- Prevent `get_const_cdata()` from becoming hot and being
> +-- compiled before the main test cycle.
> jit.off()
> 
> filled_tab[get_const_cdata()] = MAGIC
> @@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC
> jit.on()
> 
> -- Filling-up the table with GC values to minimize the amount of
> --- hash collisions and increases delta between the start of the
> +-- hash collisions and increase delta between the start of the
> -- hash part of the table and currently stored node.
> -for i = 1, N_HASH_FIELDS do
> -  filled_tab[1LL] = i
> +for _ = 1, N_HASH_FIELDS do
> +  filled_tab[1LL] = 1
> end
> 
> -- Prevent JIT misbehaviour before the main test chunk.
> ===================================================================
> 
>> 
>>> +-- 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
> 
> <snipped>
> 
>>> +
>>> +test:ok(not traceinfo(2), 'the second trace should not be compiled')
>> 
>> That’s not quite clear to me: a second trace generation is a side-effect
>> of the incorrect hash calculation. Is it always leads to the trace
>> generation? 
> 
> How I see this for now. There are two possibilities, when the
> aforementioned hash is miscalculated:
> 
> 1) We got `nil` value on a trace to lookup and we exit from the trace by
> assertion guard on the field type (the most possible one, AFAIKS).
> 2) We got a value for some existing cdata after hash lookup, so we don't
> exit from a trace, but got an incorrect value by the given key. NB: I've
> updated the generation of the table content to avoid clashing with
> `MAGIC` value on the 42nd iteration :).
> 
> So this test should cover both cases.
> 
>> 
>>> +
>>> +-- 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')
>> 
>> And this one checks what then? The hash is calculated correctly, but the value
>> read from the `filled_tab` is incorrect - what can lead to this?
>> 
>>> +end
>>> +
>>> +os.exit(test:check() and 0 or 1)
>>> -- 
>>> 2.34.1
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun


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

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 08.12.22, Sergey Kaplun via Tarantool-patches wrote:
> 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()`
> function is used to calculate hash for the corresponding object.
> 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()`
> 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()`.
> 
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM