From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maksim Kokryashkin <max.kokryashkin@gmail.com>,
tarantool-patches@dev.tarantool.org, skaplun@tarantool.org,
m.kokryashkin@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] Mark CONV as non-weak, to prevent elimination of its side-effect.
Date: Thu, 13 Jul 2023 15:23:54 +0300 [thread overview]
Message-ID: <be6a7241-ab8a-d85e-8db7-67a11b5669a1@tarantool.org> (raw)
In-Reply-To: <20230712095212.20480-1-max.kokryashkin@gmail.com>
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)
next prev parent reply other threads:[~2023-07-13 12:23 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-12 9:52 Maksim Kokryashkin via Tarantool-patches
2023-07-13 12:23 ` Sergey Bronnikov via Tarantool-patches [this message]
2023-07-17 15:11 ` Maxim Kokryashkin via Tarantool-patches
2023-11-20 8:24 ` Sergey Bronnikov via Tarantool-patches
2023-08-24 7:26 ` Sergey Kaplun via Tarantool-patches
2023-11-23 6:31 ` Igor Munkin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=be6a7241-ab8a-d85e-8db7-67a11b5669a1@tarantool.org \
--to=tarantool-patches@dev.tarantool.org \
--cc=m.kokryashkin@tarantool.org \
--cc=max.kokryashkin@gmail.com \
--cc=sergeyb@tarantool.org \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH] Mark CONV as non-weak, to prevent elimination of its side-effect.' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox