Hi, Sergey! Thanks for the patch! LGTM -- Best regards, Maxim Kokryashkin     >Четверг, 15 декабря 2022, 13:16 +03:00 от Sergey Kaplun : >  >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 >> > >> > 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