<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </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_16880373631460365101_BODY">Hi, Maxim!<br>Thanks for the review!<br>I've fixed all typos you mentioned. Branch is force-pushed.<br><br>On 29.06.23, Maxim Kokryashkin wrote:<br>><br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>> <br>> > <br>> >>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<br>> >Typo: s/hook the/hook, an/<br><br>Fixed.<br><br>> >>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<br>> >Typo: s/thread/a thread/<br>> >Typo: s/this frame/the frame/<br><br>Fixed.<br><br>> >>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<br>> >Typo: s/stack top/the stack top/<br><br>Fixed.<br><br>> >>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.<br>> >Typo: s/patches/patch/<br>> >Typo: s/adding/adding the/<br>> >Typo: s/incrementing/incrementation/<br><br>Fixed.<br><br>> >>Also, it hardens the conditions of using `lj_err_throw()`, according the<br>> >Typo: s/according the/according to the/<br><br>Fixed.<br><br>> >>availability of external unwinder.<br>> >Side note: should we test that change too?<br><br>I suppose not, because we have no exotic builds for them, and they are<br>__really__ exotic (not gcc|clang, or build with disabled external<br>unwinder). Also, build with internal unwinder only is really hard to<br>test, according our tests for exceptions on traces.<br><br>> >><br>> >>The behaviour for the GC64 mode is still wrong due to miscalculation of<br>> >Typo: s/to miscalculation/to a miscalculation/<br><br>Fixed.<br><br>> >>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><br><snipped><br><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()<br>> >>+ -- Just some payload, don't reaaly matter.<br>> >Typo: s/reaaly/really/<br><br>Fixed. Thanks!<br><br>> >>+ 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).<br>> >Typo: s/still will/it will still/<br><br>Fixed. Thanks!<br><br>> >>+ 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<br>> >Typo: s/LuaJIT’s/<br><br>Fixed.<br><br>> >>+ * `lua_yield()` inside C hook.<br>> >Typo: s/inside/inside a/<br><br>Fixed.<br><br>> >>+ * 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.<br>> >Typo: s/to core/to a core/<br><br>Fixed.<br><br>> >>+ * This will be fixed in the next commit.<br>> >>+ */<br><br><snipped><br><br>See the iterative patch for all typos below.<br><br>===================================================================<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>index 124eeb10..312c7df5 100644<br>--- a/test/tarantool-c-tests/fix-yield-c-hook-script.lua<br>+++ b/test/tarantool-c-tests/fix-yield-c-hook-script.lua<br>@@ -6,13 +6,13 @@ local M = {}<br> -- set.<br> M.yield_in_c_hook = function()<br> local co = coroutine.create(function()<br>- -- Just some payload, don't reaaly matter.<br>+ -- Just some payload, don't really matter.<br> 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).<br>+ -- line (it will still yield from the hook).<br> coroutine.resume(co)<br> end<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..a0de28ae 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>@@ -6,8 +6,8 @@<br> #define UNUSED(x) ((void)(x))<br> <br> /*<br>- * This test demonstrates LuaJIT incorrect behaviour, when calling<br>- * `lua_yield()` inside C hook.<br>+ * This test demonstrates LuaJIT's incorrect behaviour, when<br>+ * calling `lua_yield()` inside a C hook.<br> * 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>@@ -23,7 +23,7 @@ static void yield(lua_State *L, lua_Debug *ar)<br> }<br> <br> /*<br>- * XXX: This test still leads to core dump in the GC64 mode.<br>+ * XXX: This test still leads to a 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><br>> >Best regards,<br>> >Maxim Kokryashkin<br>> > <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>