<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>Please consider my comments below.</div><div> </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_16874444192026193648_BODY">From: Mike Pall <mike><br><br>Reported by Jason Carr.<br><br>(cherry picked from commit dd5032ed844c56964347c7916db66b0eb11d8091)<br><br>When we call `lua_yield()` from the C hook the additional continuation</div></div></div></div></blockquote><div>Typo: s/hook the/hook, an/</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>frame is added. This frame contains a continuation function, PC where we<br>should return, thread GC object to continue, and this frame type and</div></div></div></div></blockquote><div>Typo: s/thread/a thread/</div><div>Typo: s/this frame/the frame/</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>size (see details in <src/lj_frame.h>). For non-GC64 mode, when we set<br>the GC thread on the Lua stack, stack top isn't incremented, so the GC</div></div></div></div></blockquote><div>Typo: s/stack top/the stack top/</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>thread overwrites the PC to return. For the GC64 mode the increment is<br>missing before setting frame type and size.<br><br>This patches fixes the behaviour by adding missing slot incrementing.</div></div></div></div></blockquote><div>Typo: s/patches/patch/</div><div>Typo: s/adding/adding the/</div><div>Typo: s/incrementing/incrementation/</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>Also, it hardens the conditions of using `lj_err_throw()`, according the</div></div></div></div></blockquote><div>Typo: s/according the/according 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>availability of external unwinder.</div></div></div></div></blockquote><div>Side note: should we test that change too?</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>The behaviour for the GC64 mode is still wrong due to miscalculation of</div></div></div></div></blockquote><div>Typo: s/to miscalculation/to a miscalculation/</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>the slot of the GC thread object. This will be fixed in the next<br>commit.<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8516<br>---<br> src/lj_api.c | 5 +-<br> .../fix-yield-c-hook-script.lua | 19 +++++++<br> .../tarantool-c-tests/fix-yield-c-hook.test.c | 53 +++++++++++++++++++<br> 3 files changed, 75 insertions(+), 2 deletions(-)<br> create mode 100644 test/tarantool-c-tests/fix-yield-c-hook-script.lua<br> create mode 100644 test/tarantool-c-tests/fix-yield-c-hook.test.c<br><br>diff --git a/src/lj_api.c b/src/lj_api.c<br>index dccfe62e..89998815 100644<br>--- a/src/lj_api.c<br>+++ b/src/lj_api.c<br>@@ -1242,11 +1242,12 @@ LUA_API int lua_yield(lua_State *L, int nresults)<br> setcont(top, lj_cont_hook);<br> if (LJ_FR2) top++;<br> setframe_pc(top, cframe_pc(cf)-1);<br>- if (LJ_FR2) top++;<br>+ top++;<br> setframe_gc(top, obj2gco(L), LJ_TTHREAD);<br>+ if (LJ_FR2) top++;<br> setframe_ftsz(top, ((char *)(top+1)-(char *)L->base)+FRAME_CONT);<br> L->top = L->base = top+1;<br>-#if LJ_TARGET_X64<br>+#if ((defined(__GNUC__) || defined(__clang__)) && (LJ_TARGET_X64 || defined(LUAJIT_UNWIND_EXTERNAL)) && !LJ_NO_UNWIND) || LJ_TARGET_WINDOWS<br> lj_err_throw(L, LUA_YIELD);<br> #else<br> L->cframe = NULL;<br>diff --git a/test/tarantool-c-tests/fix-yield-c-hook-script.lua b/test/tarantool-c-tests/fix-yield-c-hook-script.lua<br>new file mode 100644<br>index 00000000..124eeb10<br>--- /dev/null<br>+++ b/test/tarantool-c-tests/fix-yield-c-hook-script.lua<br>@@ -0,0 +1,19 @@<br>+-- Auxiliary script to provide Lua functions to be used in the<br>+-- test <fix-yield-c-hook.test.c>.<br>+local M = {}<br>+<br>+-- The function to call, when line hook (calls `lua_yield()`) is<br>+-- set.<br>+M.yield_in_c_hook = function()<br>+ local co = coroutine.create(function()</div></div></div></div></blockquote><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+ -- Just some payload, don't reaaly matter.</div></div></div></div></blockquote><div>Typo: s/reaaly/really/</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>+ local _ = tostring(1)<br>+ end)<br>+ -- Enter coroutine and yield from the 1st line.<br>+ coroutine.resume(co)<br>+ -- Try to get the PC to return and continue to execute the first<br>+ -- line (still will yield from the hook).</div></div></div></div></blockquote><div>Typo: s/still will/it will still/</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>+ coroutine.resume(co)<br>+end<br>+<br>+return M<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>new file mode 100644<br>index 00000000..9068360e<br>--- /dev/null<br>+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c<br>@@ -0,0 +1,53 @@<br>+#include "lua.h"<br>+<br>+#include "test.h"<br>+#include "utils.h"<br>+<br>+#define UNUSED(x) ((void)(x))<br>+<br>+/*<br>+ * This test demonstrates LuaJIT incorrect behaviour, when calling</div></div></div></div></blockquote><div>Typo: s/LuaJIT’s/</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>+ * `lua_yield()` inside C hook.</div></div></div></div></blockquote><div>Typo: s/inside/inside a/</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>+ * See <a href="https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta" target="_blank">https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta</a><br>+ * for details.<br>+ */<br>+<br>+static lua_State *main_L = NULL;<br>+<br>+static void yield(lua_State *L, lua_Debug *ar)<br>+{<br>+ UNUSED(ar);<br>+ /* Wait for the other coroutine and yield. */<br>+ if (L != main_L)<br>+ lua_yield(L, 0);<br>+}<br>+<br>+/*<br>+ * XXX: This test still leads to core dump in the GC64 mode.</div></div></div></div></blockquote><div>Typo: s/to core/to a core/</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 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>+ utils_get_aux_lfunc(L);<br>+ lua_sethook(L, yield, LUA_MASKLINE, 0);<br>+ lua_call(L, 0, 0);<br>+ /* Remove hook. */<br>+ lua_sethook(L, yield, 0, 0);<br>+ return TEST_EXIT_SUCCESS;<br>+}<br>+<br>+int main(void)<br>+{<br>+ lua_State *L = utils_lua_init();<br>+ utils_load_aux_script(L, "fix-yield-c-hook-script.lua");<br>+ main_L = L;<br>+<br>+ const struct test_unit tgroup[] = {<br>+ test_unit_def(yield_in_c_hook)<br>+ };<br>+<br>+ const int test_result = test_run_group(tgroup, L);<br>+ utils_lua_close(L);<br>+ return test_result;<br>+}<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>