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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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