

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