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

[Maxim Kokryashkin]

Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>I've fixed all typos you mentioned. Branch is force-pushed.
>>
>>On 29.06.23, Maxim Kokryashkin wrote:
>>>
>>> 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/
>>
>>Fixed.
>>
>>> >>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/
>>
>>Fixed.
>>
>>> >>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/
>>
>>Fixed.
>>
>>> >>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/
>>
>>Fixed.
>>
>>> >>Also, it hardens the conditions of using `lj_err_throw()`, according the
>>> >Typo: s/according the/according to the/
>>
>>Fixed.
>>
>>> >>availability of external unwinder.
>>> >Side note: should we test that change too?
>>
>>I suppose not, because we have no exotic builds for them, and they are
>>__really__ exotic (not gcc|clang, or build with disabled external
>>unwinder). Also, build with internal unwinder only is really hard to
>>test, according our tests for exceptions on traces.
>>
>>> >>
>>> >>The behaviour for the GC64 mode is still wrong due to miscalculation of
>>> >Typo: s/to miscalculation/to a miscalculation/
>>
>>Fixed.
>>
>>> >>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
>>
>><snipped>
>>
>>> >>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/
>>
>>Fixed. Thanks!
>>
>>> >>+ 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/
>>
>>Fixed. Thanks!
>>
>>> >>+ 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/
>>
>>Fixed.
>>
>>> >>+ * `lua_yield()` inside C hook.
>>> >Typo: s/inside/inside a/
>>
>>Fixed.
>>
>>> >>+ * 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/
>>
>>Fixed.
>>
>>> >>+ * This will be fixed in the next commit.
>>> >>+ */
>>
>><snipped>
>>
>>See the iterative patch for all typos below.
>>
>>===================================================================
>>diff --git a/test/tarantool-c-tests/fix-yield-c-hook-script.lua b/test/tarantool-c-tests/fix-yield-c-hook-script.lua
>>index 124eeb10..312c7df5 100644
>>--- a/test/tarantool-c-tests/fix-yield-c-hook-script.lua
>>+++ b/test/tarantool-c-tests/fix-yield-c-hook-script.lua
>>@@ -6,13 +6,13 @@ local M = {}
>> -- set.
>> M.yield_in_c_hook = function()
>>   local co = coroutine.create(function()
>>- -- Just some payload, don't reaaly matter.
>>+ -- Just some payload, don't really matter.
>>     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).
>>+ -- line (it will still yield from the hook).
>>   coroutine.resume(co)
>> end
>> 
>>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..a0de28ae 100644
>>--- a/test/tarantool-c-tests/fix-yield-c-hook.test.c
>>+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c
>>@@ -6,8 +6,8 @@
>> #define UNUSED(x) ((void)(x))
>> 
>> /*
>>- * This test demonstrates LuaJIT incorrect behaviour, when calling
>>- * `lua_yield()` inside C hook.
>>+ * This test demonstrates LuaJIT's incorrect behaviour, when
>>+ * calling `lua_yield()` inside a C hook.
>>  * See  https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta
>>  * for details.
>>  */
>>@@ -23,7 +23,7 @@ static void yield(lua_State *L, lua_Debug *ar)
>> }
>> 
>> /*
>>- * XXX: This test still leads to core dump in the GC64 mode.
>>+ * XXX: This test still leads to a core dump in the GC64 mode.
>>  * This will be fixed in the next commit.
>>  */
>> static int yield_in_c_hook(void *test_state)
>>===================================================================
>>
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230630/18bb13a1/attachment.htm>