From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id C30EE6A4D5A; Tue, 10 Oct 2023 12:01:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C30EE6A4D5A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1696928512; bh=z2Qlf99zz0wFKW63I+UVmqwDw64KxIx7h91Kmqw7Zww=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ic0kByunxL9Iw3qc9Ia2jsheoj21OLnJXZfkdiBfOJx1yuHRuQizVGtGCBoon8XyM o+8swbBQPvuMhgSFCcfd5oI89m3mJg05gClHEld3ondFn5jqLUL1zmM664wCWzV5vA PGHmSlq1b7v9beTPfbpMaCBDyONzqgs/QEQrDmL0= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B39145042C1 for ; Tue, 10 Oct 2023 12:01:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B39145042C1 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1qq8cg-0003OK-St; Tue, 10 Oct 2023 12:01:51 +0300 Date: Tue, 10 Oct 2023 11:57:22 +0300 To: Maxim Kokryashkin Message-ID: References: <839195a6a6ff32a49c5a690a9f0101cc741901fd.1695968227.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <839195a6a6ff32a49c5a690a9f0101cc741901fd.1695968227.git.m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9FE0487E502468146EC9FB4099193202C3CCBF29BAB5B817F00894C459B0CD1B9DD0857CBF79F112BFB896534015F3050571D51BB1CB3E970495FBEDC84B91D45 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77BF46084C0059042EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374ECA27954A00B6C3EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BE5CCB53A13BC8DBA38B13EA6623A5D63E1AA253F0094815CCC7F00164DA146DAFE8445B8C89999728AA50765F79006377C70927E34808485389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8FA486DC37A503D0BF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE33AC447995A7AD182BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735473DB2713789FF2EC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5BC46C8F4BA04361ED40BE5FBA6A2D0EC82EAFCBCFDB7A4BCF87CCE6106E1FC07E67D4AC08A07B9B01F9513A7CA91E555CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF91B16D1FAEFD42698B161A93E4925A346A1983F7E8582891D50C4B4CBB287FEF498A0925D8903450ED8146B44B5935893F7075763B622334371EB8A11D63A5B6E48CAC7CA610320002C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj7Bv9MRqw1O3PB1gXGkoVLQ== X-DA7885C5: 203EDF3974E3AF4742560EE143F0215FE9EEA0E7816F6EF573AF22BFB709F0C9262E2D401490A4A0DB037EFA58388B346E8BC1A9835FDE71 X-Mailru-Sender: 689FA8AB762F73930F533AC2B33E986BCBB3700952E56EBEAAC0A1408292953E0FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2 5/5] Restore cur_L for specific Lua/C API use case. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 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 > > 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 > 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 > 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 > 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