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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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