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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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