[Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case.
Sergey Kaplun
skaplun at tarantool.org
Tue Oct 10 11:57:22 MSK 2023
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
More information about the Tarantool-patches
mailing list