[Tarantool-patches] [PATCH luajit v2 0/5] Fix error-throwing on an incorrect coroutine

[Tarantool-patches] [PATCH luajit v2 0/5] Fix error-throwing on an incorrect coroutine

[Maxim Kokryashkin via Tarantool-patches]

Changes in v2:
- Fixed comments as per offline talk with Igor:
    * Reverted one more patch for ARM
    ('arm64: fix cur_L restoration on error throw').
    * Added test from the original issue LuaJIT/LuaJIT#1066

Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6323-fix-curL
PR: https://github.com/tarantool/tarantool/pull/9168

Maxim Kokryashkin (4):
  Revert "Fix cur_L tracking on exceptional path"
  Revert "Update cur_L on exceptional path"
  Revert "arm64: fix cur_L restoration on error throw"
  Revert "Update cur_L on exceptional path (arm)"

Mike Pall (1):
  Restore cur_L for specific Lua/C API use case.

 src/lj_err.c                                  |  5 ++-
 src/vm_arm.dasc                               |  2 --
 src/vm_arm64.dasc                             |  2 --
 src/vm_x64.dasc                               |  8 ++---
 src/vm_x86.dasc                               |  8 ++---
 test/tarantool-tests/CMakeLists.txt           |  1 +
 ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
 .../CMakeLists.txt                            |  1 +
 .../libcur_L_coroutine.c                      | 22 +++++++++++++
 9 files changed, 66 insertions(+), 15 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c

-- 
2.42.0

[Tarantool-patches] [PATCH luajit v2 1/5] Revert "Fix cur_L tracking on exceptional path"

[Maxim Kokryashkin via Tarantool-patches]

This reverts commit 97699d9ee2467389b6aea21a098e38aff3469b5f.

As was mentioned in tarantool/tarantool#6189, throwing an error
not on the currently executed coroutine is a violation of the
Lua/C API. This patch is a part of the patchset that supports
this violation and is reverted because of it.

Part of tarantool/tarantool#6323
---
 src/vm_x64.dasc | 9 ++++-----
 src/vm_x86.dasc | 8 ++++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 09bf67e5..116716ac 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -533,11 +533,11 @@ static void build_subroutines(BuildCtx *ctx)
   |  mov eax, CARG2d			// Error return status for vm_pcall.
   |  mov rsp, CARG1
   |->vm_unwind_c_eh:			// Landing pad for external unwinder.
-  |  mov L:DISPATCH, SAVE_L
-  |  mov GL:RB, L:DISPATCH->glref
-  |  mov GL:RB->cur_L, L:DISPATCH
+  |  mov L:RB, SAVE_L
+  |  mov GL:RB, L:RB->glref
+  |  mov [GL:RB->cur_L], L:RB
   |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
-  |  mov DISPATCH, L:DISPATCH->glref	// Setup pointer to dispatch table.
+  |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
   |  add DISPATCH, GG_G2DISP
   |  jmp ->vm_leave_unw
   |
@@ -561,7 +561,6 @@ static void build_subroutines(BuildCtx *ctx)
   |  add DISPATCH, GG_G2DISP
   |  mov PC, [BASE-8]			// Fetch PC of previous frame.
   |  mov_false RA
-  |  mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
   |  mov RB, [BASE]
   |  mov [BASE-16], RA			// Prepend false to error message.
   |  mov [BASE-8], RB
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index f16ade1a..e3fbf751 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -681,11 +681,11 @@ static void build_subroutines(BuildCtx *ctx)
   |.endif
   |.endif
   |->vm_unwind_c_eh:			// Landing pad for external unwinder.
-  |  mov L:DISPATCH, SAVE_L
-  |  mov GL:RB, L:DISPATCH->glref
-  |  mov dword GL:RB->cur_L, L:DISPATCH
+  |  mov L:RB, SAVE_L
+  |  mov GL:RB, L:RB->glref
+  |  mov dword GL:RB->cur_L, L:RB
   |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
-  |  mov DISPATCH, L:DISPATCH->glref	// Setup pointer to dispatch table.
+  |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
   |  add DISPATCH, GG_G2DISP
   |  jmp ->vm_leave_unw
   |
-- 
2.42.0

[Tarantool-patches] [PATCH luajit v2 2/5] Revert "Update cur_L on exceptional path"

[Maxim Kokryashkin via Tarantool-patches]

This reverts commit ed412cd9f55fe87fd32a69c86e1732690fc5c1b0.

As was mentioned in tarantool/tarantool#6189, throwing an error
not on the currently executed coroutine is a violation of the
Lua/C API. This patch is a part of the patchset that supports
this violation and is reverted because of it.

Part of tarantool/tarantool#6323
---
 src/vm_x64.dasc | 1 -
 src/vm_x86.dasc | 2 --
 2 files changed, 3 deletions(-)

diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 116716ac..399dfcbf 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -535,7 +535,6 @@ static void build_subroutines(BuildCtx *ctx)
   |->vm_unwind_c_eh:			// Landing pad for external unwinder.
   |  mov L:RB, SAVE_L
   |  mov GL:RB, L:RB->glref
-  |  mov [GL:RB->cur_L], L:RB
   |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
   |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
   |  add DISPATCH, GG_G2DISP
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index e3fbf751..9fa9a3f7 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -683,7 +683,6 @@ static void build_subroutines(BuildCtx *ctx)
   |->vm_unwind_c_eh:			// Landing pad for external unwinder.
   |  mov L:RB, SAVE_L
   |  mov GL:RB, L:RB->glref
-  |  mov dword GL:RB->cur_L, L:RB
   |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
   |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
   |  add DISPATCH, GG_G2DISP
@@ -719,7 +718,6 @@ static void build_subroutines(BuildCtx *ctx)
   |  add DISPATCH, GG_G2DISP
   |  mov PC, [BASE-4]			// Fetch PC of previous frame.
   |  mov dword [BASE-4], LJ_TFALSE	// Prepend false to error message.
-  |  mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
   |  // INTERP until jump to BC_RET* or return to C.
   |  set_vmstate INTERP
   |  jmp ->vm_returnc			// Increments RD/MULTRES and returns.
-- 
2.42.0

[Tarantool-patches] [PATCH luajit v2 3/5] Revert "arm64: fix cur_L restoration on error throw"

[Maxim Kokryashkin via Tarantool-patches]

This reverts commit 7570ff61138fb50a17ed78ee28dde0acbbf6131e.

As was mentioned in tarantool/tarantool#6189, throwing an error
not on the currently executed coroutine is a violation of the
Lua/C API. This patch is a part of the patchset that supports
this violation and is reverted because of it.

Part of tarantool/tarantool#6323
---
 src/vm_arm64.dasc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index de33bde4..0a12b5b7 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -395,7 +395,6 @@ static void build_subroutines(BuildCtx *ctx)
   |   mv_vmstate TMP0w, CFUNC
   |  ldr GL, L->glref
   |   st_vmstate TMP0w
-  |  str L, GL->cur_L
   |  b ->vm_leave_unw
   |
   |->vm_unwind_ff:			// Unwind C stack, return from ff pcall.
@@ -411,7 +410,6 @@ static void build_subroutines(BuildCtx *ctx)
   |   ldr GL, L->glref			// Setup pointer to global state.
   |    mov_false TMP0
   |  sub RA, BASE, #8			// Results start at BASE-8.
-  |   str L, GL->cur_L
   |  ldr PC, [BASE, FRAME_PC]		// Fetch PC of previous frame.
   |    str TMP0, [BASE, #-8]		// Prepend false to error message.
   |   st_vmstate ST_INTERP
-- 
2.42.0

[Tarantool-patches] [PATCH luajit v2 4/5] Revert "Update cur_L on exceptional path (arm)"

[Maxim Kokryashkin via Tarantool-patches]

This reverts commit 5ccd25d740476a37d414733b5192d5be0ef06173.

As was mentioned in tarantool/tarantool#6189, throwing an error
not on the currently executed coroutine is a violation of the
Lua/C API. This patch is a part of the patchset that supports
this violation and is reverted because of it.

Part of tarantool/tarantool#6323
---
 src/vm_arm.dasc | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
index 767d31f9..7095e660 100644
--- a/src/vm_arm.dasc
+++ b/src/vm_arm.dasc
@@ -351,7 +351,6 @@ static void build_subroutines(BuildCtx *ctx)
   |   mv_vmstate CARG4, CFUNC
   |  ldr GL:CARG3, L->glref
   |   str CARG4, GL:CARG3->vmstate
-  |   str L, GL:CARG3->cur_L
   |  b ->vm_leave_unw
   |
   |->vm_unwind_ff:			// Unwind C stack, return from ff pcall.
@@ -372,7 +371,6 @@ static void build_subroutines(BuildCtx *ctx)
   |   mv_vmstate CARG2, INTERP
   |    str CARG1, [BASE, #-4]		// Prepend false to error message.
   |   st_vmstate CARG2
-  |   str L, [DISPATCH, #DISPATCH_GL(cur_L)]
   |  b ->vm_returnc
   |
   |->vm_unwind_ext:			// Complete external unwind.
-- 
2.42.0

[Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Maxim Kokryashkin via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)

Consider the following Lua C API function:

```
static int error_after_coroutine_return(lua_State *L)
{
	lua_State *innerL = lua_newthread(L);
	luaL_loadstring(innerL, "print('inner coro')");
	lua_pcall(innerL, 0, 0, 0);
	luaL_error(L, "my fancy error");
	return 0;
}
```

And the following Lua script:
```
local libcur_L = require('libcur_L')

local function onesnap_f(var)
  if var then
    return 1
  else
    return 0
  end
end

-- Compile function to trace with snapshot.
if jit then jit.opt.start('hotloop=1') end
onesnap_f(true)
onesnap_f(true)

local r, s = pcall(libcur_L.error_after_coroutine_return)
onesnap_f(false)
```

This is the only case when `cur_L` is not restored, according to
the analysis done in https://github.com/LuaJIT/LuaJIT/issues/1066.

This patch changes the error-catching routine, so now the patch
sets the actual cur_L there.
Now it is possible to throw errors on non-executing coroutines,
which is a violation of the Lua C API. So, even though it is now
possible, that behavior should be avoided anyway.

Maxim Kokryashkin:
* added the description and the test for the problem

Resolves tarantool/tarantool#6323
---
 src/lj_err.c                                  |  5 ++-
 test/tarantool-tests/CMakeLists.txt           |  1 +
 ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
 .../CMakeLists.txt                            |  1 +
 .../libcur_L_coroutine.c                      | 22 +++++++++++++
 5 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c

diff --git a/src/lj_err.c b/src/lj_err.c
index 46fb81ee..1a9a2f2b 100644
--- a/src/lj_err.c
+++ b/src/lj_err.c
@@ -174,12 +174,15 @@ static void *err_unwind(lua_State *L, void *stopcf, int errcode)
     case FRAME_PCALL:  /* FF pcall() frame. */
     case FRAME_PCALLH:  /* FF pcall() frame inside hook. */
       if (errcode) {
+	global_State *g;
 	if (errcode == LUA_YIELD) {
 	  frame = frame_prevd(frame);
 	  break;
 	}
+	g = G(L);
+	setgcref(g->cur_L, obj2gco(L));
 	if (frame_typep(frame) == FRAME_PCALL)
-	  hook_leave(G(L));
+	  hook_leave(g);
 	L->base = frame_prevd(frame) + 1;
 	L->cframe = cf;
 	unwindstack(L, L->base);
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index c15d6037..d84072e0 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -68,6 +68,7 @@ add_subdirectory(lj-727-lightuserdata-itern)
 add_subdirectory(lj-802-panic-at-mcode-protfail)
 add_subdirectory(lj-flush-on-trace)
 add_subdirectory(lj-1004-oom-error-frame)
+add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
 
 # The part of the memory profiler toolchain is located in tools
 # directory, jit, profiler, and bytecode toolchains are located
diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
new file mode 100644
index 00000000..3919ae23
--- /dev/null
+++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
@@ -0,0 +1,32 @@
+local tap = require('tap')
+local test = tap.test('lj-1066-fix-cur_L-after-coroutine-resume'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local libcur_L_coroutine = require('libcur_L_coroutine')
+
+local function cbool(cond)
+  if cond then
+    return 1
+  else
+    return 0
+  end
+end
+
+-- Compile function to trace with snapshot.
+jit.opt.start('hotloop=1')
+-- First call makes `cbool()` hot enough to be recorded next time.
+cbool(true)
+-- Second call records `cbool()` body (i.e. `if` branch). This is
+-- a root trace for `cbool()`.
+cbool(true)
+
+local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
+assert(res == false, "return from error")
+-- Call with restoration from a snapshot with wrong cur_L.
+cbool(false)
+
+test:ok(true)
+test:done(true)
diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
new file mode 100644
index 00000000..c8a3731f
--- /dev/null
+++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(libcur_L_coroutine libcur_L_coroutine.c)
diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
new file mode 100644
index 00000000..7a71d0f0
--- /dev/null
+++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
@@ -0,0 +1,22 @@
+#include "lua.h"
+#include "lauxlib.h"
+
+static int error_after_coroutine_return(lua_State *L)
+{
+	lua_State *innerL = lua_newthread(L);
+	luaL_loadstring(innerL, "print('inner coro')");
+	lua_pcall(innerL, 0, 0, 0);
+	luaL_error(L, "my fancy error");
+	return 0;
+}
+
+static const struct luaL_Reg libcur_L_coroutine[] = {
+	{"error_after_coroutine_return", error_after_coroutine_return},
+	{NULL, NULL}
+};
+
+LUA_API int luaopen_libcur_L_coroutine(lua_State *L)
+{
+	luaL_register(L, "libcur_L_coroutine", libcur_L_coroutine);
+	return 1;
+}
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit v2 1/5] Revert "Fix cur_L tracking on exceptional path"

[Sergey Bronnikov via Tarantool-patches]

Hi, Max

please see my comment below


On 9/29/23 09:20, Maxim Kokryashkin wrote:
> This reverts commit 97699d9ee2467389b6aea21a098e38aff3469b5f.
>
> As was mentioned in tarantool/tarantool#6189, throwing an error
> not on the currently executed coroutine is a violation of the
> Lua/C API.

Could you add a source of this statement?

There is a section "3.6 – Error Handling in C" in Lua 5.1 Reference Manual,

but there is no such statement there [1]. Could you clarify?

1. https://www.lua.org/manual/5.1/manual.html

> This patch is a part of the patchset that supports
> this violation and is reverted because of it.
>
> Part of tarantool/tarantool#6323
> ---
>   src/vm_x64.dasc | 9 ++++-----
>   src/vm_x86.dasc | 8 ++++----
>   2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 09bf67e5..116716ac 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc
> @@ -533,11 +533,11 @@ static void build_subroutines(BuildCtx *ctx)
>     |  mov eax, CARG2d			// Error return status for vm_pcall.
>     |  mov rsp, CARG1
>     |->vm_unwind_c_eh:			// Landing pad for external unwinder.
> -  |  mov L:DISPATCH, SAVE_L
> -  |  mov GL:RB, L:DISPATCH->glref
> -  |  mov GL:RB->cur_L, L:DISPATCH
> +  |  mov L:RB, SAVE_L
> +  |  mov GL:RB, L:RB->glref
> +  |  mov [GL:RB->cur_L], L:RB
>     |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
> -  |  mov DISPATCH, L:DISPATCH->glref	// Setup pointer to dispatch table.
> +  |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
>     |  add DISPATCH, GG_G2DISP
>     |  jmp ->vm_leave_unw
>     |
> @@ -561,7 +561,6 @@ static void build_subroutines(BuildCtx *ctx)
>     |  add DISPATCH, GG_G2DISP
>     |  mov PC, [BASE-8]			// Fetch PC of previous frame.
>     |  mov_false RA
> -  |  mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
>     |  mov RB, [BASE]
>     |  mov [BASE-16], RA			// Prepend false to error message.
>     |  mov [BASE-8], RB
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index f16ade1a..e3fbf751 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -681,11 +681,11 @@ static void build_subroutines(BuildCtx *ctx)
>     |.endif
>     |.endif
>     |->vm_unwind_c_eh:			// Landing pad for external unwinder.
> -  |  mov L:DISPATCH, SAVE_L
> -  |  mov GL:RB, L:DISPATCH->glref
> -  |  mov dword GL:RB->cur_L, L:DISPATCH
> +  |  mov L:RB, SAVE_L
> +  |  mov GL:RB, L:RB->glref
> +  |  mov dword GL:RB->cur_L, L:RB
>     |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
> -  |  mov DISPATCH, L:DISPATCH->glref	// Setup pointer to dispatch table.
> +  |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
>     |  add DISPATCH, GG_G2DISP
>     |  jmp ->vm_leave_unw
>     |

Re: [Tarantool-patches] [PATCH luajit v2 2/5] Revert "Update cur_L on exceptional path"

[Sergey Bronnikov via Tarantool-patches]

On 9/29/23 09:20, Maxim Kokryashkin wrote:
> This reverts commit ed412cd9f55fe87fd32a69c86e1732690fc5c1b0.
>
> As was mentioned in tarantool/tarantool#6189, throwing an error
> not on the currently executed coroutine is a violation of the
> Lua/C API. This patch is a part of the patchset that supports
> this violation and is reverted because of it.
Ditto.
> Part of tarantool/tarantool#6323
> ---
>   src/vm_x64.dasc | 1 -
>   src/vm_x86.dasc | 2 --
>   2 files changed, 3 deletions(-)
>
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> index 116716ac..399dfcbf 100644
> --- a/src/vm_x64.dasc
> +++ b/src/vm_x64.dasc
> @@ -535,7 +535,6 @@ static void build_subroutines(BuildCtx *ctx)
>     |->vm_unwind_c_eh:			// Landing pad for external unwinder.
>     |  mov L:RB, SAVE_L
>     |  mov GL:RB, L:RB->glref
> -  |  mov [GL:RB->cur_L], L:RB
>     |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>     |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
>     |  add DISPATCH, GG_G2DISP
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index e3fbf751..9fa9a3f7 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -683,7 +683,6 @@ static void build_subroutines(BuildCtx *ctx)
>     |->vm_unwind_c_eh:			// Landing pad for external unwinder.
>     |  mov L:RB, SAVE_L
>     |  mov GL:RB, L:RB->glref
> -  |  mov dword GL:RB->cur_L, L:RB
>     |  mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>     |  mov DISPATCH, GL:RB   // Setup pointer to dispatch table.
>     |  add DISPATCH, GG_G2DISP
> @@ -719,7 +718,6 @@ static void build_subroutines(BuildCtx *ctx)
>     |  add DISPATCH, GG_G2DISP
>     |  mov PC, [BASE-4]			// Fetch PC of previous frame.
>     |  mov dword [BASE-4], LJ_TFALSE	// Prepend false to error message.
> -  |  mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
>     |  // INTERP until jump to BC_RET* or return to C.
>     |  set_vmstate INTERP
>     |  jmp ->vm_returnc			// Increments RD/MULTRES and returns.

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


thanks for the patch! See my comments.


On 9/29/23 09:20, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)
>
> Consider the following Lua C API function:
>
> ```
> static int error_after_coroutine_return(lua_State *L)
> {
> 	lua_State *innerL = lua_newthread(L);
> 	luaL_loadstring(innerL, "print('inner coro')");
> 	lua_pcall(innerL, 0, 0, 0);
> 	luaL_error(L, "my fancy error");
> 	return 0;
> }
> ```
>
> And the following Lua script:
> ```
> local libcur_L = require('libcur_L')
>
> local function onesnap_f(var)
>    if var then
>      return 1
>    else
>      return 0
>    end
> end
>
> -- Compile function to trace with snapshot.
> if jit then jit.opt.start('hotloop=1') end
> onesnap_f(true)
> onesnap_f(true)
>
> local r, s = pcall(libcur_L.error_after_coroutine_return)
> onesnap_f(false)
> ```
>
> This is the only case when `cur_L` is not restored, according to
> the analysis done in https://github.com/LuaJIT/LuaJIT/issues/1066.

Please remove link to the issue from commit message,

this could be a reason of spam pings in the issue itself.

>
> This patch changes the error-catching routine, so now the patch
> sets the actual cur_L there.
> Now it is possible to throw errors on non-executing coroutines,
> which is a violation of the Lua C API. So, even though it is now
> possible, that behavior should be avoided anyway.
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Resolves tarantool/tarantool#6323
> ---
>   src/lj_err.c                                  |  5 ++-
>   test/tarantool-tests/CMakeLists.txt           |  1 +
>   ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
>   .../CMakeLists.txt                            |  1 +
>   .../libcur_L_coroutine.c                      | 22 +++++++++++++
>   5 files changed, 60 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>   create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>   create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
>
> diff --git a/src/lj_err.c b/src/lj_err.c
> index 46fb81ee..1a9a2f2b 100644
> --- a/src/lj_err.c
> +++ b/src/lj_err.c
> @@ -174,12 +174,15 @@ static void *err_unwind(lua_State *L, void *stopcf, int errcode)
>       case FRAME_PCALL:  /* FF pcall() frame. */
>       case FRAME_PCALLH:  /* FF pcall() frame inside hook. */
>         if (errcode) {
> +	global_State *g;
>   	if (errcode == LUA_YIELD) {
>   	  frame = frame_prevd(frame);
>   	  break;
>   	}
> +	g = G(L);
> +	setgcref(g->cur_L, obj2gco(L));
>   	if (frame_typep(frame) == FRAME_PCALL)
> -	  hook_leave(G(L));
> +	  hook_leave(g);
>   	L->base = frame_prevd(frame) + 1;
>   	L->cframe = cf;
>   	unwindstack(L, L->base);
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..d84072e0 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -68,6 +68,7 @@ add_subdirectory(lj-727-lightuserdata-itern)
>   add_subdirectory(lj-802-panic-at-mcode-protfail)
>   add_subdirectory(lj-flush-on-trace)
>   add_subdirectory(lj-1004-oom-error-frame)
> +add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
>   
>   # The part of the memory profiler toolchain is located in tools
>   # directory, jit, profiler, and bytecode toolchains are located
> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> new file mode 100644
> index 00000000..3919ae23
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> @@ -0,0 +1,32 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1066-fix-cur_L-after-coroutine-resume'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local libcur_L_coroutine = require('libcur_L_coroutine')
> +
> +local function cbool(cond)
> +  if cond then
> +    return 1
> +  else
> +    return 0
> +  end
> +end
> +
> +-- Compile function to trace with snapshot.
> +jit.opt.start('hotloop=1')
> +-- First call makes `cbool()` hot enough to be recorded next time.
> +cbool(true)
> +-- Second call records `cbool()` body (i.e. `if` branch). This is
> +-- a root trace for `cbool()`.
> +cbool(true)
> +
> +local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
> +assert(res == false, "return from error")
> +-- Call with restoration from a snapshot with wrong cur_L.
> +cbool(false)
> +
> +test:ok(true)
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
> new file mode 100644
> index 00000000..c8a3731f
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
> @@ -0,0 +1 @@
> +BuildTestCLib(libcur_L_coroutine libcur_L_coroutine.c)
> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> new file mode 100644
> index 00000000..7a71d0f0
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> @@ -0,0 +1,22 @@
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +static int error_after_coroutine_return(lua_State *L)
> +{
> +	lua_State *innerL = lua_newthread(L);
> +	luaL_loadstring(innerL, "print('inner coro')");
> +	lua_pcall(innerL, 0, 0, 0);
> +	luaL_error(L, "my fancy error");
> +	return 0;
> +}
> +
> +static const struct luaL_Reg libcur_L_coroutine[] = {
> +	{"error_after_coroutine_return", error_after_coroutine_return},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_libcur_L_coroutine(lua_State *L)
> +{
> +	luaL_register(L, "libcur_L_coroutine", libcur_L_coroutine);
> +	return 1;
> +}

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi!
Thanks for the review!
Fixed your comment, the branch is force-pushed.
 
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 3 октября 2023, 22:16 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hi, Max
>
>
>thanks for the patch! See my comments.
>
>
>On 9/29/23 09:20, Maxim Kokryashkin wrote:
>> From: Mike Pall <mike>
>>
>> Thanks to Peter Cawley.
>>
>> (cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)
>>
>> Consider the following Lua C API function:
>>
>> ```
>> static int error_after_coroutine_return(lua_State *L)
>> {
>> lua_State *innerL = lua_newthread(L);
>> luaL_loadstring(innerL, "print('inner coro')");
>> lua_pcall(innerL, 0, 0, 0);
>> luaL_error(L, "my fancy error");
>> return 0;
>> }
>> ```
>>
>> And the following Lua script:
>> ```
>> local libcur_L = require('libcur_L')
>>
>> local function onesnap_f(var)
>> if var then
>> return 1
>> else
>> return 0
>> end
>> end
>>
>> -- Compile function to trace with snapshot.
>> if jit then jit.opt.start('hotloop=1') end
>> onesnap_f(true)
>> onesnap_f(true)
>>
>> local r, s = pcall(libcur_L.error_after_coroutine_return)
>> onesnap_f(false)
>> ```
>>
>> This is the only case when `cur_L` is not restored, according to
>> the analysis done in  https://github.com/LuaJIT/LuaJIT/issues/1066 .
>
>Please remove link to the issue from commit message,
>
>this could be a reason of spam pings in the issue itself.
>
>>
>> This patch changes the error-catching routine, so now the patch
>> sets the actual cur_L there.
>> Now it is possible to throw errors on non-executing coroutines,
>> which is a violation of the Lua C API. So, even though it is now
>> possible, that behavior should be avoided anyway.
>>
>> Maxim Kokryashkin:
>> * added the description and the test for the problem
>>
>> Resolves tarantool/tarantool#6323
>> ---
>> src/lj_err.c | 5 ++-
>> test/tarantool-tests/CMakeLists.txt | 1 +
>> ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
>> .../CMakeLists.txt | 1 +
>> .../libcur_L_coroutine.c | 22 +++++++++++++
>> 5 files changed, 60 insertions(+), 1 deletion(-)
>> create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>> create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>> create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
>>
>> diff --git a/src/lj_err.c b/src/lj_err.c
>> index 46fb81ee..1a9a2f2b 100644
>> --- a/src/lj_err.c
>> +++ b/src/lj_err.c
>> @@ -174,12 +174,15 @@ static void *err_unwind(lua_State *L, void *stopcf, int errcode)
>> case FRAME_PCALL: /* FF pcall() frame. */
>> case FRAME_PCALLH: /* FF pcall() frame inside hook. */
>> if (errcode) {
>> + global_State *g;
>> if (errcode == LUA_YIELD) {
>> frame = frame_prevd(frame);
>> break;
>> }
>> + g = G(L);
>> + setgcref(g->cur_L, obj2gco(L));
>> if (frame_typep(frame) == FRAME_PCALL)
>> - hook_leave(G(L));
>> + hook_leave(g);
>> L->base = frame_prevd(frame) + 1;
>> L->cframe = cf;
>> unwindstack(L, L->base);
>> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>> index c15d6037..d84072e0 100644
>> --- a/test/tarantool-tests/CMakeLists.txt
>> +++ b/test/tarantool-tests/CMakeLists.txt
>> @@ -68,6 +68,7 @@ add_subdirectory(lj-727-lightuserdata-itern)
>> add_subdirectory(lj-802-panic-at-mcode-protfail)
>> add_subdirectory(lj-flush-on-trace)
>> add_subdirectory(lj-1004-oom-error-frame)
>> +add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
>>
>> # The part of the memory profiler toolchain is located in tools
>> # directory, jit, profiler, and bytecode toolchains are located
>> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>> new file mode 100644
>> index 00000000..3919ae23
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>> @@ -0,0 +1,32 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-1066-fix-cur_L-after-coroutine-resume'):skipcond({
>> + ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(1)
>> +
>> +local libcur_L_coroutine = require('libcur_L_coroutine')
>> +
>> +local function cbool(cond)
>> + if cond then
>> + return 1
>> + else
>> + return 0
>> + end
>> +end
>> +
>> +-- Compile function to trace with snapshot.
>> +jit.opt.start('hotloop=1')
>> +-- First call makes `cbool()` hot enough to be recorded next time.
>> +cbool(true)
>> +-- Second call records `cbool()` body (i.e. `if` branch). This is
>> +-- a root trace for `cbool()`.
>> +cbool(true)
>> +
>> +local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
>> +assert(res == false, "return from error")
>> +-- Call with restoration from a snapshot with wrong cur_L.
>> +cbool(false)
>> +
>> +test:ok(true)
>> +test:done(true)
>> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>> new file mode 100644
>> index 00000000..c8a3731f
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>> @@ -0,0 +1 @@
>> +BuildTestCLib(libcur_L_coroutine libcur_L_coroutine.c)
>> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
>> new file mode 100644
>> index 00000000..7a71d0f0
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
>> @@ -0,0 +1,22 @@
>> +#include "lua.h"
>> +#include "lauxlib.h"
>> +
>> +static int error_after_coroutine_return(lua_State *L)
>> +{
>> + lua_State *innerL = lua_newthread(L);
>> + luaL_loadstring(innerL, "print('inner coro')");
>> + lua_pcall(innerL, 0, 0, 0);
>> + luaL_error(L, "my fancy error");
>> + return 0;
>> +}
>> +
>> +static const struct luaL_Reg libcur_L_coroutine[] = {
>> + {"error_after_coroutine_return", error_after_coroutine_return},
>> + {NULL, NULL}
>> +};
>> +
>> +LUA_API int luaopen_libcur_L_coroutine(lua_State *L)
>> +{
>> + luaL_register(L, "libcur_L_coroutine", libcur_L_coroutine);
>> + return 1;
>> +}
 

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

Re: [Tarantool-patches] [PATCH luajit v2 2/5] Revert "Update cur_L on exceptional path"

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the review!
Fixed your comment, branch is force-pushed.
 
 
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 3 октября 2023, 21:55 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>
>On 9/29/23 09:20, Maxim Kokryashkin wrote:
>> This reverts commit ed412cd9f55fe87fd32a69c86e1732690fc5c1b0.
>>
>> As was mentioned in tarantool/tarantool#6189, throwing an error
>> not on the currently executed coroutine is a violation of the
>> Lua/C API. This patch is a part of the patchset that supports
>> this violation and is reverted because of it.
>Ditto.
>> Part of tarantool/tarantool#6323
>> ---
>> src/vm_x64.dasc | 1 -
>> src/vm_x86.dasc | 2 --
>> 2 files changed, 3 deletions(-)
>>
>> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
>> index 116716ac..399dfcbf 100644
>> --- a/src/vm_x64.dasc
>> +++ b/src/vm_x64.dasc
>> @@ -535,7 +535,6 @@ static void build_subroutines(BuildCtx *ctx)
>> |->vm_unwind_c_eh: // Landing pad for external unwinder.
>> | mov L:RB, SAVE_L
>> | mov GL:RB, L:RB->glref
>> - | mov [GL:RB->cur_L], L:RB
>> | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>> | mov DISPATCH, GL:RB // Setup pointer to dispatch table.
>> | add DISPATCH, GG_G2DISP
>> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
>> index e3fbf751..9fa9a3f7 100644
>> --- a/src/vm_x86.dasc
>> +++ b/src/vm_x86.dasc
>> @@ -683,7 +683,6 @@ static void build_subroutines(BuildCtx *ctx)
>> |->vm_unwind_c_eh: // Landing pad for external unwinder.
>> | mov L:RB, SAVE_L
>> | mov GL:RB, L:RB->glref
>> - | mov dword GL:RB->cur_L, L:RB
>> | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>> | mov DISPATCH, GL:RB // Setup pointer to dispatch table.
>> | add DISPATCH, GG_G2DISP
>> @@ -719,7 +718,6 @@ static void build_subroutines(BuildCtx *ctx)
>> | add DISPATCH, GG_G2DISP
>> | mov PC, [BASE-4] // Fetch PC of previous frame.
>> | mov dword [BASE-4], LJ_TFALSE // Prepend false to error message.
>> - | mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
>> | // INTERP until jump to BC_RET* or return to C.
>> | set_vmstate INTERP
>> | jmp ->vm_returnc // Increments RD/MULTRES and returns.
 

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

Re: [Tarantool-patches] [PATCH luajit v2 1/5] Revert "Fix cur_L tracking on exceptional path"

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the review!
Fixed your comment, branch is force-pushed.
 
 
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 3 октября 2023, 21:54 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Hi, Max
>
>please see my comment below
>
>
>On 9/29/23 09:20, Maxim Kokryashkin wrote:
>> This reverts commit 97699d9ee2467389b6aea21a098e38aff3469b5f.
>>
>> As was mentioned in tarantool/tarantool#6189, throwing an error
>> not on the currently executed coroutine is a violation of the
>> Lua/C API.
>
>Could you add a source of this statement?
>
>There is a section "3.6 – Error Handling in C" in Lua 5.1 Reference Manual,
>
>but there is no such statement there [1]. Could you clarify?
>
>1.  https://www.lua.org/manual/5.1/manual.html
>
>> This patch is a part of the patchset that supports
>> this violation and is reverted because of it.
>>
>> Part of tarantool/tarantool#6323
>> ---
>> src/vm_x64.dasc | 9 ++++-----
>> src/vm_x86.dasc | 8 ++++----
>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
>> index 09bf67e5..116716ac 100644
>> --- a/src/vm_x64.dasc
>> +++ b/src/vm_x64.dasc
>> @@ -533,11 +533,11 @@ static void build_subroutines(BuildCtx *ctx)
>> | mov eax, CARG2d // Error return status for vm_pcall.
>> | mov rsp, CARG1
>> |->vm_unwind_c_eh: // Landing pad for external unwinder.
>> - | mov L:DISPATCH, SAVE_L
>> - | mov GL:RB, L:DISPATCH->glref
>> - | mov GL:RB->cur_L, L:DISPATCH
>> + | mov L:RB, SAVE_L
>> + | mov GL:RB, L:RB->glref
>> + | mov [GL:RB->cur_L], L:RB
>> | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>> - | mov DISPATCH, L:DISPATCH->glref // Setup pointer to dispatch table.
>> + | mov DISPATCH, GL:RB // Setup pointer to dispatch table.
>> | add DISPATCH, GG_G2DISP
>> | jmp ->vm_leave_unw
>> |
>> @@ -561,7 +561,6 @@ static void build_subroutines(BuildCtx *ctx)
>> | add DISPATCH, GG_G2DISP
>> | mov PC, [BASE-8] // Fetch PC of previous frame.
>> | mov_false RA
>> - | mov [DISPATCH+DISPATCH_GL(cur_L)], L:RB
>> | mov RB, [BASE]
>> | mov [BASE-16], RA // Prepend false to error message.
>> | mov [BASE-8], RB
>> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
>> index f16ade1a..e3fbf751 100644
>> --- a/src/vm_x86.dasc
>> +++ b/src/vm_x86.dasc
>> @@ -681,11 +681,11 @@ static void build_subroutines(BuildCtx *ctx)
>> |.endif
>> |.endif
>> |->vm_unwind_c_eh: // Landing pad for external unwinder.
>> - | mov L:DISPATCH, SAVE_L
>> - | mov GL:RB, L:DISPATCH->glref
>> - | mov dword GL:RB->cur_L, L:DISPATCH
>> + | mov L:RB, SAVE_L
>> + | mov GL:RB, L:RB->glref
>> + | mov dword GL:RB->cur_L, L:RB
>> | mov dword GL:RB->vmstate, ~LJ_VMST_CFUNC
>> - | mov DISPATCH, L:DISPATCH->glref // Setup pointer to dispatch table.
>> + | mov DISPATCH, GL:RB // Setup pointer to dispatch table.
>> | add DISPATCH, GG_G2DISP
>> | jmp ->vm_leave_unw
>> |
 

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

Re: [Tarantool-patches] [PATCH luajit v2 1/5] Revert "Fix cur_L tracking on exceptional path"

[Sergey Bronnikov via Tarantool-patches]

On 10/4/23 14:27, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comment, branch is force-pushed.
>
Thanks! LGTM

Re: [Tarantool-patches] [PATCH luajit v2 2/5] Revert "Update cur_L on exceptional path"

[Sergey Bronnikov via Tarantool-patches]

On 10/4/23 14:27, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comment, branch is force-pushed.
Thanks! LGTM

Re: [Tarantool-patches] [PATCH luajit v2 3/5] Revert "arm64: fix cur_L restoration on error throw"

[Sergey Bronnikov via Tarantool-patches]

On 9/29/23 09:20, Maxim Kokryashkin wrote:
> This reverts commit 7570ff61138fb50a17ed78ee28dde0acbbf6131e.
>
> As was mentioned in tarantool/tarantool#6189, throwing an error
> not on the currently executed coroutine is a violation of the
> Lua/C API. This patch is a part of the patchset that supports
> this violation and is reverted because of it.
>
LGTM

<snipped>

Re: [Tarantool-patches] [PATCH luajit v2 4/5] Revert "Update cur_L on exceptional path (arm)"

[Sergey Bronnikov via Tarantool-patches]

On 9/29/23 09:20, Maxim Kokryashkin wrote:
> This reverts commit 5ccd25d740476a37d414733b5192d5be0ef06173.
>
> As was mentioned in tarantool/tarantool#6189, throwing an error
> not on the currently executed coroutine is a violation of the
> Lua/C API. This patch is a part of the patchset that supports
> this violation and is reverted because of it.
>
<snipped>

LGTM

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Sergey Bronnikov via Tarantool-patches]

On 10/4/23 14:26, Maxim Kokryashkin wrote:
> Hi!
> Thanks for the review!
> Fixed your comment, the branch is force-pushed.
>
<snipped>


Thanks! LGTM

Re: [Tarantool-patches] [PATCH luajit v2 1/5] Revert "Fix cur_L tracking on exceptional path"

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 2/5] Revert "Update cur_L on exceptional path"

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 3/5] Revert "arm64: fix cur_L restoration on error throw"

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 4/5] Revert "Update cur_L on exceptional path (arm)"

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM, after fixing comments below.

I suppose that this patch should be the first in the patch series
since, otherwise, the test <tarantool-tests/gh-6189-cur_L.test.lua> will
fail on the corresponding architectures before this commit.

AFAIR, there are still some API misusages in the Tarantool Lua C code.
I suppose they should be fixed first before this patch is merged.
Please correct me if I am wrong.

On 29.09.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)
> 
> Consider the following Lua C API function:
> 
> ```
> static int error_after_coroutine_return(lua_State *L)
> {
> 	lua_State *innerL = lua_newthread(L);
> 	luaL_loadstring(innerL, "print('inner coro')");
> 	lua_pcall(innerL, 0, 0, 0);
> 	luaL_error(L, "my fancy error");
> 	return 0;
> }
> ```
> 
> And the following Lua script:
> ```
> local libcur_L = require('libcur_L')
> 
> local function onesnap_f(var)
>   if var then
>     return 1
>   else
>     return 0
>   end
> end
> 
> -- Compile function to trace with snapshot.
> if jit then jit.opt.start('hotloop=1') end
> onesnap_f(true)
> onesnap_f(true)
> 
> local r, s = pcall(libcur_L.error_after_coroutine_return)
> onesnap_f(false)
> ```

Minor: I prefer to describe the test case in words but not copy-paste it
from tests. I suppose it will be enough to mention the test case for
details.

> 
> This is the only case when `cur_L` is not restored, according to
> the analysis done in https://github.com/LuaJIT/LuaJIT/issues/1066.
> 
> This patch changes the error-catching routine, so now the patch
> sets the actual cur_L there.
> Now it is possible to throw errors on non-executing coroutines,
> which is a violation of the Lua C API. So, even though it is now
> possible, that behavior should be avoided anyway.

It's worth mentioning the analysis done in the 1066 that there is no
need to patch case FRAME_CP because all work is done inside the VM, when
returning from the C function called via `lua_cpcall()`.
You may recap the analisys from 1066 here also to simplify reading of
the patch.

> 
> Maxim Kokryashkin:
> * added the description and the test for the problem
> 
> Resolves tarantool/tarantool#6323

Missed label:
| Part of tarantool/tarantool#9145

> ---
>  src/lj_err.c                                  |  5 ++-
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
>  .../CMakeLists.txt                            |  1 +
>  .../libcur_L_coroutine.c                      | 22 +++++++++++++
>  5 files changed, 60 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> 
> diff --git a/src/lj_err.c b/src/lj_err.c

<snipped>

> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..d84072e0 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt

<snipped>

> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> new file mode 100644
> index 00000000..3919ae23
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> @@ -0,0 +1,32 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1066-fix-cur_L-after-coroutine-resume'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local libcur_L_coroutine = require('libcur_L_coroutine')
> +
> +local function cbool(cond)
> +  if cond then
> +    return 1
> +  else
> +    return 0
> +  end
> +end
> +
> +-- Compile function to trace with snapshot.
> +jit.opt.start('hotloop=1')
> +-- First call makes `cbool()` hot enough to be recorded next time.
> +cbool(true)
> +-- Second call records `cbool()` body (i.e. `if` branch). This is
> +-- a root trace for `cbool()`.
> +cbool(true)
> +
> +local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
> +assert(res == false, "return from error")

Nit: Please, use single quotes here.

> +-- Call with restoration from a snapshot with wrong cur_L.
> +cbool(false)
> +
> +test:ok(true)
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt

<snipped>

> diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> new file mode 100644
> index 00000000..7a71d0f0
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> @@ -0,0 +1,22 @@
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +static int error_after_coroutine_return(lua_State *L)
> +{
> +	lua_State *innerL = lua_newthread(L);
> +	luaL_loadstring(innerL, "print('inner coro')");

There is no need for debug printing here, just "return" in enough, I
suppose.

> +	lua_pcall(innerL, 0, 0, 0);
> +	luaL_error(L, "my fancy error");
> +	return 0;
> +}
> +
> +static const struct luaL_Reg libcur_L_coroutine[] = {
> +	{"error_after_coroutine_return", error_after_coroutine_return},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_libcur_L_coroutine(lua_State *L)
> +{
> +	luaL_register(L, "libcur_L_coroutine", libcur_L_coroutine);
> +	return 1;
> +}
> -- 
> 2.42.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
See my replies below.
Fixed your comments, the branch is force-pushed.

On Tue, Oct 10, 2023 at 11:57:22AM +0300, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Maxim!
> Thanks for the patch!
> LGTM, after fixing comments below.
> 
> I suppose that this patch should be the first in the patch series
> since, otherwise, the test <tarantool-tests/gh-6189-cur_L.test.lua> will
> fail on the corresponding architectures before this commit.
Moved
> 
> AFAIR, there are still some API misusages in the Tarantool Lua C code.
> I suppose they should be fixed first before this patch is merged.
> Please correct me if I am wrong.
Yep, but there are two important aspects:
1. This patch actually make this misuse legal now, so it is questionable,
whether it should be fixed or not.
2. The issue in tarantool is more broad -- Lua coroutines and C fibers
are switched completely unaware of one another, which is not a great
design choice. To provide 100% correct switching, LuaJIT API changes
are required, which should be discussed separately.

> 
> On 29.09.23, Maxim Kokryashkin wrote:
> > From: Mike Pall <mike>
> > 
> > Thanks to Peter Cawley.
> > 
> > (cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)
> > 
> > Consider the following Lua C API function:
> > 
> > ```
> > static int error_after_coroutine_return(lua_State *L)
> > {
> > 	lua_State *innerL = lua_newthread(L);
> > 	luaL_loadstring(innerL, "print('inner coro')");
> > 	lua_pcall(innerL, 0, 0, 0);
> > 	luaL_error(L, "my fancy error");
> > 	return 0;
> > }
> > ```
> > 
> > And the following Lua script:
> > ```
> > local libcur_L = require('libcur_L')
> > 
> > local function onesnap_f(var)
> >   if var then
> >     return 1
> >   else
> >     return 0
> >   end
> > end
> > 
> > -- Compile function to trace with snapshot.
> > if jit then jit.opt.start('hotloop=1') end
> > onesnap_f(true)
> > onesnap_f(true)
> > 
> > local r, s = pcall(libcur_L.error_after_coroutine_return)
> > onesnap_f(false)
> > ```
> 
> Minor: I prefer to describe the test case in words but not copy-paste it
> from tests. I suppose it will be enough to mention the test case for
> details.
Fixed!
> 
> > 
> > This is the only case when `cur_L` is not restored, according to
> > the analysis done in https://github.com/LuaJIT/LuaJIT/issues/1066.
> > 
> > This patch changes the error-catching routine, so now the patch
> > sets the actual cur_L there.
> > Now it is possible to throw errors on non-executing coroutines,
> > which is a violation of the Lua C API. So, even though it is now
> > possible, that behavior should be avoided anyway.
> 
> It's worth mentioning the analysis done in the 1066 that there is no
> need to patch case FRAME_CP because all work is done inside the VM, when
> returning from the C function called via `lua_cpcall()`.
> You may recap the analisys from 1066 here also to simplify reading of
> the patch.
> 
> > 
> > Maxim Kokryashkin:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#6323
> 
> Missed label:
> | Part of tarantool/tarantool#9145
Fixed!

Here is the new commit message:
===
Restore cur_L for specific Lua/C API use case.

Thanks to Peter Cawley.

(cherry-picked from commit e86990f7f24a94b0897061f25a84547fe1108bed)

There is a case of valid Lua C API usage when non-throwing code
is executed on coroutine, other than one that an entry-point C
function was called with, which results in a segmentation fault
because cur_L is not being restored correspondingly. For a
comprehensive example, see the test case added within this patch.

According to the analysis done in lj-1066, to hit this issue,
we first need to call into a C function. That C function needs
to change cur_L by calling lua_resume/lua_p?call, and then the
only problematic path is leaving the C function by throwing an
error, which is caught by the fast-function x?pcall. When
x?pcall then returns to its caller, we can be executing
bytecode in the VM with an incorrect cur_L, and things will go
badly if we enter a trace before correcting cur_L.

The fix proposed in lj-740 was to set cur_L on error throw;
however, the analysis from lj-1066 suggests that it can be set
on the catching phase.

This patch changes the error-catching routine, so now the patch
sets the actual cur_L there.
Now it is possible to throw errors on non-executing coroutines,
which is a violation of the Lua C API. So, even though it is now
possible, that behavior should be avoided anyway.

Maxim Kokryashkin:
* added the description and the test for the problem

Resolves tarantool/tarantool#6323
Part of tarantool/tarantool#9145
===
> 
> > ---
> >  src/lj_err.c                                  |  5 ++-
> >  test/tarantool-tests/CMakeLists.txt           |  1 +
> >  ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
> >  .../CMakeLists.txt                            |  1 +
> >  .../libcur_L_coroutine.c                      | 22 +++++++++++++
> >  5 files changed, 60 insertions(+), 1 deletion(-)
> >  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> >  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
> >  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> > 
> > diff --git a/src/lj_err.c b/src/lj_err.c
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index c15d6037..d84072e0 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> > new file mode 100644
> > index 00000000..3919ae23
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
> > @@ -0,0 +1,32 @@
> > +local tap = require('tap')
> > +local test = tap.test('lj-1066-fix-cur_L-after-coroutine-resume'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(1)
> > +
> > +local libcur_L_coroutine = require('libcur_L_coroutine')
> > +
> > +local function cbool(cond)
> > +  if cond then
> > +    return 1
> > +  else
> > +    return 0
> > +  end
> > +end
> > +
> > +-- Compile function to trace with snapshot.
> > +jit.opt.start('hotloop=1')
> > +-- First call makes `cbool()` hot enough to be recorded next time.
> > +cbool(true)
> > +-- Second call records `cbool()` body (i.e. `if` branch). This is
> > +-- a root trace for `cbool()`.
> > +cbool(true)
> > +
> > +local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
> > +assert(res == false, "return from error")
> 
> Nit: Please, use single quotes here.
Fixed!
> 
> > +-- Call with restoration from a snapshot with wrong cur_L.
> > +cbool(false)
> > +
> > +test:ok(true)
> > +test:done(true)
> > diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> > new file mode 100644
> > index 00000000..7a71d0f0
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> > @@ -0,0 +1,22 @@
> > +#include "lua.h"
> > +#include "lauxlib.h"
> > +
> > +static int error_after_coroutine_return(lua_State *L)
> > +{
> > +	lua_State *innerL = lua_newthread(L);
> > +	luaL_loadstring(innerL, "print('inner coro')");
> 
> There is no need for debug printing here, just "return" in enough, I
> suppose.
> 
Fixed!
> > +	lua_pcall(innerL, 0, 0, 0);
> > +	luaL_error(L, "my fancy error");
> > +	return 0;
> > +}
> > +
> > +static const struct luaL_Reg libcur_L_coroutine[] = {
> > +	{"error_after_coroutine_return", error_after_coroutine_return},
> > +	{NULL, NULL}
> > +};
> > +
> > +LUA_API int luaopen_libcur_L_coroutine(lua_State *L)
> > +{
> > +	luaL_register(L, "libcur_L_coroutine", libcur_L_coroutine);
> > +	return 1;
> > +}
> > -- 
> > 2.42.0
Here is the diff:
===
diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
index 3919ae23..7f3739bf 100644
--- a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
+++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
@@ -24,7 +24,7 @@ cbool(true)
 cbool(true)
 
 local res = pcall(libcur_L_coroutine.error_after_coroutine_return)
-assert(res == false, "return from error")
+assert(res == false, 'return from error')
 -- Call with restoration from a snapshot with wrong cur_L.
 cbool(false)
 
diff --git a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
index 7a71d0f0..39d90e18 100644
--- a/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
+++ b/test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
@@ -4,7 +4,7 @@
 static int error_after_coroutine_return(lua_State *L)
 {
 	lua_State *innerL = lua_newthread(L);
-	luaL_loadstring(innerL, "print('inner coro')");
+	luaL_loadstring(innerL, "return");
 	lua_pcall(innerL, 0, 0, 0);
 	luaL_error(L, "my fancy error");
 	return 0;
===
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2 0/5] Fix error-throwing on an incorrect coroutine

[Igor Munkin via Tarantool-patches]

Max,

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 29.09.23, Maxim Kokryashkin via Tarantool-patches wrote:
> Changes in v2:
> - Fixed comments as per offline talk with Igor:
>     * Reverted one more patch for ARM
>     ('arm64: fix cur_L restoration on error throw').
>     * Added test from the original issue LuaJIT/LuaJIT#1066
> 
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6323-fix-curL
> PR: https://github.com/tarantool/tarantool/pull/9168
> 
> Maxim Kokryashkin (4):
>   Revert "Fix cur_L tracking on exceptional path"
>   Revert "Update cur_L on exceptional path"
>   Revert "arm64: fix cur_L restoration on error throw"
>   Revert "Update cur_L on exceptional path (arm)"
> 
> Mike Pall (1):
>   Restore cur_L for specific Lua/C API use case.
> 
>  src/lj_err.c                                  |  5 ++-
>  src/vm_arm.dasc                               |  2 --
>  src/vm_arm64.dasc                             |  2 --
>  src/vm_x64.dasc                               |  8 ++---
>  src/vm_x86.dasc                               |  8 ++---
>  test/tarantool-tests/CMakeLists.txt           |  1 +
>  ...-fix-cur_L-after-coroutine-resume.test.lua | 32 +++++++++++++++++++
>  .../CMakeLists.txt                            |  1 +
>  .../libcur_L_coroutine.c                      | 22 +++++++++++++
>  9 files changed, 66 insertions(+), 15 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume.test.lua
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/CMakeLists.txt
>  create mode 100644 test/tarantool-tests/lj-1066-fix-cur_L-after-coroutine-resume/libcur_L_coroutine.c
> 
> -- 
> 2.42.0
> 

-- 
Best regards,
IM