[Tarantool-patches] [PATCH luajit 2/2] Fix canonicalization of +-0.0 keys for IR_NEWREF.

[Maxim Kokryashkin]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits regarding the commit message.
 
> 
>>From: Mike Pall <mike>
>>
>>Reported by Sergey Kaplun.
>>
>>(cherry picked from commit 96fc114a7a3be3fd2c227d5a0ac53aa50cfb85d1)
>>
>>This commit is a follow-up for the commit
>>f067cf638cf8987ab3b6db372d609a5982e458b5 ("Fix narrowing of unary
>>minus."). Since this commit -0 IR constant is stored as well as +0
>Typo: s/as well as/as well as the/
>>constant on the trace. Since IR NEWREF keys don't canonicalizied for -0
>Typo: s/don’t canonicalized/don’t get canonicalized/
>>opposed of IR HREFK, it may lead to inconsistencies during trace
>Typo: s/opposed of/as opposed to/
>>recording.
>>
>>In particular, since -0 and 0 are different IR constants, alias analysis
>>declares that they can't be aliased during folding optimization.
>>Therefore:
>>1) For the IR TNEW we have non-nil value to lookup from the table via
>>   HLOAD, when only nil lookup is expected due to alias analysis.
>>2) For the IR TDUP we have non-nil value to lookup from the table via
>>   HLOAD, but the template table has no 0 field initiated as far as -0
>>   isn't folding to 0 during parsing (see `bcemit_unop()` in
>>   <src/lj_parse.c>).
>>These cases lead to the assertion failures in `fwd_ahload()`.
>Typo: s/the assertion/assertion/
>>
>>This patch adds the aforementioned canonicalization.
>>
>>Sergey Bronnikov:
>>* reported the original issue for the TDUP IR
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8516
>>---
>>
>>Side note: I don't mention the 981 issue by intend -- I don't want to
>>bother Mike with force pushes:). I suppose Igor should add this line (if
>>he wants) went this commit will be cherry-picked to our master branch
>>(a.k.a. tarantool).
>>
>> src/lj_record.c | 2 +
>> .../tarantool-tests/lj-981-folding-0.test.lua | 57 +++++++++++++++++++
>> 2 files changed, 59 insertions(+)
>> create mode 100644 test/tarantool-tests/lj-981-folding-0.test.lua
>>
>>diff --git a/src/lj_record.c b/src/lj_record.c
>>index 9e2e1d9e..cc44db8d 100644
>>--- a/src/lj_record.c
>>+++ b/src/lj_record.c
>>@@ -1474,6 +1474,8 @@ TRef lj_record_idx(jit_State *J, RecordIndex *ix)
>>  TRef key = ix->key;
>>  if (tref_isinteger(key)) /* NEWREF needs a TValue as a key. */
>>  key = emitir(IRTN(IR_CONV), key, IRCONV_NUM_INT);
>>+ else if (tref_isnumber(key) && tref_isk(key) && tvismzero(&ix->keyv))
>>+ key = lj_ir_knum_zero(J); /* Canonicalize -0.0 to +0.0. */
>>  xref = emitir(IRT(IR_NEWREF, IRT_PGC), ix->tab, key);
>>  keybarrier = 0; /* NEWREF already takes care of the key barrier. */
>> #ifdef LUAJIT_ENABLE_TABLE_BUMP
>>diff --git a/test/tarantool-tests/lj-981-folding-0.test.lua b/test/tarantool-tests/lj-981-folding-0.test.lua
>>new file mode 100644
>>index 00000000..251da24d
>>--- /dev/null
>>+++ b/test/tarantool-tests/lj-981-folding-0.test.lua
>>@@ -0,0 +1,57 @@
>>+local tap = require('tap')
>>+local test = tap.test('lj-981-folding-0'):skipcond({
>>+ ['Test requires JIT enabled'] = not jit.status(),
>>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>>+})
>>+
>>+-- Test file to demonstrate LuaJIT misbehaviour on load forwarding
>>+-- for -0 IR constant as table index.
>>+-- See also,  https://github.com/LuaJIT/LuaJIT/issues/981 .
>>+
>>+local jparse = require('utils.jit_parse')
>>+
>>+jit.opt.start('hotloop=1')
>>+
>>+test:plan(4)
>>+
>>+-- Reset traces.
>>+jit.flush()
>>+
>>+jparse.start('i')
>>+local result
>>+local expected = 'result'
>>+-- TNEW:
>>+-- -0 isn't folded during parsing, so it will be set with KSHORT,
>>+-- UNM bytecodes. See <src/lj_parse.c> and bytecode listing
>>+-- for details.
>>+-- Because of it, empty table is created via TNEW.
>>+for _ = 1, 4 do
>>+ result = ({[-0] = expected})[0]
>>+end
>>+
>>+local traces = jparse.finish()
>>+
>>+-- Test that there is no any assertion failure.
>>+test:ok(result == expected, 'TNEW and -0 folding')
>>+-- Test that there is no NEWREF -0 IR.
>>+test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TNEW tab')
>>+
>>+-- Reset traces.
>>+jit.flush()
>>+
>>+jparse.start('i')
>>+-- TDUP:
>>+-- Now just add a constant field for the table to use TDUP with
>>+-- template table instead TNEW before -0 is set.
>>+for _ = 1, 4 do
>>+ result = ({[-0] = expected, [1] = 1})[0]
>>+end
>>+
>>+traces = jparse.finish()
>>+
>>+-- Test that there is no any assertion failure.
>>+test:ok(result == expected, 'TDUP and -0 folding')
>>+-- Test that there is no NEWREF -0 IR.
>>+test:ok(not traces[1]:has_ir('NEWREF.*-0'), '-0 is canonized for TDUP tab')
>>+
>>+os.exit(test:check() and 0 or 1)
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230515/3ac59700/attachment.htm>