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