Re: [Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

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

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


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comments and force-pushed the branch.
>>
>>On 29.06.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> LGTM, except for a few comments and the single note below.
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Reported by Jason Carr.
>>> >>
>>> >>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
>>> >>
>>> >>This commit is the follow up for the previous commit ("Fix lua_yield()
>>> >>from C hook."). In GC64 mode stack slot for a GC thread object is still
>>> >>miscalculated during creating a continuation frame for `lua_yield()`
>>> >Typo: s/during creating/during the creation of/
>>
>>Fixed.
>>
>>> >>.This happens due to tricky usage of the previous slot instead of the
>>> >Typo: s/due to/due to the/
>>
>>Fixed.
>>
>>> >>given one in `setframe_gc()` macro.
>>> >>
>>> >>This patch changes the semantics of `setframe_gc()` macro to use the
>>> >>given as argument slot as the destination to store GC value. Also, it
>>> >Typo: s/the given as argument slot/the slot given as argument/
>>
>>Fixed.
>>
>>> >>fixups all usages of this macro to match new semantics.
>>> >Typo: s/new/the new/
>>
>>Fixed.
>>
>>> > 
>>> >Also, I strongly suggest to distinct the changes that directly relate
>>> >to the issue, from the semantic fixups related to the macro update.
>>> >An additional sentence or a list in the commit message would be enough.
>>
>>I mention all places with changed semantics to avoid misunderstanding.
>>The commit message looks like the following:
>>
>>===================================================================
>>Another fix for lua_yield() from C hook.
>>
>>Reported by Jason Carr.
>>
>>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
>>
>>This commit is the follow up for the previous commit ("Fix lua_yield()
>>from C hook."). In GC64 mode stack slot for a GC thread object is still
>>miscalculated during the creation of a continuation frame for
>>`lua_yield()`. This happens due to the tricky usage of the previous slot
>>instead of the given one in `setframe_gc()` macro.
>>
>>This patch changes the semantics of `setframe_gc()` macro to use the
>>slot given as argument as the destination to store GC value. Also, it
>>fixups all usages of this macro to match the new semantics, i.e. in:
>>* <src/lj_ccallback.c>
>>* <src/lj_err.c>
>>* <src/lj_meta.c>
>>
>>Sergey Kaplun:
>>* added the description for the problem
>>
>>Part of tarantool/tarantool#8516
>>===================================================================
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8516
>>> >>---
>>> >> src/lj_ccallback.c | 2 +-
>>> >> src/lj_err.c | 2 +-
>>> >> src/lj_frame.h | 2 +-
>>> >> src/lj_meta.c | 2 +-
>>> >> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
>>> >> 5 files changed, 4 insertions(+), 8 deletions(-)
>>> >>
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

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