[Tarantool-patches] [PATCH luajit 0/2] Fix use-def analysis for varargs

[Tarantool-patches] [PATCH luajit 0/2] Fix use-def analysis for varargs

[Sergey Kaplun via Tarantool-patches]

The first patch in the patchset fixes the problem related to the flaky
test from tarantool/tarantool#8718. And a really similar issue is fixed
via the second commit.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-704-bc-varg-use-def
PR: https://github.com/tarantool/tarantool/pull/8754
Related Issues:
* https://github.com/tarantool/tarantool/issues/8516
* https://github.com/tarantool/tarantool/issues/8718
* https://github.com/LuaJIT/LuaJIT/issues/704

Mike Pall (2):
  Fix use-def analysis for BC_VARG.
  Fix use-def analysis for vararg functions.

 src/lj_snap.c                                 | 10 ++-
 .../lj-704-bc-varg-use-def.test.lua           | 90 +++++++++++++++++++
 2 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Ryan Lucia.

(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)

Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
no slots may considered as used. This leads to addititional SLOAD on
trace with incorrect value used later. This patch disables the use-def
analisis for BC_VARG as NIY.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8516
Relates to tarantool/tarantool#8718
---
 src/lj_snap.c                                 |  4 +-
 .../lj-704-bc-varg-use-def.test.lua           | 65 +++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua

diff --git a/src/lj_snap.c b/src/lj_snap.c
index a8b49fcb..5bbe8498 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -267,7 +267,7 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
        if (!(op == BC_ISTC || op == BC_ISFC)) DEF_SLOT(bc_a(ins));
        break;
     case BCMbase:
-      if (op >= BC_CALLM && op <= BC_VARG) {
+      if (op >= BC_CALLM && op <= BC_ITERN) {
 	BCReg top = (op == BC_CALLM || op == BC_CALLMT || bc_c(ins) == 0) ?
 		    maxslot : (bc_a(ins) + bc_c(ins)+LJ_FR2);
 	if (LJ_FR2) DEF_SLOT(bc_a(ins)+1);
@@ -278,6 +278,8 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
 	  for (s = 0; s < bc_a(ins); s++) DEF_SLOT(s);
 	  return 0;
 	}
+      } else if (op == BC_VARG) {
+	return maxslot;  /* NYI: punt. */
       } else if (op == BC_KNIL) {
 	for (s = bc_a(ins); s <= bc_d(ins); s++) DEF_SLOT(s);
       } else if (op == BC_TSETM) {
diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
new file mode 100644
index 00000000..c3ba65dd
--- /dev/null
+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
@@ -0,0 +1,65 @@
+local tap = require('tap')
+-- Test file to demonstrate LuaJIT misbehaviour in use-def
+-- snapshot analysis for BC_VARG.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/704.
+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- XXX: we don't really need to store this builtins, but this is
+-- reduces `jitdump()` output for reader significantly.
+local fmod = math.fmod
+local pcall = pcall
+
+-- Use the 2 values for `fmod()` to produce non-zero value for
+-- the call on trace (the last one call).
+local ARG_ON_RECORDING = 6
+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
+
+-- The `jitdump()` output was like the following before the patch:
+-- 0003 >  num SLOAD  #1    T
+-- ....        SNAP   #1   [`wrap()`|---- pcall|`varg()`|----]
+-- 0004 }  tab TNEW   #3    #0
+-- 0005 >  num SLOAD  #4    T
+-- 0006    p32 FLOAD  0004  tab.array
+-- 0007    p32 AREF   0006  +1
+-- 0008 }  num ASTORE 0007  0005
+-- ....        SNAP   #2   [`wrap()`|---- pcall|math.fmod|+6 0005]
+--
+-- The first snapshot misses the 0003 IR in the last slot to be
+-- used in the `fmod()` later, so it leads to the additional
+-- 0005 SLOAD #4, and storing it in the second snapshot.
+--
+-- The correct snapshot content after the patch is the following:
+-- ....        SNAP   #1   [`wrap()`|---- pcall|`varg()`|0003]
+-- ....
+-- ....        SNAP   #2   [`wrap()`|---- pcall|math.fmod|+6 0003]
+local function varg(...)
+  -- Generate snapshot after `pcall()` with missing slot.
+  -- The snapshot is generated before each TNEW after the commit
+  -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
+  -- OOM errors from helper functions.")
+  local slot = ({...})[1]
+  -- Forcify stitch and usage of vararg slot.
+  return fmod(ARG_ON_RECORDING, slot)
+end
+
+jit.opt.start('hotloop=1')
+
+local _, result
+local function wrap(arg)
+  -- `pcall()` is needed to emit snapshot to handle on-trace
+  -- errors.
+  _, result = pcall(varg, arg)
+end
+-- Record trace with the 0 result.
+wrap(ARG_ON_RECORDING)
+wrap(ARG_ON_RECORDING)
+-- Record trace with the non-zero result.
+wrap(ON_TRACE_VALUE)
+
+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

[Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Shmuel Zeigerman.

(cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57)

Use-def analysis for BC_FUNCV may consider slots greater than amount of
non-vararg parameters as dead slots due to early return for BC_RET
emitted before usage of BC_VARG. This patch restricts the maxslot to be
analyzed in such case with amount of parameters for the prototype of the
current function being recorded.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8516
Relates to tarantool/tarantool#8718
---
 src/lj_snap.c                                 |  6 +++--
 .../lj-704-bc-varg-use-def.test.lua           | 27 ++++++++++++++++++-
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 5bbe8498..a063c316 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -301,8 +301,10 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
 void lj_snap_purge(jit_State *J)
 {
   uint8_t udf[SNAP_USEDEF_SLOTS];
-  BCReg maxslot = J->maxslot;
-  BCReg s = snap_usedef(J, udf, J->pc, maxslot);
+  BCReg s, maxslot = J->maxslot;
+  if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams)
+    maxslot = J->pt->numparams;
+  s = snap_usedef(J, udf, J->pc, maxslot);
   for (; s < maxslot; s++)
     if (udf[s] != 0)
       J->base[s] = 0;  /* Purge dead slots. */
diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
index c3ba65dd..38606686 100644
--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
@@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
-test:plan(1)
+test:plan(2)
 
 -- XXX: we don't really need to store this builtins, but this is
 -- reduces `jitdump()` output for reader significantly.
@@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE)
 
 test:ok(result ~= 0, 'use-def analysis for BC_VARG')
 
+-- Now check the same case, but with BC_RET before the BC_VARG,
+-- so use-def analysis will take early return case.
+-- See `snap_usedef()` in <src/lj_snap.c> for details.
+-- The test checks that slots greater than `numparams` are not
+-- purged.
+local function varg_ret_bc(...)
+  -- XXX: This branch contains BC_RET. See the comment above.
+  -- luacheck: ignore
+  if false then else end
+  local slot = ({...})[1]
+  return fmod(ARG_ON_RECORDING, slot)
+end
+
+local function wrap_ret_bc(arg)
+  _, result = pcall(varg_ret_bc, arg)
+end
+
+-- Record trace with the 0 result.
+wrap_ret_bc(ARG_ON_RECORDING)
+wrap_ret_bc(ARG_ON_RECORDING)
+-- Record trace with the non-zero result.
+wrap_ret_bc(ON_TRACE_VALUE)
+
+test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')
+
 os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
 
> 
>>From: Mike Pall <mike>
>>
>>Reported by Ryan Lucia.
>>
>>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
>>
>>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
>Typo: s/analizis/analysis/
>>no slots may considered as used. This leads to addititional SLOAD on
>Typo: s/may/may be/
>Typo: s/to additional/to an additional/
>>trace with incorrect value used later. This patch disables the use-def
>Typo: s/trace with/the trace with an/
>>analisis for BC_VARG as NIY.
>Typo: s/analisis/analysis/
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8516
>>Relates to tarantool/tarantool#8718
>>---
>> src/lj_snap.c | 4 +-
>> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++
>> 2 files changed, 68 insertions(+), 1 deletion(-)
>> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>
>>diff --git a/src/lj_snap.c b/src/lj_snap.c
>>index a8b49fcb..5bbe8498 100644
>>--- a/src/lj_snap.c
>>+++ b/src/lj_snap.c
>>@@ -267,7 +267,7 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
>>        if (!(op == BC_ISTC || op == BC_ISFC)) DEF_SLOT(bc_a(ins));
>>        break;
>>     case BCMbase:
>>- if (op >= BC_CALLM && op <= BC_VARG) {
>>+ if (op >= BC_CALLM && op <= BC_ITERN) {
>>  BCReg top = (op == BC_CALLM || op == BC_CALLMT || bc_c(ins) == 0) ?
>>  maxslot : (bc_a(ins) + bc_c(ins)+LJ_FR2);
>>  if (LJ_FR2) DEF_SLOT(bc_a(ins)+1);
>>@@ -278,6 +278,8 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
>>  for (s = 0; s < bc_a(ins); s++) DEF_SLOT(s);
>>  return 0;
>>  }
>>+ } else if (op == BC_VARG) {
>>+ return maxslot; /* NYI: punt. */
>>       } else if (op == BC_KNIL) {
>>  for (s = bc_a(ins); s <= bc_d(ins); s++) DEF_SLOT(s);
>>       } else if (op == BC_TSETM) {
>>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>new file mode 100644
>>index 00000000..c3ba65dd
>>--- /dev/null
>>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>@@ -0,0 +1,65 @@
>>+local tap = require('tap')
>>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
>>+-- snapshot analysis for BC_VARG.
>>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
>>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>+ ['Test requires JIT enabled'] = not jit.status(),
>>+})
>>+
>>+test:plan(1)
>>+
>>+-- XXX: we don't really need to store this builtins, but this is
>Typo: s/this/these/
>>+-- reduces `jitdump()` output for reader significantly.
>>+local fmod = math.fmod
>>+local pcall = pcall
>>+
>>+-- Use the 2 values for `fmod()` to produce non-zero value for
>>+-- the call on trace (the last one call).
>>+local ARG_ON_RECORDING = 6
>>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
>Why are they exactly 6 and 7? Please drop a comment.
>>+
>>+-- The `jitdump()` output was like the following before the patch:
>>+-- 0003 > num SLOAD #1 T
>>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
>>+-- 0004 } tab TNEW #3 #0
>>+-- 0005 > num SLOAD #4 T
>>+-- 0006 p32 FLOAD 0004 tab.array
>>+-- 0007 p32 AREF 0006 +1
>>+-- 0008 } num ASTORE 0007 0005
>>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
>>+--
>>+-- The first snapshot misses the 0003 IR in the last slot to be
>>+-- used in the `fmod()` later, so it leads to the additional
>>+-- 0005 SLOAD #4, and storing it in the second snapshot.
>>+--
>>+-- The correct snapshot content after the patch is the following:
>>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
>>+-- ....
>>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
>>+local function varg(...)
>>+ -- Generate snapshot after `pcall()` with missing slot.
>>+ -- The snapshot is generated before each TNEW after the commit
>>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
>>+ -- OOM errors from helper functions.")
>>+ local slot = ({...})[1]
>>+ -- Forcify stitch and usage of vararg slot.
>>+ return fmod(ARG_ON_RECORDING, slot)
>Are there any reasons behind the `fmod` choice? If so, please drop a comment.
>>+end
>>+
>>+jit.opt.start('hotloop=1')
>>+
>>+local _, result
>>+local function wrap(arg)
>>+ -- `pcall()` is needed to emit snapshot to handle on-trace
>>+ -- errors.
>Maybe it is worth mentioning Mike’s original comment[1] here.
>Feel free to ignore.
>>+ _, result = pcall(varg, arg)
>>+end
>>+-- Record trace with the 0 result.
>>+wrap(ARG_ON_RECORDING)
>>+wrap(ARG_ON_RECORDING)
>>+-- Record trace with the non-zero result.
>>+wrap(ON_TRACE_VALUE)
>>+
>>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>>+
>>+os.exit(test:check() and 0 or 1)
>>--
>>2.34.1
>[1]:  https://github.com/LuaJIT/LuaJIT/issues/704
>--
>Best regards,
>Maxim Kokryashkin

[-- Attachment #2: Type: text/html, Size: 7246 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
 
> 
>>From: Mike Pall <mike>
>>
>>Reported by Shmuel Zeigerman.
>>
>>(cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57)
>>
>>Use-def analysis for BC_FUNCV may consider slots greater than amount of
>Typo: s/than/than the/
>>non-vararg parameters as dead slots due to early return for BC_RET
>Typo: s/due to/due to the/
>Also, it’d be nice to mention the exact spot where the early return
>is use-def analysis happens.
>>emitted before usage of BC_VARG. This patch restricts the maxslot to be
>>analyzed in such case with amount of parameters for the prototype of the
>Typo: s/with/with the/
>>current function being recorded.
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8516
>>Relates to tarantool/tarantool#8718
>>---
>> src/lj_snap.c | 6 +++--
>> .../lj-704-bc-varg-use-def.test.lua | 27 ++++++++++++++++++-
>> 2 files changed, 30 insertions(+), 3 deletions(-)
>>
>>diff --git a/src/lj_snap.c b/src/lj_snap.c
>>index 5bbe8498..a063c316 100644
>>--- a/src/lj_snap.c
>>+++ b/src/lj_snap.c
>>@@ -301,8 +301,10 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
>> void lj_snap_purge(jit_State *J)
>> {
>>   uint8_t udf[SNAP_USEDEF_SLOTS];
>>- BCReg maxslot = J->maxslot;
>>- BCReg s = snap_usedef(J, udf, J->pc, maxslot);
>>+ BCReg s, maxslot = J->maxslot;
>>+ if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams)
>>+ maxslot = J->pt->numparams;
>>+ s = snap_usedef(J, udf, J->pc, maxslot);
>>   for (; s < maxslot; s++)
>>     if (udf[s] != 0)
>>       J->base[s] = 0; /* Purge dead slots. */
>>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>index c3ba65dd..38606686 100644
>>--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>@@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>   ['Test requires JIT enabled'] = not jit.status(),
>> })
>> 
>>-test:plan(1)
>>+test:plan(2)
>> 
>> -- XXX: we don't really need to store this builtins, but this is
>> -- reduces `jitdump()` output for reader significantly.
>>@@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE)
>> 
>> test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>> 
>>+-- Now check the same case, but with BC_RET before the BC_VARG,
>>+-- so use-def analysis will take early return case.
>Same as in the commit message, please mention the exact spot.
>>+-- See `snap_usedef()` in <src/lj_snap.c> for details.
>>+-- The test checks that slots greater than `numparams` are not
>>+-- purged.
>>+local function varg_ret_bc(...)
>>+ -- XXX: This branch contains BC_RET. See the comment above.
>>+ -- luacheck: ignore
>>+ if false then else end
>>+ local slot = ({...})[1]
>>+ return fmod(ARG_ON_RECORDING, slot)
>Is there any reason why it has to be `fmod`?
>>+end
>>+
>>+local function wrap_ret_bc(arg)
>>+ _, result = pcall(varg_ret_bc, arg)
>>+end
>Is it possible to adapt the `wrap` function from the test
>for the previous patch, so it can be used for both tests?
>>+
>>+-- Record trace with the 0 result.
>>+wrap_ret_bc(ARG_ON_RECORDING)
>>+wrap_ret_bc(ARG_ON_RECORDING)
>>+-- Record trace with the non-zero result.
>>+wrap_ret_bc(ON_TRACE_VALUE)
>>+
>>+test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')
>>+
>> os.exit(test:check() and 0 or 1)
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 5369 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 14.06.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Ryan Lucia.
> >>
> >>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
> >>
> >>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
> >Typo: s/analizis/analysis/

Fixed! Thanks!

> >>no slots may considered as used. This leads to addititional SLOAD on
> >Typo: s/may/may be/
> >Typo: s/to additional/to an additional/

Fixed.

> >>trace with incorrect value used later. This patch disables the use-def
> >Typo: s/trace with/the trace with an/

Fixed.

> >>analisis for BC_VARG as NIY.
> >Typo: s/analisis/analysis/

Fixed, thanks!

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>Relates to tarantool/tarantool#8718
> >>---
> >> src/lj_snap.c | 4 +-
> >> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++
> >> 2 files changed, 68 insertions(+), 1 deletion(-)
> >> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>

<snipped>

> >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>new file mode 100644
> >>index 00000000..c3ba65dd
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>@@ -0,0 +1,65 @@
> >>+local tap = require('tap')
> >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
> >>+-- snapshot analysis for BC_VARG.
> >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
> >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> >>+ ['Test requires JIT enabled'] = not jit.status(),
> >>+})
> >>+
> >>+test:plan(1)
> >>+
> >>+-- XXX: we don't really need to store this builtins, but this is
> >Typo: s/this/these/

Fixed, thanks!

> >>+-- reduces `jitdump()` output for reader significantly.
> >>+local fmod = math.fmod
> >>+local pcall = pcall
> >>+
> >>+-- Use the 2 values for `fmod()` to produce non-zero value for
> >>+-- the call on trace (the last one call).
> >>+local ARG_ON_RECORDING = 6
> >>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
> >Why are they exactly 6 and 7? Please drop a comment.

No special meaning, added a comment.

> >>+
> >>+-- The `jitdump()` output was like the following before the patch:
> >>+-- 0003 > num SLOAD #1 T
> >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
> >>+-- 0004 } tab TNEW #3 #0
> >>+-- 0005 > num SLOAD #4 T
> >>+-- 0006 p32 FLOAD 0004 tab.array
> >>+-- 0007 p32 AREF 0006 +1
> >>+-- 0008 } num ASTORE 0007 0005
> >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
> >>+--
> >>+-- The first snapshot misses the 0003 IR in the last slot to be
> >>+-- used in the `fmod()` later, so it leads to the additional
> >>+-- 0005 SLOAD #4, and storing it in the second snapshot.
> >>+--
> >>+-- The correct snapshot content after the patch is the following:
> >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
> >>+-- ....
> >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
> >>+local function varg(...)
> >>+ -- Generate snapshot after `pcall()` with missing slot.
> >>+ -- The snapshot is generated before each TNEW after the commit
> >>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
> >>+ -- OOM errors from helper functions.")
> >>+ local slot = ({...})[1]
> >>+ -- Forcify stitch and usage of vararg slot.
> >>+ return fmod(ARG_ON_RECORDING, slot)
> >Are there any reasons behind the `fmod` choice? If so, please drop a comment.

No, added the comment.

> >>+end
> >>+
> >>+jit.opt.start('hotloop=1')
> >>+
> >>+local _, result
> >>+local function wrap(arg)
> >>+ -- `pcall()` is needed to emit snapshot to handle on-trace
> >>+ -- errors.
> >Maybe it is worth mentioning Mike’s original comment[1] here.
> >Feel free to ignore.

I just added the reference to the issue in the header, the comment above
is about the same as Mike's but more verbose.

===================================================================
diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
index c3ba65dd..3608ea4e 100644
--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
@@ -8,13 +8,13 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
 
 test:plan(1)
 
--- XXX: we don't really need to store this builtins, but this is
+-- XXX: we don't really need to store these builtins, but this is
 -- reduces `jitdump()` output for reader significantly.
 local fmod = math.fmod
 local pcall = pcall
 
 -- Use the 2 values for `fmod()` to produce non-zero value for
--- the call on trace (the last one call).
+-- the call on trace (the last one call). No special meaning.
 local ARG_ON_RECORDING = 6
 local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
 
@@ -42,23 +42,23 @@ local function varg(...)
   -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
   -- OOM errors from helper functions.")
   local slot = ({...})[1]
-  -- Forcify stitch and usage of vararg slot.
+  -- Forcify stitch and usage of vararg slot. Any NIY is OK here.
   return fmod(ARG_ON_RECORDING, slot)
 end
 
 jit.opt.start('hotloop=1')
 
 local _, result
-local function wrap(arg)
+local function wrap(func, arg)
   -- `pcall()` is needed to emit snapshot to handle on-trace
   -- errors.
-  _, result = pcall(varg, arg)
+  _, result = pcall(func, arg)
 end
 -- Record trace with the 0 result.
-wrap(ARG_ON_RECORDING)
-wrap(ARG_ON_RECORDING)
+wrap(varg, ARG_ON_RECORDING)
+wrap(varg, ARG_ON_RECORDING)
 -- Record trace with the non-zero result.
-wrap(ON_TRACE_VALUE)
+wrap(varg, ON_TRACE_VALUE)
 
 test:ok(result ~= 0, 'use-def analysis for BC_VARG')
===================================================================

I also modify `wrap()` function to get the function to call considering
your comments in the next patch.

> >>+ _, result = pcall(varg, arg)
> >>+end
> >>+-- Record trace with the 0 result.
> >>+wrap(ARG_ON_RECORDING)
> >>+wrap(ARG_ON_RECORDING)
> >>+-- Record trace with the non-zero result.
> >>+wrap(ON_TRACE_VALUE)
> >>+
> >>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> >>+
> >>+os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >[1]:  https://github.com/LuaJIT/LuaJIT/issues/704
> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Sergey Kaplun via Tarantool-patches]

On 21.06.23, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maxim!
> Thanks for the review!
> 
> On 14.06.23, Maxim Kokryashkin wrote:
> > 
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> >  
> > > 
> > >>From: Mike Pall <mike>
> > >>
> > >>Reported by Ryan Lucia.
> > >>
> > >>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
> > >>
> > >>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
> > >Typo: s/analizis/analysis/
> 
> Fixed! Thanks!
> 
> > >>no slots may considered as used. This leads to addititional SLOAD on
> > >Typo: s/may/may be/
> > >Typo: s/to additional/to an additional/
> 
> Fixed.
> 
> > >>trace with incorrect value used later. This patch disables the use-def
> > >Typo: s/trace with/the trace with an/
> 
> Fixed.
> 
> > >>analisis for BC_VARG as NIY.
> > >Typo: s/analisis/analysis/
> 
> Fixed, thanks!
> 
> > >>
> > >>Sergey Kaplun:
> > >>* added the description and the test for the problem
> > >>
> > >>Part of tarantool/tarantool#8516
> > >>Relates to tarantool/tarantool#8718
> > >>---
> > >> src/lj_snap.c | 4 +-
> > >> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++
> > >> 2 files changed, 68 insertions(+), 1 deletion(-)
> > >> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> > >>
> 
> <snipped>
> 
> > >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> > >>new file mode 100644
> > >>index 00000000..c3ba65dd
> > >>--- /dev/null
> > >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> > >>@@ -0,0 +1,65 @@
> > >>+local tap = require('tap')
> > >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
> > >>+-- snapshot analysis for BC_VARG.
> > >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
> > >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> > >>+ ['Test requires JIT enabled'] = not jit.status(),
> > >>+})
> > >>+
> > >>+test:plan(1)
> > >>+
> > >>+-- XXX: we don't really need to store this builtins, but this is
> > >Typo: s/this/these/
> 
> Fixed, thanks!
> 
> > >>+-- reduces `jitdump()` output for reader significantly.
> > >>+local fmod = math.fmod
> > >>+local pcall = pcall
> > >>+
> > >>+-- Use the 2 values for `fmod()` to produce non-zero value for
> > >>+-- the call on trace (the last one call).
> > >>+local ARG_ON_RECORDING = 6
> > >>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
> > >Why are they exactly 6 and 7? Please drop a comment.
> 
> No special meaning, added a comment.
> 
> > >>+
> > >>+-- The `jitdump()` output was like the following before the patch:
> > >>+-- 0003 > num SLOAD #1 T
> > >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
> > >>+-- 0004 } tab TNEW #3 #0
> > >>+-- 0005 > num SLOAD #4 T
> > >>+-- 0006 p32 FLOAD 0004 tab.array
> > >>+-- 0007 p32 AREF 0006 +1
> > >>+-- 0008 } num ASTORE 0007 0005
> > >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
> > >>+--
> > >>+-- The first snapshot misses the 0003 IR in the last slot to be
> > >>+-- used in the `fmod()` later, so it leads to the additional
> > >>+-- 0005 SLOAD #4, and storing it in the second snapshot.
> > >>+--
> > >>+-- The correct snapshot content after the patch is the following:
> > >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
> > >>+-- ....
> > >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
> > >>+local function varg(...)
> > >>+ -- Generate snapshot after `pcall()` with missing slot.
> > >>+ -- The snapshot is generated before each TNEW after the commit
> > >>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
> > >>+ -- OOM errors from helper functions.")
> > >>+ local slot = ({...})[1]
> > >>+ -- Forcify stitch and usage of vararg slot.
> > >>+ return fmod(ARG_ON_RECORDING, slot)
> > >Are there any reasons behind the `fmod` choice? If so, please drop a comment.
> 
> No, added the comment.
> 
> > >>+end
> > >>+
> > >>+jit.opt.start('hotloop=1')
> > >>+
> > >>+local _, result
> > >>+local function wrap(arg)
> > >>+ -- `pcall()` is needed to emit snapshot to handle on-trace
> > >>+ -- errors.
> > >Maybe it is worth mentioning Mike’s original comment[1] here.
> > >Feel free to ignore.
> 
> I just added the reference to the issue in the header, the comment above
> is about the same as Mike's but more verbose.
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> index c3ba65dd..3608ea4e 100644
> --- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> +++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> @@ -8,13 +8,13 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>  
>  test:plan(1)
>  
> --- XXX: we don't really need to store this builtins, but this is
> +-- XXX: we don't really need to store these builtins, but this is
>  -- reduces `jitdump()` output for reader significantly.
>  local fmod = math.fmod
>  local pcall = pcall
>  
>  -- Use the 2 values for `fmod()` to produce non-zero value for
> --- the call on trace (the last one call).
> +-- the call on trace (the last one call). No special meaning.
>  local ARG_ON_RECORDING = 6
>  local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
>  
> @@ -42,23 +42,23 @@ local function varg(...)
>    -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
>    -- OOM errors from helper functions.")
>    local slot = ({...})[1]
> -  -- Forcify stitch and usage of vararg slot.
> +  -- Forcify stitch and usage of vararg slot. Any NIY is OK here.
>    return fmod(ARG_ON_RECORDING, slot)
>  end
>  
>  jit.opt.start('hotloop=1')
>  
>  local _, result
> -local function wrap(arg)
> +local function wrap(func, arg)
>    -- `pcall()` is needed to emit snapshot to handle on-trace
>    -- errors.
> -  _, result = pcall(varg, arg)
> +  _, result = pcall(func, arg)
>  end
>  -- Record trace with the 0 result.
> -wrap(ARG_ON_RECORDING)
> -wrap(ARG_ON_RECORDING)
> +wrap(varg, ARG_ON_RECORDING)
> +wrap(varg, ARG_ON_RECORDING)
>  -- Record trace with the non-zero result.
> -wrap(ON_TRACE_VALUE)
> +wrap(varg, ON_TRACE_VALUE)

Brr, acturally, we need to separate two `wrap()` functions - to prevent
compilation for the `wrap()` itself as non pcall-ed fixed-arg function.
Added the comment.

>  
>  test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> ===================================================================
> 
> I also modify `wrap()` function to get the function to call considering
> your comments in the next patch.
> 
> > >>+ _, result = pcall(varg, arg)
> > >>+end
> > >>+-- Record trace with the 0 result.
> > >>+wrap(ARG_ON_RECORDING)
> > >>+wrap(ARG_ON_RECORDING)
> > >>+-- Record trace with the non-zero result.
> > >>+wrap(ON_TRACE_VALUE)
> > >>+
> > >>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> > >>+
> > >>+os.exit(test:check() and 0 or 1)
> > >>--
> > >>2.34.1
> > >[1]:  https://github.com/LuaJIT/LuaJIT/issues/704
> > >--
> > >Best regards,
> > >Maxim Kokryashkin
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments on the branch!

On 16.06.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Shmuel Zeigerman.
> >>
> >>(cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57)
> >>
> >>Use-def analysis for BC_FUNCV may consider slots greater than amount of
> >Typo: s/than/than the/

Fixed.

> >>non-vararg parameters as dead slots due to early return for BC_RET
> >Typo: s/due to/due to the/

Fixed.

> >Also, it’d be nice to mention the exact spot where the early return
> >is use-def analysis happens.

Added.

> >>emitted before usage of BC_VARG. This patch restricts the maxslot to be
> >>analyzed in such case with amount of parameters for the prototype of the
> >Typo: s/with/with the/

Fixed.

> >>current function being recorded.
> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>Relates to tarantool/tarantool#8718
> >>---
> >> src/lj_snap.c | 6 +++--
> >> .../lj-704-bc-varg-use-def.test.lua | 27 ++++++++++++++++++-
> >> 2 files changed, 30 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/src/lj_snap.c b/src/lj_snap.c
> >>index 5bbe8498..a063c316 100644
> >>--- a/src/lj_snap.c
> >>+++ b/src/lj_snap.c
> >>@@ -301,8 +301,10 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
> >> void lj_snap_purge(jit_State *J)
> >> {
> >>   uint8_t udf[SNAP_USEDEF_SLOTS];
> >>- BCReg maxslot = J->maxslot;
> >>- BCReg s = snap_usedef(J, udf, J->pc, maxslot);
> >>+ BCReg s, maxslot = J->maxslot;
> >>+ if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams)
> >>+ maxslot = J->pt->numparams;
> >>+ s = snap_usedef(J, udf, J->pc, maxslot);
> >>   for (; s < maxslot; s++)
> >>     if (udf[s] != 0)
> >>       J->base[s] = 0; /* Purge dead slots. */
> >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>index c3ba65dd..38606686 100644
> >>--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> >>@@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> >>   ['Test requires JIT enabled'] = not jit.status(),
> >> })
> >> 
> >>-test:plan(1)
> >>+test:plan(2)
> >> 
> >> -- XXX: we don't really need to store this builtins, but this is
> >> -- reduces `jitdump()` output for reader significantly.
> >>@@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE)
> >> 
> >> test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> >> 
> >>+-- Now check the same case, but with BC_RET before the BC_VARG,
> >>+-- so use-def analysis will take early return case.
> >Same as in the commit message, please mention the exact spot.
> >>+-- See `snap_usedef()` in <src/lj_snap.c> for details.
> >>+-- The test checks that slots greater than `numparams` are not
> >>+-- purged.
> >>+local function varg_ret_bc(...)
> >>+ -- XXX: This branch contains BC_RET. See the comment above.
> >>+ -- luacheck: ignore
> >>+ if false then else end
> >>+ local slot = ({...})[1]
> >>+ return fmod(ARG_ON_RECORDING, slot)
> >Is there any reason why it has to be `fmod`?

Added the comment.

> >>+end
> >>+
> >>+local function wrap_ret_bc(arg)
> >>+ _, result = pcall(varg_ret_bc, arg)
> >>+end
> >Is it possible to adapt the `wrap` function from the test
> >for the previous patch, so it can be used for both tests?

Unfortunately no, because we want to record not wrap function (at the
second test), but the one is pcall-ed.
Added the comment.

> >>+
> >>+-- Record trace with the 0 result.
> >>+wrap_ret_bc(ARG_ON_RECORDING)
> >>+wrap_ret_bc(ARG_ON_RECORDING)
> >>+-- Record trace with the non-zero result.
> >>+wrap_ret_bc(ON_TRACE_VALUE)
> >>+
> >>+test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')
> >>+
> >> os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fixes!
LGTM, except for a single nit below.
 
> 
>>On 21.06.23, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Maxim!
>>> Thanks for the review!
>>>
>>> On 14.06.23, Maxim Kokryashkin wrote:
>>> >
>>> > Hi, Sergey!
>>> > Thanks for the patch!
>>> > Please consider my comments below.
>>> >  
>>> > > 
>>> > >>From: Mike Pall <mike>
>>> > >>
>>> > >>Reported by Ryan Lucia.
>>> > >>
>>> > >>(cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
>>> > >>
>>> > >>Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
>>> > >Typo: s/analizis/analysis/
>>>
>>> Fixed! Thanks!
>>>
>>> > >>no slots may considered as used. This leads to addititional SLOAD on
>>> > >Typo: s/may/may be/
>>> > >Typo: s/to additional/to an additional/
>>>
>>> Fixed.
>>>
>>> > >>trace with incorrect value used later. This patch disables the use-def
>>> > >Typo: s/trace with/the trace with an/
>>>
>>> Fixed.
>>>
>>> > >>analisis for BC_VARG as NIY.
>>> > >Typo: s/analisis/analysis/
>>>
>>> Fixed, thanks!
>>>
>>> > >>
>>> > >>Sergey Kaplun:
>>> > >>* added the description and the test for the problem
>>> > >>
>>> > >>Part of tarantool/tarantool#8516
>>> > >>Relates to tarantool/tarantool#8718
>>> > >>---
>>> > >> src/lj_snap.c | 4 +-
>>> > >> .../lj-704-bc-varg-use-def.test.lua | 65 +++++++++++++++++++
>>> > >> 2 files changed, 68 insertions(+), 1 deletion(-)
>>> > >> create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> > >>
>>>
>>> <snipped>
>>>
>>> > >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> > >>new file mode 100644
>>> > >>index 00000000..c3ba65dd
>>> > >>--- /dev/null
>>> > >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> > >>@@ -0,0 +1,65 @@
>>> > >>+local tap = require('tap')
>>> > >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
>>> > >>+-- snapshot analysis for BC_VARG.
>>> > >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
>>> > >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>> > >>+ ['Test requires JIT enabled'] = not jit.status(),
>>> > >>+})
>>> > >>+
>>> > >>+test:plan(1)
>>> > >>+
>>> > >>+-- XXX: we don't really need to store this builtins, but this is
>>> > >Typo: s/this/these/
>>>
>>> Fixed, thanks!
>Typo: s/this is/this/
>>>
>>> > >>+-- reduces `jitdump()` output for reader significantly.
>>> > >>+local fmod = math.fmod
>>> > >>+local pcall = pcall
>>> > >>+
>>> > >>+-- Use the 2 values for `fmod()` to produce non-zero value for
>>> > >>+-- the call on trace (the last one call).
>>> > >>+local ARG_ON_RECORDING = 6
>>> > >>+local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
>>> > >Why are they exactly 6 and 7? Please drop a comment.
>>>
>>> No special meaning, added a comment.
>>>
>>> > >>+
>>> > >>+-- The `jitdump()` output was like the following before the patch:
>>> > >>+-- 0003 > num SLOAD #1 T
>>> > >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
>>> > >>+-- 0004 } tab TNEW #3 #0
>>> > >>+-- 0005 > num SLOAD #4 T
>>> > >>+-- 0006 p32 FLOAD 0004 tab.array
>>> > >>+-- 0007 p32 AREF 0006 +1
>>> > >>+-- 0008 } num ASTORE 0007 0005
>>> > >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
>>> > >>+--
>>> > >>+-- The first snapshot misses the 0003 IR in the last slot to be
>>> > >>+-- used in the `fmod()` later, so it leads to the additional
>>> > >>+-- 0005 SLOAD #4, and storing it in the second snapshot.
>>> > >>+--
>>> > >>+-- The correct snapshot content after the patch is the following:
>>> > >>+-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
>>> > >>+-- ....
>>> > >>+-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
>>> > >>+local function varg(...)
>>> > >>+ -- Generate snapshot after `pcall()` with missing slot.
>>> > >>+ -- The snapshot is generated before each TNEW after the commit
>>> > >>+ -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
>>> > >>+ -- OOM errors from helper functions.")
>>> > >>+ local slot = ({...})[1]
>>> > >>+ -- Forcify stitch and usage of vararg slot.
>>> > >>+ return fmod(ARG_ON_RECORDING, slot)
>>> > >Are there any reasons behind the `fmod` choice? If so, please drop a comment.
>>>
>>> No, added the comment.
>>>
>>> > >>+end
>>> > >>+
>>> > >>+jit.opt.start('hotloop=1')
>>> > >>+
>>> > >>+local _, result
>>> > >>+local function wrap(arg)
>>> > >>+ -- `pcall()` is needed to emit snapshot to handle on-trace
>>> > >>+ -- errors.
>>> > >Maybe it is worth mentioning Mike’s original comment[1] here.
>>> > >Feel free to ignore.
>>>
>>> I just added the reference to the issue in the header, the comment above
>>> is about the same as Mike's but more verbose.
>>>
>>> ===================================================================
>>> diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> index c3ba65dd..3608ea4e 100644
>>> --- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> +++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> @@ -8,13 +8,13 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>>
>>> test:plan(1)
>>>
>>> --- XXX: we don't really need to store this builtins, but this is
>>> +-- XXX: we don't really need to store these builtins, but this is
>>> -- reduces `jitdump()` output for reader significantly.
>>> local fmod = math.fmod
>>> local pcall = pcall
>>>
>>> -- Use the 2 values for `fmod()` to produce non-zero value for
>>> --- the call on trace (the last one call).
>>> +-- the call on trace (the last one call). No special meaning.
>>> local ARG_ON_RECORDING = 6
>>> local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
>>>
>>> @@ -42,23 +42,23 @@ local function varg(...)
>>> -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
>>> -- OOM errors from helper functions.")
>>> local slot = ({...})[1]
>>> - -- Forcify stitch and usage of vararg slot.
>>> + -- Forcify stitch and usage of vararg slot. Any NIY is OK here.
>>> return fmod(ARG_ON_RECORDING, slot)
>>> end
>>>
>>> jit.opt.start('hotloop=1')
>>>
>>> local _, result
>>> -local function wrap(arg)
>>> +local function wrap(func, arg)
>>> -- `pcall()` is needed to emit snapshot to handle on-trace
>>> -- errors.
>>> - _, result = pcall(varg, arg)
>>> + _, result = pcall(func, arg)
>>> end
>>> -- Record trace with the 0 result.
>>> -wrap(ARG_ON_RECORDING)
>>> -wrap(ARG_ON_RECORDING)
>>> +wrap(varg, ARG_ON_RECORDING)
>>> +wrap(varg, ARG_ON_RECORDING)
>>> -- Record trace with the non-zero result.
>>> -wrap(ON_TRACE_VALUE)
>>> +wrap(varg, ON_TRACE_VALUE)
>>
>>Brr, acturally, we need to separate two `wrap()` functions - to prevent
>>compilation for the `wrap()` itself as non pcall-ed fixed-arg function.
>>Added the comment.
>>
>>>
>>> test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>>> ===================================================================
>>>
>>> I also modify `wrap()` function to get the function to call considering
>>> your comments in the next patch.
>>>
>>> > >>+ _, result = pcall(varg, arg)
>>> > >>+end
>>> > >>+-- Record trace with the 0 result.
>>> > >>+wrap(ARG_ON_RECORDING)
>>> > >>+wrap(ARG_ON_RECORDING)
>>> > >>+-- Record trace with the non-zero result.
>>> > >>+wrap(ON_TRACE_VALUE)
>>> > >>+
>>> > >>+test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>>> > >>+
>>> > >>+os.exit(test:check() and 0 or 1)
>>> > >>--
>>> > >>2.34.1
>>> > >[1]:   https://github.com/LuaJIT/LuaJIT/issues/704
>>> > >--
>>> > >Best regards,
>>> > >Maxim Kokryashkin
>>>
>>> --
>>> Best regards,
>>> Sergey Kaplun
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 10282 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comments on the branch!
>>
>>On 16.06.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please consider my comments below.
>>>  
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Reported by Shmuel Zeigerman.
>>> >>
>>> >>(cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57)
>>> >>
>>> >>Use-def analysis for BC_FUNCV may consider slots greater than amount of
>>> >Typo: s/than/than the/
>>
>>Fixed.
>>
>>> >>non-vararg parameters as dead slots due to early return for BC_RET
>>> >Typo: s/due to/due to the/
>>
>>Fixed.
>>
>>> >Also, it’d be nice to mention the exact spot where the early return
>>> >is use-def analysis happens.
>>
>>Added.
>>
>>> >>emitted before usage of BC_VARG. This patch restricts the maxslot to be
>>> >>analyzed in such case with amount of parameters for the prototype of the
>>> >Typo: s/with/with the/
>>
>>Fixed.
>>
>>> >>current function being recorded.
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8516
>>> >>Relates to tarantool/tarantool#8718
>>> >>---
>>> >> src/lj_snap.c | 6 +++--
>>> >> .../lj-704-bc-varg-use-def.test.lua | 27 ++++++++++++++++++-
>>> >> 2 files changed, 30 insertions(+), 3 deletions(-)
>>> >>
>>> >>diff --git a/src/lj_snap.c b/src/lj_snap.c
>>> >>index 5bbe8498..a063c316 100644
>>> >>--- a/src/lj_snap.c
>>> >>+++ b/src/lj_snap.c
>>> >>@@ -301,8 +301,10 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
>>> >> void lj_snap_purge(jit_State *J)
>>> >> {
>>> >>   uint8_t udf[SNAP_USEDEF_SLOTS];
>>> >>- BCReg maxslot = J->maxslot;
>>> >>- BCReg s = snap_usedef(J, udf, J->pc, maxslot);
>>> >>+ BCReg s, maxslot = J->maxslot;
>>> >>+ if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams)
>>> >>+ maxslot = J->pt->numparams;
>>> >>+ s = snap_usedef(J, udf, J->pc, maxslot);
>>> >>   for (; s < maxslot; s++)
>>> >>     if (udf[s] != 0)
>>> >>       J->base[s] = 0; /* Purge dead slots. */
>>> >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> >>index c3ba65dd..38606686 100644
>>> >>--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
>>> >>@@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>> >>   ['Test requires JIT enabled'] = not jit.status(),
>>> >> })
>>> >> 
>>> >>-test:plan(1)
>>> >>+test:plan(2)
>>> >> 
>>> >> -- XXX: we don't really need to store this builtins, but this is
>>> >> -- reduces `jitdump()` output for reader significantly.
>>> >>@@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE)
>>> >> 
>>> >> test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>>> >> 
>>> >>+-- Now check the same case, but with BC_RET before the BC_VARG,
>>> >>+-- so use-def analysis will take early return case.
>>> >Same as in the commit message, please mention the exact spot.
>>> >>+-- See `snap_usedef()` in <src/lj_snap.c> for details.
>>> >>+-- The test checks that slots greater than `numparams` are not
>>> >>+-- purged.
>>> >>+local function varg_ret_bc(...)
>>> >>+ -- XXX: This branch contains BC_RET. See the comment above.
>>> >>+ -- luacheck: ignore
>>> >>+ if false then else end
>>> >>+ local slot = ({...})[1]
>>> >>+ return fmod(ARG_ON_RECORDING, slot)
>>> >Is there any reason why it has to be `fmod`?
>>
>>Added the comment.
>>
>>> >>+end
>>> >>+
>>> >>+local function wrap_ret_bc(arg)
>>> >>+ _, result = pcall(varg_ret_bc, arg)
>>> >>+end
>>> >Is it possible to adapt the `wrap` function from the test
>>> >for the previous patch, so it can be used for both tests?
>>
>>Unfortunately no, because we want to record not wrap function (at the
>>second test), but the one is pcall-ed.
>>Added the comment.
>>
>>> >>+
>>> >>+-- Record trace with the 0 result.
>>> >>+wrap_ret_bc(ARG_ON_RECORDING)
>>> >>+wrap_ret_bc(ARG_ON_RECORDING)
>>> >>+-- Record trace with the non-zero result.
>>> >>+wrap_ret_bc(ON_TRACE_VALUE)
>>> >>+
>>> >>+test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')
>>> >>+
>>> >> os.exit(test:check() and 0 or 1)
>>> >>--
>>> >>2.34.1
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 6154 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 22.06.23, Maxim Kokryashkin wrote:
> 
> Hi!
> Thanks for the fixes!
> LGTM, except for a single nit below.
>  
> > 

<snipped>

> >>> > >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
> >>> > >>+-- snapshot analysis for BC_VARG.
> >>> > >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
> >>> > >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> >>> > >>+ ['Test requires JIT enabled'] = not jit.status(),
> >>> > >>+})
> >>> > >>+
> >>> > >>+test:plan(1)
> >>> > >>+
> >>> > >>+-- XXX: we don't really need to store this builtins, but this is
> >>> > >Typo: s/this/these/
> >>>
> >>> Fixed, thanks!
> >Typo: s/this is/this/

Fixed, thanks!

> >>>

<snipped>

> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the fix!
LGTM
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comment and force-pushed the branch.
>>
>>On 22.06.23, Maxim Kokryashkin wrote:
>>>
>>> Hi!
>>> Thanks for the fixes!
>>> LGTM, except for a single nit below.
>>>  
>>> > 
>>
>><snipped>
>>
>>> >>> > >>+-- Test file to demonstrate LuaJIT misbehaviour in use-def
>>> >>> > >>+-- snapshot analysis for BC_VARG.
>>> >>> > >>+-- See also  https://github.com/LuaJIT/LuaJIT/issues/704 .
>>> >>> > >>+local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>>> >>> > >>+ ['Test requires JIT enabled'] = not jit.status(),
>>> >>> > >>+})
>>> >>> > >>+
>>> >>> > >>+test:plan(1)
>>> >>> > >>+
>>> >>> > >>+-- XXX: we don't really need to store this builtins, but this is
>>> >>> > >Typo: s/this/these/
>>> >>>
>>> >>> Fixed, thanks!
>>> >Typo: s/this is/this/
>>
>>Fixed, thanks!
>>
>>> >>>
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 1840 bytes --]

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix use-def analysis for BC_VARG.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, considering the fixes made to resolve the
comments left by Max.

On 09.06.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by Ryan Lucia.
> 
> (cherry-picked from commit 2801500a26084491ae035170cad4700513790890)
> 
> Use-def analizis for BC_VARG has to strong limit for the top/maxslot, so
> no slots may considered as used. This leads to addititional SLOAD on

Side note: s/addititional/additional/. I've already fixed this.

> trace with incorrect value used later. This patch disables the use-def
> analisis for BC_VARG as NIY.

Side note: s/NIY/NYI/. I've already fixed this.

> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8516
> Relates to tarantool/tarantool#8718
> ---
>  src/lj_snap.c                                 |  4 +-
>  .../lj-704-bc-varg-use-def.test.lua           | 65 +++++++++++++++++++
>  2 files changed, 68 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> new file mode 100644
> index 00000000..c3ba65dd
> --- /dev/null
> +++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> @@ -0,0 +1,65 @@
> +local tap = require('tap')
> +-- Test file to demonstrate LuaJIT misbehaviour in use-def
> +-- snapshot analysis for BC_VARG.
> +-- See also https://github.com/LuaJIT/LuaJIT/issues/704.
> +local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +-- XXX: we don't really need to store this builtins, but this is
> +-- reduces `jitdump()` output for reader significantly.
> +local fmod = math.fmod
> +local pcall = pcall
> +
> +-- Use the 2 values for `fmod()` to produce non-zero value for
> +-- the call on trace (the last one call).
> +local ARG_ON_RECORDING = 6
> +local ON_TRACE_VALUE = ARG_ON_RECORDING + 1
> +
> +-- The `jitdump()` output was like the following before the patch:
> +-- 0003 >  num SLOAD  #1    T
> +-- ....        SNAP   #1   [`wrap()`|---- pcall|`varg()`|----]
> +-- 0004 }  tab TNEW   #3    #0
> +-- 0005 >  num SLOAD  #4    T
> +-- 0006    p32 FLOAD  0004  tab.array
> +-- 0007    p32 AREF   0006  +1
> +-- 0008 }  num ASTORE 0007  0005
> +-- ....        SNAP   #2   [`wrap()`|---- pcall|math.fmod|+6 0005]
> +--
> +-- The first snapshot misses the 0003 IR in the last slot to be
> +-- used in the `fmod()` later, so it leads to the additional
> +-- 0005 SLOAD #4, and storing it in the second snapshot.
> +--
> +-- The correct snapshot content after the patch is the following:
> +-- ....        SNAP   #1   [`wrap()`|---- pcall|`varg()`|0003]
> +-- ....
> +-- ....        SNAP   #2   [`wrap()`|---- pcall|math.fmod|+6 0003]
> +local function varg(...)
> +  -- Generate snapshot after `pcall()` with missing slot.
> +  -- The snapshot is generated before each TNEW after the commit
> +  -- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
> +  -- OOM errors from helper functions.")
> +  local slot = ({...})[1]
> +  -- Forcify stitch and usage of vararg slot.
> +  return fmod(ARG_ON_RECORDING, slot)
> +end
> +
> +jit.opt.start('hotloop=1')
> +
> +local _, result
> +local function wrap(arg)
> +  -- `pcall()` is needed to emit snapshot to handle on-trace
> +  -- errors.
> +  _, result = pcall(varg, arg)
> +end
> +-- Record trace with the 0 result.
> +wrap(ARG_ON_RECORDING)
> +wrap(ARG_ON_RECORDING)
> +-- Record trace with the non-zero result.
> +wrap(ON_TRACE_VALUE)
> +
> +test:ok(result ~= 0, 'use-def analysis for BC_VARG')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, considering the fixes made to resolve the
comments left by Max. I also applied the changes we discussed offline.

On 09.06.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by Shmuel Zeigerman.
> 
> (cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57)
> 
> Use-def analysis for BC_FUNCV may consider slots greater than amount of
> non-vararg parameters as dead slots due to early return for BC_RET

Side note: mentioned BC_JMP here too.

> emitted before usage of BC_VARG. This patch restricts the maxslot to be
> analyzed in such case with amount of parameters for the prototype of the
> current function being recorded.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8516
> Relates to tarantool/tarantool#8718
> ---
>  src/lj_snap.c                                 |  6 +++--
>  .../lj-704-bc-varg-use-def.test.lua           | 27 ++++++++++++++++++-
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 

<snipped>

> diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> index c3ba65dd..38606686 100644
> --- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> +++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> @@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
>    ['Test requires JIT enabled'] = not jit.status(),
>  })
>  
> -test:plan(1)
> +test:plan(2)
>  
>  -- XXX: we don't really need to store this builtins, but this is
>  -- reduces `jitdump()` output for reader significantly.
> @@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE)
>  
>  test:ok(result ~= 0, 'use-def analysis for BC_VARG')
>  
> +-- Now check the same case, but with BC_RET before the BC_VARG,
> +-- so use-def analysis will take early return case.
> +-- See `snap_usedef()` in <src/lj_snap.c> for details.
> +-- The test checks that slots greater than `numparams` are not
> +-- purged.
> +local function varg_ret_bc(...)
> +  -- XXX: This branch contains BC_RET. See the comment above.
> +  -- luacheck: ignore
> +  if false then else end
> +  local slot = ({...})[1]
> +  return fmod(ARG_ON_RECORDING, slot)
> +end
> +
> +local function wrap_ret_bc(arg)
> +  _, result = pcall(varg_ret_bc, arg)
> +end
> +
> +-- Record trace with the 0 result.
> +wrap_ret_bc(ARG_ON_RECORDING)
> +wrap_ret_bc(ARG_ON_RECORDING)
> +-- Record trace with the non-zero result.
> +wrap_ret_bc(ON_TRACE_VALUE)
> +
> +test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')

Side note: Applied the changes we've discussed. The diff is below:

================================================================================

diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
index 36fa450a..e6a0973b 100644
--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
@@ -62,17 +62,17 @@ wrap(ON_TRACE_VALUE)
 
 test:ok(result ~= 0, 'use-def analysis for BC_VARG')
 
--- Now check the same case, but with BC_RET before the BC_VARG,
+-- Now check the same case, but with BC_JMP before the BC_VARG,
 -- so use-def analysis will take early return case for BCMlit.
 -- See `snap_usedef()` in <src/lj_snap.c> for details.
 -- The test checks that slots greater than `numparams` are not
 -- purged.
 local function varg_ret_bc(...)
-  -- XXX: This branch contains BC_RET. See the comment above.
+  -- XXX: This branch contains BC_JMP. See the comment above.
   -- luacheck: ignore
   if false then else end
   local slot = ({...})[1]
-  -- Forcify stitch and usage of vararg slot. Any NIY is OK here.
+  -- Forcify stitch and usage of vararg slot. Any NYI is OK here.
   return fmod(ARG_ON_RECORDING, slot)
 end
 
@@ -88,6 +88,6 @@ wrap_ret_bc(ARG_ON_RECORDING)
 -- Record trace with the non-zero result.
 wrap_ret_bc(ON_TRACE_VALUE)
 
-test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG')
+test:ok(result ~= 0, 'use-def analysis for FUNCV with jump before BC_VARG')
 
 os.exit(test:check() and 0 or 1)

================================================================================

> +
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix use-def analysis for varargs

[Igor Munkin via Tarantool-patches]

Sergey,

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 09.06.23, Sergey Kaplun via Tarantool-patches wrote:
> The first patch in the patchset fixes the problem related to the flaky
> test from tarantool/tarantool#8718. And a really similar issue is fixed
> via the second commit.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-704-bc-varg-use-def
> PR: https://github.com/tarantool/tarantool/pull/8754
> Related Issues:
> * https://github.com/tarantool/tarantool/issues/8516
> * https://github.com/tarantool/tarantool/issues/8718
> * https://github.com/LuaJIT/LuaJIT/issues/704
> 
> Mike Pall (2):
>   Fix use-def analysis for BC_VARG.
>   Fix use-def analysis for vararg functions.
> 
>  src/lj_snap.c                                 | 10 ++-
>  .../lj-704-bc-varg-use-def.test.lua           | 90 +++++++++++++++++++
>  2 files changed, 97 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM