<HTML><BODY><div>Hi, Sergey!</div><div>LGTM, except for a few comments and the single note below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16874444200081822669_BODY">From: Mike Pall <mike><br><br>Reported by Jason Carr.<br><br>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)<br><br>This commit is the follow up for the previous commit ("Fix lua_yield()<br>from C hook."). In GC64 mode stack slot for a GC thread object is still<br>miscalculated during creating a continuation frame for `lua_yield()`</div></div></div></div></blockquote><div>Typo: s/during creating/during the creation of/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>.This happens due to tricky usage of the previous slot instead of the</div></div></div></div></blockquote><div>Typo: s/due to/due to the/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>given one in `setframe_gc()` macro.<br><br>This patch changes the semantics of `setframe_gc()` macro to use the<br>given as argument slot as the destination to store GC value. Also, it</div></div></div></div></blockquote><div>Typo: s/the given as argument slot/the slot given as argument/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>fixups all usages of this macro to match new semantics.</div></div></div></div></blockquote><div>Typo: s/new/the new/</div><div> </div><div>Also, I strongly suggest to distinct the changes that directly relate</div><div>to the issue, from the semantic fixups related to the macro update.</div><div>An additional sentence or a list in the commit message would be enough.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>Sergey Kaplun:<br>* added the description for the problem<br><br>Part of tarantool/tarantool#8516<br>---<br> src/lj_ccallback.c | 2 +-<br> src/lj_err.c | 2 +-<br> src/lj_frame.h | 2 +-<br> src/lj_meta.c | 2 +-<br> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----<br> 5 files changed, 4 insertions(+), 8 deletions(-)<br><br>diff --git a/src/lj_ccallback.c b/src/lj_ccallback.c<br>index 9158c2d3..224b6b94 100644<br>--- a/src/lj_ccallback.c<br>+++ b/src/lj_ccallback.c<br>@@ -533,13 +533,13 @@ static void callback_conv_args(CTState *cts, lua_State *L)<br> if (LJ_FR2) {<br> (o++)->u64 = LJ_CONT_FFI_CALLBACK;<br> (o++)->u64 = rid;<br>- o++;<br> } else {<br> o->u32.lo = LJ_CONT_FFI_CALLBACK;<br> o->u32.hi = rid;<br> o++;<br> }<br> setframe_gc(o, obj2gco(fn), fntp);<br>+ if (LJ_FR2) o++;<br> setframe_ftsz(o, ((char *)(o+1) - (char *)L->base) + FRAME_CONT);<br> L->top = L->base = ++o;<br> if (!ct)<br>diff --git a/src/lj_err.c b/src/lj_err.c<br>index d0223384..c400a9ba 100644<br>--- a/src/lj_err.c<br>+++ b/src/lj_err.c<br>@@ -707,9 +707,9 @@ LJ_NOINLINE void lj_err_optype_call(lua_State *L, TValue *o)<br> const BCIns *pc = cframe_Lpc(L);<br> if (((ptrdiff_t)pc & FRAME_TYPE) != FRAME_LUA) {<br> const char *tname = lj_typename(o);<br>+ setframe_gc(o, obj2gco(L), LJ_TTHREAD);<br> if (LJ_FR2) o++;<br> setframe_pc(o, pc);<br>- setframe_gc(o, obj2gco(L), LJ_TTHREAD);<br> L->top = L->base = o+1;<br> err_msgv(L, LJ_ERR_BADCALL, tname);<br> }<br>diff --git a/src/lj_frame.h b/src/lj_frame.h<br>index 1e4adaa3..2bdf3c48 100644<br>--- a/src/lj_frame.h<br>+++ b/src/lj_frame.h<br>@@ -46,7 +46,7 @@ enum {<br> #define frame_gc(f) (gcval((f)-1))<br> #define frame_ftsz(f) ((ptrdiff_t)(f)->ftsz)<br> #define frame_pc(f) ((const BCIns *)frame_ftsz(f))<br>-#define setframe_gc(f, p, tp) (setgcVraw((f)-1, (p), (tp)))<br>+#define setframe_gc(f, p, tp) (setgcVraw((f), (p), (tp)))<br> #define setframe_ftsz(f, sz) ((f)->ftsz = (sz))<br> #define setframe_pc(f, pc) ((f)->ftsz = (int64_t)(intptr_t)(pc))<br> #else<br>diff --git a/src/lj_meta.c b/src/lj_meta.c<br>index 0bd4d842..7ef7a8e0 100644<br>--- a/src/lj_meta.c<br>+++ b/src/lj_meta.c<br>@@ -86,8 +86,8 @@ int lj_meta_tailcall(lua_State *L, cTValue *tv)<br> else<br> top->u32.lo = LJ_CONT_TAILCALL;<br> setframe_pc(top++, pc);<br>- if (LJ_FR2) top++;<br> setframe_gc(top, obj2gco(L), LJ_TTHREAD); /* Dummy frame object. */<br>+ if (LJ_FR2) top++;<br> setframe_ftsz(top, ((char *)(top+1) - (char *)base) + FRAME_CONT);<br> L->base = L->top = top+1;<br> /*<br>diff --git a/test/tarantool-c-tests/fix-yield-c-hook.test.c b/test/tarantool-c-tests/fix-yield-c-hook.test.c<br>index 9068360e..b84cdc7e 100644<br>--- a/test/tarantool-c-tests/fix-yield-c-hook.test.c<br>+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c<br>@@ -22,10 +22,6 @@ static void yield(lua_State *L, lua_Debug *ar)<br> lua_yield(L, 0);<br> }<br> <br>-/*<br>- * XXX: This test still leads to core dump in the GC64 mode.<br>- * This will be fixed in the next commit.<br>- */<br> static int yield_in_c_hook(void *test_state)<br> {<br> lua_State *L = test_state;<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>