Re: [Tarantool-patches] [PATCH] Mark CONV as non-weak, to prevent elimination of its side-effect.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5323 bytes --]


Hi!
It’s silly how it is escaped from my attention, that this patch
is essentially relevant only for `DUALNUM` mode. This issue
reproduces on arm64, or if you have LuaJIT built with
`-DLUJAIT_NUMMODE=2` option. I’ve altered the commit message
a bit to mention the mode name explicitly. Also, I’ve added an
additional comment to the test case itself.
Here is the diff:
===============================================
diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
index aad39058..9a325202 100644
--- a/test/tarantool-tests/mark-conv-non-weak.test.lua
+++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
@@ -10,6 +10,10 @@ local sum = 0
 
 jit.opt.start('hotloop=1', 'hotexit=1')
 
+-- XXX: The test fails before the patch only
+-- for `DUALNUM` mode. All of the IRs below are
+-- produced by the corresponding LuaJIT build.
+
 -- When the last trace is recorded, the traced bytecode
 -- is the following before the patch:
 -- ---- TRACE 4 start 2/3 test.lua:6
===============================================
 
And here is the altered commit message:
===============================================
Mark CONV as non-weak, to prevent elimination of its side-effect.
 
An unused guarded CONV int.num cannot be omitted in general.
 
(cherry-picked from commit 881d02d3117838acaf4fb844332c8e33cc95c8c5)
 
In some cases, an unused `CONV int.num` omission in `DUALNUM` mode
may lead to a guard absence, resulting in invalid control flow
branching and undefined behavior. For a comprehensive example of
the described situation, please refer to the comment
in `test/tarantool-tests/mark-conv-non-weak.test.lua`.
 
Maxim Kokryashkin:
* added the description and the test for the problem
 
Part of tarantool/tarantool#8825
===============================================
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 13 июля 2023, 15:23 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Hi, Max!
>
>
>thanks for the patch!
>
>Links to the branch and PR are missed, but I found them:
>
>branch:  https://github.com/tarantool/luajit/tree/fckxorg/mark-conv-non-weak
>
>PR:  https://github.com/tarantool/tarantool/pull/8871
>
>
>Test is passed after reverting the patch with fix.
>
>
>Sergey
>
>
>On 7/12/23 12:52, Maksim Kokryashkin wrote:
>> From: Mike Pall <mike>
>>
>> An unused guarded CONV int.num cannot be omitted in general.
>>
>> (cherry-picked from commit 881d02d3117838acaf4fb844332c8e33cc95c8c5)
>>
>> In some cases, an unused `CONV` omission may lead to a guard
>> absence, resulting in invalid control flow branching and
>> undefined behavior. For a comprehensive example of
>> the described situation, please refer to the comment
>> in `test/tarantool-tests/mark-conv-non-weak.test.lua`.
>>
>> Maxim Kokryashkin:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#8825
>> ---
>> src/lj_ir.h | 2 +-
>> .../mark-conv-non-weak.test.lua | 58 +++++++++++++++++++
>> 2 files changed, 59 insertions(+), 1 deletion(-)
>> create mode 100644 test/tarantool-tests/mark-conv-non-weak.test.lua
>>
>> diff --git a/src/lj_ir.h b/src/lj_ir.h
>> index e8bca275..bf9b9292 100644
>> --- a/src/lj_ir.h
>> +++ b/src/lj_ir.h
>> @@ -132,7 +132,7 @@
>> _(XBAR, S , ___, ___) \
>> \
>> /* Type conversions. */ \
>> - _(CONV, NW, ref, lit) \
>> + _(CONV, N , ref, lit) \
>> _(TOBIT, N , ref, ref) \
>> _(TOSTR, N , ref, lit) \
>> _(STRTO, N , ref, ___) \
>> diff --git a/test/tarantool-tests/mark-conv-non-weak.test.lua b/test/tarantool-tests/mark-conv-non-weak.test.lua
>> new file mode 100644
>> index 00000000..aad39058
>> --- /dev/null
>> +++ b/test/tarantool-tests/mark-conv-non-weak.test.lua
>> @@ -0,0 +1,58 @@
>> +local tap = require('tap')
>> +local test = tap.test('mark-conv-non-weak'):skipcond({
>> + ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(1)
>> +
>> +local data = {0.1, 0, 0.1, 0, 0 / 0}
>> +local sum = 0
>> +
>> +jit.opt.start('hotloop=1', 'hotexit=1')
>> +
>> +-- When the last trace is recorded, the traced bytecode
>> +-- is the following before the patch:
>> +-- ---- TRACE 4 start 2/3 test.lua:6
>> +-- 0018 ADDVV 1 1 6
>> +-- 0019 ITERC 5 3 3
>> +-- 0000 . FUNCC ; ipairs_aux
>> +-- 0020 JITERL 5 1
>> +-- 0021 GGET 2 7 ; "assert"
>> +-- 0022 ISEQV 1 1
>> +-- 0023 JMP 4 => 0026
>> +-- 0024 KPRI 4 1
>> +-- 0025 JMP 5 => 0027
>> +-- 0027 CALL 2 1 2
>> +-- 0000 . FUNCC ; assert
>> +--
>> +-- And the following after the patch:
>> +-- ---- TRACE 4 start 2/2 test.lua:5
>> +-- 0016 ISNEV 6 6
>> +-- 0017 JMP 7 => 0019
>> +-- 0019 ITERC 5 3 3
>> +-- 0000 . FUNCC ; ipairs_aux
>> +-- 0020 JITERL 5 1
>> +-- 0021 GGET 2 7 ; "assert"
>> +-- 0022 ISEQV 1 1
>> +-- 0023 JMP 4 => 0026
>> +-- 0026 KPRI 4 2
>> +-- 0027 CALL 2 1 2
>> +-- 0000 . FUNCC ; assert
>> +-- 0028 RET0 0 1
>> +--
>> +-- The crucial difference here is the abscent
>> +-- `ISNEV` in the first case, which produces the
>> +-- desired guarded `CONV`, when translated to IR.
>> +--
>> +-- Since there is no guard, NaN is added to the sum,
>> +-- despite the test case logic.
>> +
>> +for _, val in ipairs(data) do
>> + if val == val then
>> + sum = sum + val
>> + end
>> +end
>> +
>> +test:ok(sum == sum, 'NaN check was not omitted')
>> +
>> +os.exit(test:check() and 0 or 1)
 

[-- Attachment #2: Type: text/html, Size: 7473 bytes --]