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

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

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