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