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

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

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