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

[Maxim Kokryashkin]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230614/c7eaca5c/attachment.htm>