From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 91CBC6E674A; Mon, 20 Nov 2023 11:24:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 91CBC6E674A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1700468655; bh=+4lH3LUiVRam5xEwAyCUsiKvxND0Iy1HcQ+kJerLZZY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=je2VW/Py5JqragZb/HYxA62hWFIRCwPuBq+p3+82R1yCtXZDAB5b+XMD8VHzadCjn XaABojR5J6eWMvE7R9Df/f5wSJmyL8RvUNY6EmZlN5a2BZ3b1p3AcU2cX+XqIih2nr TslVpFwOhog+sADGmaO+8dx+mnzXordURvWcidYo= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A91DF55BD6E for ; Mon, 20 Nov 2023 11:24:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A91DF55BD6E Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1r4zZj-000djf-21; Mon, 20 Nov 2023 11:24:12 +0300 Content-Type: multipart/alternative; boundary="------------2ve0W2UFB33g564c1DGV55Il" Message-ID: <6cd70422-9c55-4d78-a110-5d031e51f13b@tarantool.org> Date: Mon, 20 Nov 2023 11:24:11 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Maxim Kokryashkin References: <20230712095212.20480-1-max.kokryashkin@gmail.com> <1689606718.357696928@f315.i.mail.ru> Content-Language: en-US In-Reply-To: <1689606718.357696928@f315.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD93F1575C7510F55475E315F415624ABB7C25A6AFC357EBC4400894C459B0CD1B9939533FC6659898C15E91BBC8225978CD03D653FD54843E58F67A5DED2D21E38 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7624C4D757C4F5837EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063706922F90966A37BA8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88A75A764D25E111767C16B32C3781AAC117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCE2CCD8F0CAA010FB389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8BC0ADEB1C81BB362F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C86A7C529F68B8E5C03F1AB874ED890284AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C33439DF9FD084CBEFBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF4964A708C60C975A1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C337FDC682149C2B9D35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5ED1676E03B5E6FD69872DD9819DA2EE89BB1E11B2305983AF87CCE6106E1FC07E67D4AC08A07B9B0A816C540FC8EEC30CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFAE9B272BC8D098588FE661ABA0427061A4437FBD5F603F5EFB6555896A33AD154C7498080C0F87FCFCC135EDF961F15FAE75CBCEEAB51696F799F0806BF471A9A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmRIjd71J0y0m9Glpz28OVQ== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458A391A834490C8D15C6554E57AE2399CB40F6B59427225522282EC151BADDC1D3523A6D01B4765B2DFB59E2DDD9FE06B14FA522850F29BC30B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] Mark CONV as non-weak, to prevent elimination of its side-effect. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: Maksim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------2ve0W2UFB33g564c1DGV55Il Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > : > 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 > > > > 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) > --------------2ve0W2UFB33g564c1DGV55Il Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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)
 
--------------2ve0W2UFB33g564c1DGV55Il--