[Tarantool-patches] [PATCH luajit 0/2] Fix lua_yield from the C hook.

[Tarantool-patches] [PATCH luajit 0/2] Fix lua_yield from the C hook.

[Sergey Kaplun via Tarantool-patches]

This patchset fixes the behaviour of the `lua_yield()` invocation inside
C hooks. The first patch fixes the behaviour for non-GC64 mode, but the
GC64 mode is still failing with a core dump. The second patch fixes
behaviour for the GC64 mode as well.

This patchset is based on the our C tests to be introduced, but their
patch set has 2 LGTMs, so I just checkout this branch from the branch
with C tests.

PR: https://github.com/tarantool/tarantool/pull/8804
Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-yield-c-hook
Related issue: https://github.com/tarantool/tarantool/issues/8516
Mail list: https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta

Mike Pall (2):
  Fix lua_yield() from C hook.
  Another fix for lua_yield() from C hook.

 src/lj_api.c                                  |  5 +-
 src/lj_ccallback.c                            |  2 +-
 src/lj_err.c                                  |  2 +-
 src/lj_frame.h                                |  2 +-
 src/lj_meta.c                                 |  2 +-
 .../fix-yield-c-hook-script.lua               | 19 +++++++
 .../tarantool-c-tests/fix-yield-c-hook.test.c | 49 +++++++++++++++++++
 7 files changed, 75 insertions(+), 6 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

-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

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
frame is added. This frame contains a continuation function, PC where we
should return, thread GC object to continue, and this frame type and
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
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.
Also, it hardens the conditions of using `lj_err_throw()`, according the
availability of external unwinder.

The behaviour for the GC64 mode is still wrong due to miscalculation of
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.
+    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).
+  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
+ * `lua_yield()` inside C hook.
+ * 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.
+ * 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

[Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Jason Carr.

(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)

This commit is the follow up for the previous commit ("Fix lua_yield()
from C hook."). In GC64 mode stack slot for a GC thread object is still
miscalculated during creating a continuation frame for `lua_yield()`.
This happens due to tricky usage of the previous slot instead of the
given one in `setframe_gc()` macro.

This patch changes the semantics of `setframe_gc()` macro to use the
given as argument slot as the destination to store GC value. Also, it
fixups all usages of this macro to match new semantics.

Sergey Kaplun:
* added the description for the problem

Part of tarantool/tarantool#8516
---
 src/lj_ccallback.c                             | 2 +-
 src/lj_err.c                                   | 2 +-
 src/lj_frame.h                                 | 2 +-
 src/lj_meta.c                                  | 2 +-
 test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
 5 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/lj_ccallback.c b/src/lj_ccallback.c
index 9158c2d3..224b6b94 100644
--- a/src/lj_ccallback.c
+++ b/src/lj_ccallback.c
@@ -533,13 +533,13 @@ static void callback_conv_args(CTState *cts, lua_State *L)
   if (LJ_FR2) {
     (o++)->u64 = LJ_CONT_FFI_CALLBACK;
     (o++)->u64 = rid;
-    o++;
   } else {
     o->u32.lo = LJ_CONT_FFI_CALLBACK;
     o->u32.hi = rid;
     o++;
   }
   setframe_gc(o, obj2gco(fn), fntp);
+  if (LJ_FR2) o++;
   setframe_ftsz(o, ((char *)(o+1) - (char *)L->base) + FRAME_CONT);
   L->top = L->base = ++o;
   if (!ct)
diff --git a/src/lj_err.c b/src/lj_err.c
index d0223384..c400a9ba 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -707,9 +707,9 @@ LJ_NOINLINE void lj_err_optype_call(lua_State *L, TValue *o)
   const BCIns *pc = cframe_Lpc(L);
   if (((ptrdiff_t)pc & FRAME_TYPE) != FRAME_LUA) {
     const char *tname = lj_typename(o);
+    setframe_gc(o, obj2gco(L), LJ_TTHREAD);
     if (LJ_FR2) o++;
     setframe_pc(o, pc);
-    setframe_gc(o, obj2gco(L), LJ_TTHREAD);
     L->top = L->base = o+1;
     err_msgv(L, LJ_ERR_BADCALL, tname);
   }
diff --git a/src/lj_frame.h b/src/lj_frame.h
index 1e4adaa3..2bdf3c48 100644
--- a/src/lj_frame.h
+++ b/src/lj_frame.h
@@ -46,7 +46,7 @@ enum {
 #define frame_gc(f)		(gcval((f)-1))
 #define frame_ftsz(f)		((ptrdiff_t)(f)->ftsz)
 #define frame_pc(f)		((const BCIns *)frame_ftsz(f))
-#define setframe_gc(f, p, tp)	(setgcVraw((f)-1, (p), (tp)))
+#define setframe_gc(f, p, tp)	(setgcVraw((f), (p), (tp)))
 #define setframe_ftsz(f, sz)	((f)->ftsz = (sz))
 #define setframe_pc(f, pc)	((f)->ftsz = (int64_t)(intptr_t)(pc))
 #else
diff --git a/src/lj_meta.c b/src/lj_meta.c
index 0bd4d842..7ef7a8e0 100644
--- a/src/lj_meta.c
+++ b/src/lj_meta.c
@@ -86,8 +86,8 @@ int lj_meta_tailcall(lua_State *L, cTValue *tv)
   else
     top->u32.lo = LJ_CONT_TAILCALL;
   setframe_pc(top++, pc);
-  if (LJ_FR2) top++;
   setframe_gc(top, obj2gco(L), LJ_TTHREAD);  /* Dummy frame object. */
+  if (LJ_FR2) top++;
   setframe_ftsz(top, ((char *)(top+1) - (char *)base) + FRAME_CONT);
   L->base = L->top = top+1;
   /*
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..b84cdc7e 100644
--- a/test/tarantool-c-tests/fix-yield-c-hook.test.c
+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c
@@ -22,10 +22,6 @@ static void yield(lua_State *L, lua_Debug *ar)
 		lua_yield(L, 0);
 }
 
-/*
- * XXX: This test still leads to core dump in the GC64 mode.
- * This will be fixed in the next commit.
- */
 static int yield_in_c_hook(void *test_state)
 {
 	lua_State *L = test_state;
-- 
2.34.1

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5259 bytes --]


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
> 

[-- Attachment #2: Type: text/html, Size: 8601 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4372 bytes --]


Hi, Sergey!
LGTM, except for a few comments and the single note below.
> 
>>From: Mike Pall <mike>
>>
>>Reported by Jason Carr.
>>
>>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
>>
>>This commit is the follow up for the previous commit ("Fix lua_yield()
>>from C hook."). In GC64 mode stack slot for a GC thread object is still
>>miscalculated during creating a continuation frame for `lua_yield()`
>Typo: s/during creating/during the creation of/
>>.This happens due to tricky usage of the previous slot instead of the
>Typo: s/due to/due to the/
>>given one in `setframe_gc()` macro.
>>
>>This patch changes the semantics of `setframe_gc()` macro to use the
>>given as argument slot as the destination to store GC value. Also, it
>Typo: s/the given as argument slot/the slot given as argument/
>>fixups all usages of this macro to match new semantics.
>Typo: s/new/the new/
> 
>Also, I strongly suggest to distinct the changes that directly relate
>to the issue, from the semantic fixups related to the macro update.
>An additional sentence or a list in the commit message would be enough.
>>
>>Sergey Kaplun:
>>* added the description for the problem
>>
>>Part of tarantool/tarantool#8516
>>---
>> src/lj_ccallback.c | 2 +-
>> src/lj_err.c | 2 +-
>> src/lj_frame.h | 2 +-
>> src/lj_meta.c | 2 +-
>> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
>> 5 files changed, 4 insertions(+), 8 deletions(-)
>>
>>diff --git a/src/lj_ccallback.c b/src/lj_ccallback.c
>>index 9158c2d3..224b6b94 100644
>>--- a/src/lj_ccallback.c
>>+++ b/src/lj_ccallback.c
>>@@ -533,13 +533,13 @@ static void callback_conv_args(CTState *cts, lua_State *L)
>>   if (LJ_FR2) {
>>     (o++)->u64 = LJ_CONT_FFI_CALLBACK;
>>     (o++)->u64 = rid;
>>- o++;
>>   } else {
>>     o->u32.lo = LJ_CONT_FFI_CALLBACK;
>>     o->u32.hi = rid;
>>     o++;
>>   }
>>   setframe_gc(o, obj2gco(fn), fntp);
>>+ if (LJ_FR2) o++;
>>   setframe_ftsz(o, ((char *)(o+1) - (char *)L->base) + FRAME_CONT);
>>   L->top = L->base = ++o;
>>   if (!ct)
>>diff --git a/src/lj_err.c b/src/lj_err.c
>>index d0223384..c400a9ba 100644
>>--- a/src/lj_err.c
>>+++ b/src/lj_err.c
>>@@ -707,9 +707,9 @@ LJ_NOINLINE void lj_err_optype_call(lua_State *L, TValue *o)
>>   const BCIns *pc = cframe_Lpc(L);
>>   if (((ptrdiff_t)pc & FRAME_TYPE) != FRAME_LUA) {
>>     const char *tname = lj_typename(o);
>>+ setframe_gc(o, obj2gco(L), LJ_TTHREAD);
>>     if (LJ_FR2) o++;
>>     setframe_pc(o, pc);
>>- setframe_gc(o, obj2gco(L), LJ_TTHREAD);
>>     L->top = L->base = o+1;
>>     err_msgv(L, LJ_ERR_BADCALL, tname);
>>   }
>>diff --git a/src/lj_frame.h b/src/lj_frame.h
>>index 1e4adaa3..2bdf3c48 100644
>>--- a/src/lj_frame.h
>>+++ b/src/lj_frame.h
>>@@ -46,7 +46,7 @@ enum {
>> #define frame_gc(f) (gcval((f)-1))
>> #define frame_ftsz(f) ((ptrdiff_t)(f)->ftsz)
>> #define frame_pc(f) ((const BCIns *)frame_ftsz(f))
>>-#define setframe_gc(f, p, tp) (setgcVraw((f)-1, (p), (tp)))
>>+#define setframe_gc(f, p, tp) (setgcVraw((f), (p), (tp)))
>> #define setframe_ftsz(f, sz) ((f)->ftsz = (sz))
>> #define setframe_pc(f, pc) ((f)->ftsz = (int64_t)(intptr_t)(pc))
>> #else
>>diff --git a/src/lj_meta.c b/src/lj_meta.c
>>index 0bd4d842..7ef7a8e0 100644
>>--- a/src/lj_meta.c
>>+++ b/src/lj_meta.c
>>@@ -86,8 +86,8 @@ int lj_meta_tailcall(lua_State *L, cTValue *tv)
>>   else
>>     top->u32.lo = LJ_CONT_TAILCALL;
>>   setframe_pc(top++, pc);
>>- if (LJ_FR2) top++;
>>   setframe_gc(top, obj2gco(L), LJ_TTHREAD); /* Dummy frame object. */
>>+ if (LJ_FR2) top++;
>>   setframe_ftsz(top, ((char *)(top+1) - (char *)base) + FRAME_CONT);
>>   L->base = L->top = top+1;
>>   /*
>>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..b84cdc7e 100644
>>--- a/test/tarantool-c-tests/fix-yield-c-hook.test.c
>>+++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c
>>@@ -22,10 +22,6 @@ static void yield(lua_State *L, lua_Debug *ar)
>>  lua_yield(L, 0);
>> }
>> 
>>-/*
>>- * XXX: This test still leads to core dump in the GC64 mode.
>>- * This will be fixed in the next commit.
>>- */
>> static int yield_in_c_hook(void *test_state)
>> {
>>  lua_State *L = test_state;
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
> 

[-- Attachment #2: Type: text/html, Size: 6192 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 29.06.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> LGTM, except for a few comments and the single note below.
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Jason Carr.
> >>
> >>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
> >>
> >>This commit is the follow up for the previous commit ("Fix lua_yield()
> >>from C hook."). In GC64 mode stack slot for a GC thread object is still
> >>miscalculated during creating a continuation frame for `lua_yield()`
> >Typo: s/during creating/during the creation of/

Fixed.

> >>.This happens due to tricky usage of the previous slot instead of the
> >Typo: s/due to/due to the/

Fixed.

> >>given one in `setframe_gc()` macro.
> >>
> >>This patch changes the semantics of `setframe_gc()` macro to use the
> >>given as argument slot as the destination to store GC value. Also, it
> >Typo: s/the given as argument slot/the slot given as argument/

Fixed.

> >>fixups all usages of this macro to match new semantics.
> >Typo: s/new/the new/

Fixed.

> > 
> >Also, I strongly suggest to distinct the changes that directly relate
> >to the issue, from the semantic fixups related to the macro update.
> >An additional sentence or a list in the commit message would be enough.

I mention all places with changed semantics to avoid misunderstanding.
The commit message looks like the following:

===================================================================
Another fix for lua_yield() from C hook.

Reported by Jason Carr.

(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)

This commit is the follow up for the previous commit ("Fix lua_yield()
from C hook."). In GC64 mode stack slot for a GC thread object is still
miscalculated during the creation of a continuation frame for
`lua_yield()`. This happens due to the tricky usage of the previous slot
instead of the given one in `setframe_gc()` macro.

This patch changes the semantics of `setframe_gc()` macro to use the
slot given as argument as the destination to store GC value. Also, it
fixups all usages of this macro to match the new semantics, i.e. in:
* <src/lj_ccallback.c>
* <src/lj_err.c>
* <src/lj_meta.c>

Sergey Kaplun:
* added the description for the problem

Part of tarantool/tarantool#8516
===================================================================

> >>
> >>Sergey Kaplun:
> >>* added the description for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>---
> >> src/lj_ccallback.c | 2 +-
> >> src/lj_err.c | 2 +-
> >> src/lj_frame.h | 2 +-
> >> src/lj_meta.c | 2 +-
> >> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
> >> 5 files changed, 4 insertions(+), 8 deletions(-)
> >>

<snipped>

> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 7186 bytes --]


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
> 

[-- Attachment #2: Type: text/html, Size: 9429 bytes --]

Re: [Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Maxim!
>>Thanks for the review!
>>Fixed your comments and force-pushed the branch.
>>
>>On 29.06.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> LGTM, except for a few comments and the single note below.
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Reported by Jason Carr.
>>> >>
>>> >>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
>>> >>
>>> >>This commit is the follow up for the previous commit ("Fix lua_yield()
>>> >>from C hook."). In GC64 mode stack slot for a GC thread object is still
>>> >>miscalculated during creating a continuation frame for `lua_yield()`
>>> >Typo: s/during creating/during the creation of/
>>
>>Fixed.
>>
>>> >>.This happens due to tricky usage of the previous slot instead of the
>>> >Typo: s/due to/due to the/
>>
>>Fixed.
>>
>>> >>given one in `setframe_gc()` macro.
>>> >>
>>> >>This patch changes the semantics of `setframe_gc()` macro to use the
>>> >>given as argument slot as the destination to store GC value. Also, it
>>> >Typo: s/the given as argument slot/the slot given as argument/
>>
>>Fixed.
>>
>>> >>fixups all usages of this macro to match new semantics.
>>> >Typo: s/new/the new/
>>
>>Fixed.
>>
>>> > 
>>> >Also, I strongly suggest to distinct the changes that directly relate
>>> >to the issue, from the semantic fixups related to the macro update.
>>> >An additional sentence or a list in the commit message would be enough.
>>
>>I mention all places with changed semantics to avoid misunderstanding.
>>The commit message looks like the following:
>>
>>===================================================================
>>Another fix for lua_yield() from C hook.
>>
>>Reported by Jason Carr.
>>
>>(cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
>>
>>This commit is the follow up for the previous commit ("Fix lua_yield()
>>from C hook."). In GC64 mode stack slot for a GC thread object is still
>>miscalculated during the creation of a continuation frame for
>>`lua_yield()`. This happens due to the tricky usage of the previous slot
>>instead of the given one in `setframe_gc()` macro.
>>
>>This patch changes the semantics of `setframe_gc()` macro to use the
>>slot given as argument as the destination to store GC value. Also, it
>>fixups all usages of this macro to match the new semantics, i.e. in:
>>* <src/lj_ccallback.c>
>>* <src/lj_err.c>
>>* <src/lj_meta.c>
>>
>>Sergey Kaplun:
>>* added the description for the problem
>>
>>Part of tarantool/tarantool#8516
>>===================================================================
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8516
>>> >>---
>>> >> src/lj_ccallback.c | 2 +-
>>> >> src/lj_err.c | 2 +-
>>> >> src/lj_frame.h | 2 +-
>>> >> src/lj_meta.c | 2 +-
>>> >> test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
>>> >> 5 files changed, 4 insertions(+), 8 deletions(-)
>>> >>
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 4172 bytes --]

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, considering the fixes for the comments left
by Max.

On 22.06.23, Sergey Kaplun wrote:
> 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
> frame is added. This frame contains a continuation function, PC where we
> should return, thread GC object to continue, and this frame type and
> 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
> 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.
> Also, it hardens the conditions of using `lj_err_throw()`, according the
> availability of external unwinder.
> 
> The behaviour for the GC64 mode is still wrong due to miscalculation of
> 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
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] Another fix for lua_yield() from C hook.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, considering the fixes made to resolve the
comments left by Max.

On 22.06.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Jason Carr.
> 
> (cherry picked from commit dd0f09f95f36caf1f2111c10fec02748116003bb)
> 
> This commit is the follow up for the previous commit ("Fix lua_yield()
> from C hook."). In GC64 mode stack slot for a GC thread object is still
> miscalculated during creating a continuation frame for `lua_yield()`.
> This happens due to tricky usage of the previous slot instead of the
> given one in `setframe_gc()` macro.
> 
> This patch changes the semantics of `setframe_gc()` macro to use the
> given as argument slot as the destination to store GC value. Also, it
> fixups all usages of this macro to match new semantics.
> 
> Sergey Kaplun:
> * added the description for the problem
> 
> Part of tarantool/tarantool#8516
> ---
>  src/lj_ccallback.c                             | 2 +-
>  src/lj_err.c                                   | 2 +-
>  src/lj_frame.h                                 | 2 +-
>  src/lj_meta.c                                  | 2 +-
>  test/tarantool-c-tests/fix-yield-c-hook.test.c | 4 ----
>  5 files changed, 4 insertions(+), 8 deletions(-)
> 

<snipped>

> 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..b84cdc7e 100644
> --- a/test/tarantool-c-tests/fix-yield-c-hook.test.c
> +++ b/test/tarantool-c-tests/fix-yield-c-hook.test.c
> @@ -22,10 +22,6 @@ static void yield(lua_State *L, lua_Debug *ar)
>  		lua_yield(L, 0);
>  }
>  
> -/*
> - * XXX: This test still leads to core dump in the GC64 mode.
> - * This will be fixed in the next commit.
> - */

Side note: I didn't mention this in the previous commit, but I wonder,
why you simply didn't disable the test for GC64 build? Anyway, I guess
we can ignore the fact that tests doesn't work for the previous patch,
since this one is a fix for the fix.

>  static int yield_in_c_hook(void *test_state)
>  {
>  	lua_State *L = test_state;
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix lua_yield from the C hook.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 22.06.23, Sergey Kaplun wrote:
> This patchset fixes the behaviour of the `lua_yield()` invocation inside
> C hooks. The first patch fixes the behaviour for non-GC64 mode, but the
> GC64 mode is still failing with a core dump. The second patch fixes
> behaviour for the GC64 mode as well.
> 
> This patchset is based on the our C tests to be introduced, but their
> patch set has 2 LGTMs, so I just checkout this branch from the branch
> with C tests.
> 
> PR: https://github.com/tarantool/tarantool/pull/8804
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-yield-c-hook
> Related issue: https://github.com/tarantool/tarantool/issues/8516
> Mail list: https://www.freelists.org/post/luajit/BUG-Unable-to-yield-in-a-debug-hook-in-latest-21-beta
> 
> Mike Pall (2):
>   Fix lua_yield() from C hook.
>   Another fix for lua_yield() from C hook.
> 
>  src/lj_api.c                                  |  5 +-
>  src/lj_ccallback.c                            |  2 +-
>  src/lj_err.c                                  |  2 +-
>  src/lj_frame.h                                |  2 +-
>  src/lj_meta.c                                 |  2 +-
>  .../fix-yield-c-hook-script.lua               | 19 +++++++
>  .../tarantool-c-tests/fix-yield-c-hook.test.c | 49 +++++++++++++++++++
>  7 files changed, 75 insertions(+), 6 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
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM