Re: [Tarantool-patches] [PATCH luajit] ARM64: Fix pcall() error case.

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

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


Hi, Sergey!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>
>>On 15.02.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> LGTM, except for a few nits regarding the commit message.
>>
>>Fixed your comments, the new commit message is the following:
>>
>>
>>| ARM64: Fix pcall() error case.
>>|
>>| Reported by Alex Orlenko.
>>|
>>| (cherry picked from commit b4b2dce9fc3ffaaaede39b36d06415311e2aa516)
>>|
>>| The `pcall()` assembler preambule modifies `RC`, which is mapped to
>>| `x28` (the value is N_args * 8), during the check of the amount of the
>>| given arguments. So, this wrong value being used in the `fff_fallback`
>>| routine leads to a crash on error throwing, because the Lua stack is
>>| incorrectly filled and can't be unwound.
>>|
>>| This patch adds the additional comparison before taking the fallback
>>| branch and modifies `RC` only after this branch.
>>|
>>| Sergey Kaplun:
>>| * added the description and the test for the problem
>>|
>>| Part of tarantool/tarantool#8069
>>
>>Branch is force-pushed.
>>
>>>  
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Reported by Alex Orlenko.
>>> >>
>>> >>(cherry picked from commit b4b2dce9fc3ffaaaede39b36d06415311e2aa516)
>>> >>
>>> >>The `pcall()` assembler preambule modifies `RC` (`x28`) (N args * 8)
>>> >The «`RC` (`x28`) (N args * 8)» expression is hard to percieve. I suggest
>>> >reformulating it in a way like «modifies `RC`, which is mapped to `x28`, so
>>> >it has value ...». Feel free to ignore.
>>> >>during the check of the amount of the given arguments. So, this wrong
>>> >>value using in the `fff_fallback` routine leading to a crash on the
>>> >Typo: s/using/being used/
>>> >Typo: s/leading/leads
>>> >Typo: s/on the/on
>>> >>error throwing, because the Lua stack is filled incorrect and can't be
>>> >Typo: s/is filled incorrect/is incorrectly filled/
>>> >>unwound.
>>> >>
>>> >>This patch adds the additional comparison before taking the fallback
>>> >>branch and modifies `RC` only after this branch.
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8069
>>> >>---
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

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