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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for the patch! LGTM, considering the fixes made to resolve the
comments left by Max.

On 22.06.23, Sergey Kaplun wrote:
> 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()`.
> This happens due to 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
> given as argument slot as the destination to store GC value. Also, it
> fixups all usages of this macro to match new semantics.
> 
> 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>

> diff --git a/test/tarantool-c-tests/fix-yield-c-hook.test.c b/test/tarantool-c-tests/fix-yield-c-hook.test.c
> index 9068360e..b84cdc7e 100644
> --- a/test/tarantool-c-tests/fix-yield-c-hook.test.c
> +++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c
> @@ -22,10 +22,6 @@ static void yield(lua_State *L, lua_Debug *ar)
>  		lua_yield(L, 0);
>  }
>  
> -/*
> - * XXX: This test still leads to core dump in the GC64 mode.
> - * This will be fixed in the next commit.
> - */

Side note: I didn't mention this in the previous commit, but I wonder,
why you simply didn't disable the test for GC64 build? Anyway, I guess
we can ignore the fact that tests doesn't work for the previous patch,
since this one is a fix for the fix.

>  static int yield_in_c_hook(void *test_state)
>  {
>  	lua_State *L = test_state;
> -- 
> 2.34.1
> 

-- 
Best regards,
IM