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

[Maxim Kokryashkin]

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/
>>.This happens due to tricky usage of the previous slot instead of the
>Typo: s/due to/due to 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
>Typo: s/the given as argument slot/the slot given as argument/
>>fixups all usages of this macro to match new semantics.
>Typo: s/new/the new/
> 
>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.
>>
>>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(-)
>>
>>diff --git a/src/lj_ccallback.c b/src/lj_ccallback.c
>>index 9158c2d3..224b6b94 100644
>>--- a/src/lj_ccallback.c
>>+++ b/src/lj_ccallback.c
>>@@ -533,13 +533,13 @@ static void callback_conv_args(CTState *cts, lua_State *L)
>>   if (LJ_FR2) {
>>     (o++)->u64 = LJ_CONT_FFI_CALLBACK;
>>     (o++)->u64 = rid;
>>- o++;
>>   } else {
>>     o->u32.lo = LJ_CONT_FFI_CALLBACK;
>>     o->u32.hi = rid;
>>     o++;
>>   }
>>   setframe_gc(o, obj2gco(fn), fntp);
>>+ if (LJ_FR2) o++;
>>   setframe_ftsz(o, ((char *)(o+1) - (char *)L->base) + FRAME_CONT);
>>   L->top = L->base = ++o;
>>   if (!ct)
>>diff --git a/src/lj_err.c b/src/lj_err.c
>>index d0223384..c400a9ba 100644
>>--- a/src/lj_err.c
>>+++ b/src/lj_err.c
>>@@ -707,9 +707,9 @@ LJ_NOINLINE void lj_err_optype_call(lua_State *L, TValue *o)
>>   const BCIns *pc = cframe_Lpc(L);
>>   if (((ptrdiff_t)pc & FRAME_TYPE) != FRAME_LUA) {
>>     const char *tname = lj_typename(o);
>>+ setframe_gc(o, obj2gco(L), LJ_TTHREAD);
>>     if (LJ_FR2) o++;
>>     setframe_pc(o, pc);
>>- setframe_gc(o, obj2gco(L), LJ_TTHREAD);
>>     L->top = L->base = o+1;
>>     err_msgv(L, LJ_ERR_BADCALL, tname);
>>   }
>>diff --git a/src/lj_frame.h b/src/lj_frame.h
>>index 1e4adaa3..2bdf3c48 100644
>>--- a/src/lj_frame.h
>>+++ b/src/lj_frame.h
>>@@ -46,7 +46,7 @@ enum {
>> #define frame_gc(f) (gcval((f)-1))
>> #define frame_ftsz(f) ((ptrdiff_t)(f)->ftsz)
>> #define frame_pc(f) ((const BCIns *)frame_ftsz(f))
>>-#define setframe_gc(f, p, tp) (setgcVraw((f)-1, (p), (tp)))
>>+#define setframe_gc(f, p, tp) (setgcVraw((f), (p), (tp)))
>> #define setframe_ftsz(f, sz) ((f)->ftsz = (sz))
>> #define setframe_pc(f, pc) ((f)->ftsz = (int64_t)(intptr_t)(pc))
>> #else
>>diff --git a/src/lj_meta.c b/src/lj_meta.c
>>index 0bd4d842..7ef7a8e0 100644
>>--- a/src/lj_meta.c
>>+++ b/src/lj_meta.c
>>@@ -86,8 +86,8 @@ int lj_meta_tailcall(lua_State *L, cTValue *tv)
>>   else
>>     top->u32.lo = LJ_CONT_TAILCALL;
>>   setframe_pc(top++, pc);
>>- if (LJ_FR2) top++;
>>   setframe_gc(top, obj2gco(L), LJ_TTHREAD); /* Dummy frame object. */
>>+ if (LJ_FR2) top++;
>>   setframe_ftsz(top, ((char *)(top+1) - (char *)base) + FRAME_CONT);
>>   L->base = L->top = top+1;
>>   /*
>>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.
>>- */
>> static int yield_in_c_hook(void *test_state)
>> {
>>  lua_State *L = test_state;
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230629/92f5514d/attachment.htm>