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

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

Hi, Sergey!
Thanks for the updates!
LGTM, after fixing several minor comments below.

On 29.08.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for review! See my comments.
> 
> New changes were force-pushed.
> 
> 
> On 8/29/23 16:38, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> >
> > On 29.08.23, Sergey Bronnikov wrote:
> >> From:sergeyb@tarantool.org
> >>

<snipped>

> >> In a function `predict_next` variable `exprpc` looks forward and expects
> > Minor: I suggest using of `()` for distinguishing function and variable
> > names.
> > Feel free to ignore.
> 
> Fixed. However "function" was before "predict_next".
> 

Yes, I understand, its just matter of taste :).

<snipped>

> >> Sergey Bronnikov:
> >> * added the description and the test for the problem
> >>
> >> Part of tarantool/tarantool#8825
> >> ---
> >>
> >> PR:https://github.com/tarantool/tarantool/pull/9054
> >> Branch:https://github.com/tarantool/luajit/tree/ligurio/lj-1054-incorrect-pc-value-predict_next
> >> Related issue:
> >> *https://github.com/LuaJIT/LuaJIT/issues/1054
> >>
> >>   src/lj_parse.c                                 |  4 +++-
> >>   ...incorrect-pc-value-in-predict_next.test.lua | 18 ++++++++++++++++++

I suggest to use predict-next instead in filename and testname to be
consistent with other tests.

> >>   2 files changed, 21 insertions(+), 1 deletion(-)
> >>   create mode 100644 test/tarantool-tests/lj-1054-incorrect-pc-value-in-predict_next.test.lua
> >>

<snipped>

> >
> >> +-- Sample executed in LuaJIT instrumented by ASAN leads to
> >> +-- a heap-buffer-overflow.
> > Minor: IDK why, but suggested varian here is "heap buffer overflow".
> 
> 
> ASAN reports error with hyphens, like this:
> 
> |==90673==ERROR: AddressSanitizer: heap-buffer-overflow on address 
> 0x6020000000fb at pc 0x000108868a95 bp 0x7fff573979a0 sp 0x7fff57397998 
> READ of size 1 at 0x6020000000fb thread T0|
> 
> If you don't like variant "heap-buffer-overflow" then we can use variant 
> used in CWE list: "heap-based buffer overflow", see [1].
> 
> What variant should
> 
> 1. https://cwe.mitre.org/data/definitions/122.html

Yes, lets used it.

> 
> >
> >> +-- See alsohttps://github.com/LuaJIT/LuaJIT/issues/528

Nit: Missed dot at the end of the sentence.
Typo: s/528/1054./

<snipped>

-- 
Best regards,
Sergey Kaplun