<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hello, Max</p>
<p>thanks for the patch! LGTM</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 7/17/23 18:11, Maxim Kokryashkin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1689606718.357696928@f315.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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
<a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>:<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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true" class="moz-txt-link-freetext">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>
</blockquote>
</body>
</html>