Re: [Tarantool-patches] [PATCH luajit 1/2] Fix maxslots when recording BC_VARG.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]


Hi!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comments! Branch is force-pushed.
>>
>>On 14.07.23, Maxim Kokryashkin wrote:
>>>
>>> Hi!
>>> Thanks for the patch!
>>> LGTM, except for a few nits regarding the commit message.
>>>  
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Analyzed by Sergey Kaplun.
>>> >>
>>> >>(cherry-picked from commit 94ada59628dd6ce5d6d2dad1d35a68ad30127f53)
>>> >>
>>> >>While recording BC_VARG `J->maxslot` isn't shrunk to the effective stack
>>> >Typo: s/shrunk/shrinking
>>
>>I meant the third form shrink/shrank/shrunk here. Ignoring.
>>
>>> >>top. This leads to dead value stored in the JIT slots and the following
>>> >Typo: s/value/values/
>>
>>Fixed.
>>
>>> >>assertion failure for these slots check in `rec_check_slots()`. Note,
>>> >>that `rec_varg()` modifies `maxslot` only under the condition that
>>> >>`maxslot` should be increased, but the dead values are left for the
>>> >>opposite case.
>>> >>
>>> >>This patch removes the condition inside `rec_varg()` only for the case
>>> >>when varargs are not defined on trace (`framedepth` is 0), but the
>>> >>similar issue still occurs for the case when vararg are defined on the
>>> >Typo: s/vararg/varagrs/
>>> >>trace.
>>
>>Fixed.
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8825
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 2437 bytes --]