Hi! Thanks for replies! LGTM. Sergos > On 15 Dec 2022, at 13:13, Sergey Kaplun 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 wrote: >>> >>> From: Mike Pall >>> >>> 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 . >>> >>> 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 . > | > | 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. > > > >>> 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 , 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 , 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 > > > >>> + >>> +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