

<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 15 декабря 2022, 13:16 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16710994131007383716_BODY">Hi, Sergos!<br><br>Thanks for the review!<br><br>On 14.12.22, sergos wrote:<br>> Hi!<br>><br>> Thanks for the patch!<br>><br>> Some addition to Max’s comments. And a question on the test.<br>><br>> Sergos<br>><br>> > On 8 Dec 2022, at 08:46, Sergey Kaplun <<a href="/compose?To=skaplun@tarantool.org">skaplun@tarantool.org</a>> wrote:<br>> ><br>> > From: Mike Pall <mike><br>> ><br>> > Contributed by Peter Cawley.<br>> ><br>> > (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)<br>> ><br>> > When emitting `IR_HREF` for constant value to lookup the `ir_khash()`<br>> an ^^^<br>> perhaps just ‘for a constant value lokup’?<br>><br>> > function is used to calculate hash for the corresponding object.<br>> > This calculation must be the same as in the corresponding `hashkey()`<br>> > function from <lj_tab.c>.<br>> ><br>> > Hash calculating via passing two arguments `lo`, and `hi` to `hashrot()`<br>> the<br>><br>> > routine. For non-string GC objects the first `lo` argument is the same<br>> > for GC64 and not GC64 mode -- lower 32 bits of the object address. For<br>> > GC64 mode `hi` argument is upper 32 bits of the object address,<br>> > including specific type NaN-tag. This `hi` argument in `ir_khash()`<br>> a<br>><br>> > function is miscalculated in GC64 using non-GC64 value (`lo` +<br>> mode a<br>><br>> > `HASH_BIAS`). As a result, the hash for the GC object is miscalculated<br>> > on trace and we exit from trace due to assertion guard on the type or<br>> the an<br>> > value check.<br>> ><br>> > This patch fixes calculation of hash value on trace for GC64 mode by<br>> > making it consistent with `hashkey()`.<br>> the<br>> ><br><br>Fixed your comments.<br>The new commit message is the following:<br><br>| LJ_GC64: Fix ir_khash for non-string GCobj.<br>|<br>| Contributed by Peter Cawley.<br>|<br>| (cherry picked from commit b4ed3219a1a98dd9fe7d1e3eeea3b82f5a780948)<br>|<br>| When emitting the `IR_HREF` for a constant value lookup the `ir_khash()`<br>| function is used to calculate the hash for the corresponding object.<br>| This calculation must be the same as in the corresponding `hashkey()`<br>| function from <lj_tab.c>.<br>|<br>| Hash is calculated by passing two arguments `lo`, and `hi` to the<br>| `hashrot()` routine. For non-string GC objects the first `lo` argument<br>| is the same for GC64 and not GC64 mode -- lower 32 bits of the object<br>| address. For GC64 mode `hi` argument is upper 32 bits of the object<br>| address, including a specific type NaN-tag. This `hi` argument in<br>| `ir_khash()` function is miscalculated in GC64 mode using a non-GC64<br>| value (`lo` + `HASH_BIAS`). As a result, the hash for the GC object is<br>| miscalculated on trace and we exit from the trace due to an assertion<br>| guard on the type or value check.<br>|<br>| This patch fixes calculation of the hash value on trace for GC64 mode by<br>| making it consistent with the `hashkey()`.<br>|<br>| Sergey Kaplun:<br>| * added the description and the test for the problem<br>|<br>| Part of tarantool/tarantool#7230<br><br><br>> > Sergey Kaplun:<br>> > * added the description and the test for the problem<br>> ><br>> > Part of tarantool/tarantool#7230<br>> > ---<br>> ><br>> > Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-356-ir-khash-non-string-obj-full-ci" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-356-ir-khash-non-string-obj-full-ci</a><br>> > Issue/PR:<br>> > * <a href="https://github.com/tarantool/tarantool/issues/7230" target="_blank">https://github.com/tarantool/tarantool/issues/7230</a><br>> > * <a href="https://github.com/LuaJIT/LuaJIT/pull/356" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/356</a><br>> > Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/8020" target="_blank">https://github.com/tarantool/tarantool/pull/8020</a><br>> ><br>> > Side note: Problems with red fuzzer jobs look irrelevant to the patch.<br><br><snipped><br><br>> > 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<br>> > new file mode 100644<br>> > index 00000000..fff0b1a5<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua<br>> > @@ -0,0 +1,90 @@<br>> > +local tap = require('tap')<br>> > +local traceinfo = require('jit.util').traceinfo<br>> > +local table_new = require('table.new')<br>> > +<br>> > +-- Test file to demonstrate the incorrect GC64 JIT behaviour<br>> > +-- for `IR_HREF` for on-trace-constant key lookup.<br>> of an an<br>> > +-- See also <a href="https://github.com/LuaJIT/LuaJIT/pull/356" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/356</a>.<br>> > +local test = tap.test('lj-356-ir-khash-non-string-obj')<br>> > +local N_ITERATIONS = 4<br>> > +<br>> > +-- Amount of iteration for trace compilation and execution and<br>> > +-- additional check, that there is no new trace compiled.<br>> > +test:plan(N_ITERATIONS + 1)<br>> > +<br>> > +-- To reproduce the issue we need to compile a trace with<br>> > +-- `IR_HREF`, with a lookup of constant hash key GC value. To<br>> > +-- prevent `IR_HREFK` to be emitted instead, we need a table with<br>> an `IR_HREFK` emission<br><br>Side note: I'm not sure about "emission" corectness here, so ignoring<br>this part.<br><br>I've fixed the rest of your comments, see the iterative patch below.<br><br>===================================================================<br>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<br>index fff0b1a5..7f304183 100644<br>--- a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua<br>+++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua<br>@@ -3,7 +3,7 @@ local traceinfo = require('jit.util').traceinfo<br> local table_new = require('table.new')<br> <br> -- Test file to demonstrate the incorrect GC64 JIT behaviour<br>--- for `IR_HREF` for on-trace-constant key lookup.<br>+-- of an `IR_HREF` for the on-trace-constant key lookup.<br> -- See also <a href="https://github.com/LuaJIT/LuaJIT/pull/356" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/356</a>.<br> local test = tap.test('lj-356-ir-khash-non-string-obj')<br> local N_ITERATIONS = 4<br>@@ -14,10 +14,10 @@ test:plan(N_ITERATIONS + 1)<br> <br> -- To reproduce the issue we need to compile a trace with<br> -- `IR_HREF`, with a lookup of constant hash key GC value. To<br>--- prevent `IR_HREFK` to be emitted instead, we need a table with<br>--- a huge hash part. Delta of address between the start of the<br>--- hash part of the table and the current node to lookup must be<br>--- more than `(1024 * 64 - 1) * sizeof(Node)`.<br>+-- prevent an `IR_HREFK` to be emitted instead, we need a table<br>+-- with a huge hash part. Delta of address between the start of<br>+-- the hash part of the table and the current node to lookup must<br>+-- be more than `(1024 * 64 - 1) * sizeof(Node)`.<br> -- See <src/lj_record.c>, for details.<br> -- XXX: This constant is well suited to prevent test to be flaky,<br> -- because the aforementioned delta is always large enough.<br>@@ -36,8 +36,8 @@ end<br> -- exiting the main test cycle.<br> jit.opt.start('hotloop=1')<br> <br>--- Prevent `get_const_cdata()` become hot and be compiled before<br>--- the main test cycle.<br>+-- Prevent `get_const_cdata()` from becoming hot and being<br>+-- compiled before the main test cycle.<br> jit.off()<br> <br> filled_tab[get_const_cdata()] = MAGIC<br>@@ -46,10 +46,10 @@ filled_tab[get_const_cdata()] = MAGIC<br> jit.on()<br> <br> -- Filling-up the table with GC values to minimize the amount of<br>--- hash collisions and increases delta between the start of the<br>+-- hash collisions and increase delta between the start of the<br> -- hash part of the table and currently stored node.<br>-for i = 1, N_HASH_FIELDS do<br>- filled_tab[1LL] = i<br>+for _ = 1, N_HASH_FIELDS do<br>+ filled_tab[1LL] = 1<br> end<br> <br> -- Prevent JIT misbehaviour before the main test chunk.<br>===================================================================<br><br>><br>> > +-- a huge hash part. Delta of address between the start of the<br>> > +-- hash part of the table and the current node to lookup must be<br>> > +-- more than `(1024 * 64 - 1) * sizeof(Node)`.<br>> > +-- See <src/lj_record.c>, for details.<br>> > +-- XXX: This constant is well suited to prevent test to be flaky,<br>> > +-- because the aforementioned delta is always large enough.<br>> > +local N_HASH_FIELDS = 1024 * 1024 * 8<br>> > +local MAGIC = 42<br><br><snipped><br><br>> > +<br>> > +test:ok(not traceinfo(2), 'the second trace should not be compiled')<br>><br>> That’s not quite clear to me: a second trace generation is a side-effect<br>> of the incorrect hash calculation. Is it always leads to the trace<br>> generation?<br><br>How I see this for now. There are two possibilities, when the<br>aforementioned hash is miscalculated:<br><br>1) We got `nil` value on a trace to lookup and we exit from the trace by<br>assertion guard on the field type (the most possible one, AFAIKS).<br>2) We got a value for some existing cdata after hash lookup, so we don't<br>exit from a trace, but got an incorrect value by the given key. NB: I've<br>updated the generation of the table content to avoid clashing with<br>`MAGIC` value on the 42nd iteration :).<br><br>So this test should cover both cases.<br><br>><br>> > +<br>> > +-- No more need to prevent trace compilation.<br>> > +jit.on()<br>> > +<br>> > +for i = 1, N_ITERATIONS do<br>> > + -- Check that that all lookups are correct and there is no<br>> > + -- value from other cdata stored in the table.<br>> > + test:ok(result_tab[i] == MAGIC, 'correct hash lookup from the table')<br>><br>> And this one checks what then? The hash is calculated correctly, but the value<br>> read from the `filled_tab` is incorrect - what can lead to this?<br>><br>> > +end<br>> > +<br>> > +os.exit(test:check() and 0 or 1)<br>> > --<br>> > 2.34.1<br>> ><br>><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>