

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