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

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

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