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

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

[Maksim Kokryashkin via Tarantool-patches]

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)
-- 
2.39.2 (Apple Git-143)

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

[Sergey Bronnikov via Tarantool-patches]

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)

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

[Maxim Kokryashkin via Tarantool-patches]

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


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: 7473 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please consider my comment below.

On 12.07.23, 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
> ---

<snipped>

> +
> +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.

There are two important statements:
As you mentioned, the case is observed only in the DUALNUM mode.
As mentioned in the commit, this is due to `CONV int.num` IRs.

The comment for the test itself is misleading the reason of misbehaviour
isn't the wrong bytecode in the recording -- it's the consequence.

Let's use the reproducer, but with hotloop=2 to decrease amount of
traces:

| LUA_PATH="src/?.lua;;" src/luajit -jdump=X -e "
| local data = {0.1, 0, 0.1, 0, 0 / 0}
| local sum = 0
|
| jit.opt.start('hotloop=2', 'hotexit=1')
|
| for _, val in ipairs(data) do
|     if val == val then
|         sum = sum + val
|     end
| end
|
| math.fmod(1,1)
|
| print(sum)
| "

This is output before the patch (see `<-- !!!` marks):

| ---- TRACE 1 exit 0
|  000056449701ff26 0000000041bb9880 00000000409cf228 0000000041bb9e94
|  00007fffa700cfa0 00000000409c2378 00000000409cf228 00000000409c3320
|  0000000000000000 0000000000000000 00007fd4e2d38938 00000000fffffffe
|  00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0
|  +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310
|  +4.6863058152444e-310 +4.6863058152873e-310 +4.6863058152729e-310 +4.6863058152444e-310
|  +4.6863058152213e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| ---- TRACE 1 exit 3 <-- !!!
|  0000000041bb9848 0000000000000006 0000000041bb9828 0000000000000005
|  00007fffa700cfa0 0000000000000006 00000000409cf228 00000000409c3320
|  0000000000000000 0000000000000029 00007fd4e2d3ab28 00000000fffffffe
|  00007fffa700d3e8 000056446cea132a 00000000409c3098 0000000041bb9ee0
|  +4.6863058149307e-310 +4.6863058151824e-310 +4.6863058151959e-310 +4.6863058152213e-310
|                nan              +0.2               nan               nan <-- !!!
|  +4.6863058152213e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| nan

And this is after:

| ---- TRACE 1 exit 0
|  0000556168ccff06 0000000041f8a880 0000000040275220 0000000041f8ae94
|  00007ffda4a82900 0000000040268378 0000000040275220 0000000040269320
|  0000000000000000 0000000000000000 00007fdd97c11938 00000000fffffffe
|  00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0
|  +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310
|  +4.6380982093151e-310 +4.6380982093497e-310 +4.6380982093353e-310 +4.6380982093068e-310
|  +4.6380982092837e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| ---- TRACE 1 exit 2 <-- !!!
|  0000000041f8a848 0000000000000006 0000000041f8a828 0000000041f8ae94
|  00007ffda4a82900 0000000000000005 0000000040275220 0000000080000000
|  0000000000000000 0000000000000029 00007fdd97c13b28 00000000fffffffe
|  00007ffda4a82d48 000055613eb5132a 0000000040269090 0000000041f8aee0
|  +4.6380982089931e-310 +4.6380982092477e-310 +4.6380982092617e-310 +4.6380982092839e-310
|                nan              +0.2               nan       -2147483648 <-- !!!
|  +4.6380982092837e-310                +0 -0.66666666666667  +0.7999999995324
|   -1.1429096284595                +0                +0                +0
| 0.2

As you can see different side exits are taken, and **this** leads to
different bytecodes on the recording of the side trace (this is observed
in the dump of the bytecode). So, the second side exit doesn't contain
NaN.

The IRs of the first trace are the following:

| ---- TRACE 1 start (command line):7
| ---- TRACE 1 IR
| ....              SNAP   #0   [ ---- ---- ---- ---- ---- ---- ---- ---- ]
| 0001          u8  XLOAD  [0x41b864c1]  V
| 0002          int BAND   0001  +12
| 0003       >  int EQ     0002  +0
| 0004       >  int SLOAD  #7    T
| ....              SNAP   #1   [ ---- ---- ---- ---- ---- ---- ---- ---- ]
| 0005       >  num SLOAD  #2    T
| 0006 xmm7     num CONV   0004  num.int
| 0007 xmm7   + num ADD    0006  0005
| 0008       >  fun SLOAD  #3    T
| 0009 rdx   >  tab SLOAD  #4    T
| 0010 rbp   >  int SLOAD  #5    T
| 0011       >  fun EQ     0008  ipairs_aux
| 0012 rbp    + int ADD    0010  +1
| 0013 rcx      int FLOAD  0009  tab.asize
| 0014       >  int ABC    0013  0012
| 0015 rax      p32 FLOAD  0009  tab.array
| 0016          p32 AREF   0015  0012
| 0017 xmm6  >+ num ALOAD  0016
| ....              SNAP   #2   [ ---- ---- 0007 ---- ---- 0012 0012 0017 ]
| 0018 ------------ LOOP ------------
| 0019          u8  XLOAD  [0x41b864c1]  V
| 0020          int BAND   0019  +12
| 0021       >  int EQ     0020  +0
| 0022 rdi   >  int CONV   0017  int.num
| ....              SNAP   #3   [ ---- ---- 0007 ---- ---- 0012 0012 0017 ]
| 0023 xmm7   + num ADD    0017  0007
| 0024 rbp    + int ADD    0012  +1
| 0025       >  int ABC    0013  0024
| 0026          p32 AREF   0015  0024
| 0027 xmm6  >+ num ALOAD  0026
| 0028 xmm6     num PHI    0017  0027
| 0029 xmm7     num PHI    0007  0023
| 0030 rbp      int PHI    0012  0024
| 0031 rbx      nil RENAME 0012  #3
| 0032 xmm4     nil RENAME 0017  #2
| 0033 xmm5     nil RENAME 0007  #2
| ---- TRACE 1 stop -> loop

The skipped due to DCE conversion is the following:
| 0022 rdi   >  int CONV   0017 int.num

Also, from gdb we can check, that omitted conversion has `IRCONV_CHECK`
mode (I suppose that only check conversion can't be omitted, but Mike
prefer more coarse fix).

Also, it should be mentioned that we get this conversion due to type
instability in loop carried dependency (see `loop_unrool()` in
<src/lj_opt_loop.c>):
| else if (irt_isnum(irr->t) && irt_isinteger(t))  /* Fix num->int. */
|   ref = tref_ref(emitir(IRTGI(IR_CONV), ref,
|                  IRCONV_INT_NUM|IRCONV_CHECK));

All this confusion is because of the too complex traces in the given
example. We have 4 traces, but I suppose that should be reduced to only
one with the necessary side exit to restore NaN value after missing
check.

I suggest to look forward for all places, where such int.num check
conversion is used:

| src/lj_record.c:491:      tr[i] = emitir(IRTGI(IR_CONV), tr[i], IRCONV_INT_NUM|IRCONV_CHECK);
| src/lj_opt_loop.c:349:                              IRCONV_INT_NUM|IRCONV_CHECK));
| src/lj_ffrecord.c:529:  tr = emitir(IRTGI(IR_CONV), tr, IRCONV_INT_NUM|IRCONV_CHECK);
| src/lj_opt_narrow.c:604:      rc = emitir(IRTGI(IR_CONV), rc, IRCONV_INT_NUM|IRCONV_CHECK);

to create a testcase with one trace.

> +--
> +-- 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)
> -- 
> 2.39.2 (Apple Git-143)
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

[-- 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 --]

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

[Igor Munkin via Tarantool-patches]

Max,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 12.07.23, Maksim Kokryashkin via Tarantool-patches 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
> 

<snipped>

> -- 
> 2.39.2 (Apple Git-143)
> 

-- 
Best regards,
IM