[Tarantool-patches] [PATCH luajit 1/2] Fix lua_yield() from C hook.

[Maxim Kokryashkin]

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