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;
> +}