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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hello, Max

thanks for the patch! LGTM


On 7/17/23 18:11, Maxim Kokryashkin wrote:
> 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: 11658 bytes --]