From: Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: tarantool-patches@dev.tarantool.org, sergeyb@tarantool.org, skaplun@tarantool.org, m.kokryashkin@tarantool.org Subject: [Tarantool-patches] [PATCH luajit v2] Mark CONV as non-weak, to prevent elimination of its side-effect. Date: Tue, 3 Oct 2023 16:37:03 +0300 [thread overview] Message-ID: <20231003133705.5700-1-max.kokryashkin@gmail.com> (raw) 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 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#9145 --- Branch: https://github.com/tarantool/luajit/tree/fckxorg/mark-conv-non-weak PR: https://github.com/tarantool/tarantool/pull/8871 Changes in v2: - Fixed comments as per review by Sergey Kaplun src/lj_ir.h | 2 +- .../mark-conv-non-weak.test.lua | 115 ++++++++++++++++++ 2 files changed, 116 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 46af54e4..e3ba6ff3 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..f54f30ba --- /dev/null +++ b/test/tarantool-tests/mark-conv-non-weak.test.lua @@ -0,0 +1,115 @@ +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=2', 'hotexit=2') + +-- 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 trace is recorded, the IR +-- is the following before the patch: +---- TRACE 1 IR +-- .... SNAP #0 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] +-- 0001 u8 XLOAD [0x100dac521] V +-- 0002 int BAND 0001 +12 +-- 0003 > int EQ 0002 +0 +-- 0004 > int SLOAD #8 T +-- .... SNAP #1 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] +-- 0005 > num SLOAD #3 T +-- 0006 num CONV 0004 num.int +-- 0007 + num ADD 0006 0005 +-- 0008 > fun SLOAD #4 T +-- 0009 > tab SLOAD #5 T +-- 0010 > int SLOAD #6 T +-- 0011 > fun EQ 0008 ipairs_aux +-- 0012 + int ADD 0010 +1 +-- 0013 int FLOAD 0009 tab.asize +-- 0014 > int ABC 0013 0012 +-- 0015 p64 FLOAD 0009 tab.array +-- 0016 p64 AREF 0015 0012 +-- 0017 >+ num ALOAD 0016 +-- .... SNAP #2 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] +-- 0018 ------ LOOP ------------ +-- 0019 u8 XLOAD [0x100dac521] V +-- 0020 int BAND 0019 +12 +-- 0021 > int EQ 0020 +0 +-- 0022 > int CONV 0017 int.num +-- .... SNAP #3 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] +-- 0023 + num ADD 0017 0007 +-- 0024 + int ADD 0012 +1 +-- 0025 > int ABC 0013 0024 +-- 0026 p64 AREF 0015 0024 +-- 0027 >+ num ALOAD 0026 +-- 0028 num PHI 0017 0027 +-- 0029 num PHI 0007 0023 +-- 0030 int PHI 0012 0024 +---- TRACE 1 stop -> loop + +---- TRACE 1 exit 0 +---- TRACE 1 exit 3 +-- +-- And the following after the patch: +---- TRACE 1 IR +-- .... SNAP #0 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] +-- 0001 u8 XLOAD [0x102438521] V +-- 0002 int BAND 0001 +12 +-- 0003 > int EQ 0002 +0 +-- 0004 > int SLOAD #8 T +-- .... SNAP #1 [ ---- ---- ---- ---- ---- ---- ---- ---- ---- ] +-- 0005 > num SLOAD #3 T +-- 0006 num CONV 0004 num.int +-- 0007 + num ADD 0006 0005 +-- 0008 > fun SLOAD #4 T +-- 0009 > tab SLOAD #5 T +-- 0010 > int SLOAD #6 T +-- 0011 > fun EQ 0008 ipairs_aux +-- 0012 + int ADD 0010 +1 +-- 0013 int FLOAD 0009 tab.asize +-- 0014 > int ABC 0013 0012 +-- 0015 p64 FLOAD 0009 tab.array +-- 0016 p64 AREF 0015 0012 +-- 0017 >+ num ALOAD 0016 +-- .... SNAP #2 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] +-- 0018 ------ LOOP ------------ +-- 0019 u8 XLOAD [0x102438521] V +-- 0020 int BAND 0019 +12 +-- 0021 > int EQ 0020 +0 +-- 0022 > int CONV 0017 int.num +-- .... SNAP #3 [ ---- ---- ---- 0007 ---- ---- 0012 0012 0017 ] +-- 0023 + num ADD 0017 0007 +-- 0024 + int ADD 0012 +1 +-- 0025 > int ABC 0013 0024 +-- 0026 p64 AREF 0015 0024 +-- 0027 >+ num ALOAD 0026 +-- 0028 num PHI 0017 0027 +-- 0029 num PHI 0007 0023 +-- 0030 int PHI 0012 0024 +---- TRACE 1 stop -> loop + +---- TRACE 1 exit 0 +---- TRACE 1 exit 2 +-- +-- Before the patch, the `0022 > int CONV 0017 int.num` +-- instruction is omitted due to DCE, which results in the +-- third side exit being taken, instead of the second, +-- and, hence, incorrect summation. After the patch, `CONV` +-- is left intact and is not omitted; it remains as a guarded +-- instruction, so the second side exit is taken and sum is +-- performed correctly. + +for _, val in ipairs(data) do + if val == val then + sum = sum + val + end +end + +test:ok(sum == sum, 'NaN check was not omitted') +test:done(true) -- 2.39.3 (Apple Git-145)
next reply other threads:[~2023-10-03 13:37 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-10-03 13:37 Maksim Kokryashkin via Tarantool-patches [this message] 2023-10-03 16:24 ` Sergey Bronnikov via Tarantool-patches 2023-10-06 10:49 ` Maxim Kokryashkin via Tarantool-patches 2023-10-09 9:27 ` Sergey Kaplun via Tarantool-patches 2023-10-13 10:11 ` Maxim Kokryashkin via Tarantool-patches 2023-10-23 14:59 ` Sergey Kaplun via Tarantool-patches 2023-10-03 16:25 ` Sergey Bronnikov 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=20231003133705.5700-1-max.kokryashkin@gmail.com \ --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 luajit v2] 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