Re: [Tarantool-patches] [PATCH luajit] Fix predict_next() in parser.

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

Hi, Maxim!
Thanks for the review!
Fixed your comments inline.
Branch is force-pushed.

On 16.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> On Tue, Aug 15, 2023 at 05:25:41PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Reported by Sergey Kaplun.
> > 

<snipped>

> > The `0001 KNIL` is a result of merging two `KPRI` instructions: one for
> > the local variable, one for the slot with `nil` object. During parsing in
> > `predict_next()` the second `MOV` bytecode is examined to set `pairs` or
> > `next` local variable. But, as far as it moves `nil` value, that isn't
> > an actual variable, so it has no the name this leads to the crash.
> Typo: s/variable, so it/variable and/
> Typo: s/the name this/name, that move/

Fixed.

> > 
> > This patch adds the check to be sure that `RD` in the `MOV` bytecode is
> Typo: s/the check/a check/

Fixed.

> > an actual variable.
> Please mention the lj_bc.h here, so it is obvious what `RD` is.

Fixed.

> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#8825
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1033-fix-parsing-predict-next
> > PR: https://github.com/tarantool/tarantool/pull/8987
> > Related issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/1033
> > * https://github.com/tarantool/tarantool/issues/8825
> > 
> >  src/lj_parse.c                                |  1 +
> >  .../lj-1033-fix-parsing-predict-next.test.lua | 30 +++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >  create mode 100644 test/tarantool-tests/lj-1033-fix-parsing-predict-next.test.lua
> > 
> > diff --git a/src/lj_parse.c b/src/lj_parse.c
> > index 3f6caaec..420b95cb 100644
> > --- a/src/lj_parse.c
> > +++ b/src/lj_parse.c

<snipped>

> > +-- The resulting bytecode is the following:
> > +--
> > +-- 0001    KNIL     0   1
> > +-- 0002    MOV      2   1
> > +-- 0003    TGETS    1   1   0  ; "foo"
> > +-- 0004    CALL     1   4   2
> > +--
> > +-- This MOV don't use any variable value from the stack, so the
> Typo: s/don't/doesn't/

Fixed. See the iterative diff below:

===================================================================
diff --git a/test/tarantool-tests/lj-1033-fix-parsing-predict-next.test.lua b/test/tarantool-tests/lj-1033-fix-parsing-predict-next.test.lua
index 624344eb..63998d8c 100644
--- a/test/tarantool-tests/lj-1033-fix-parsing-predict-next.test.lua
+++ b/test/tarantool-tests/lj-1033-fix-parsing-predict-next.test.lua
@@ -14,7 +14,7 @@ local res_f = loadstring([[
 -- 0003    TGETS    1   1   0  ; "foo"
 -- 0004    CALL     1   4   2
 --
--- This MOV don't use any variable value from the stack, so the
+-- This MOV doesn't use any variable value from the stack, so the
 -- attempt to get the name in `predict_next() leads to the crash.
 local _
 for _ in (nil):foo() do end
===================================================================

<snipped>

> > 

-- 
Best regards,
Sergey Kaplun