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 DFF5D6EC40; Wed, 18 Aug 2021 23:04:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DFF5D6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629317082; bh=vLJNYqvw02fNGfwPlMwHKlKWsT9UsrM6ieRjs4AEvN8=; 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=rPZYLKic5PGZKn91aUFZOpUqLDWtLdl1uZQpXUBPAAJLjUTvN7wGH3v7+MImIAUk+ HBVGb0ZctRfm20CHyOA74z7fESKV+9zWs6zdpK6qnfVNOwhO0alYtQfFAxTdnOTLmc 4UfQXmgWcixXd3yZkVxG6bSOW0O8pARVsBFSQ7as= Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 AD0356EC40 for ; Wed, 18 Aug 2021 23:04:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AD0356EC40 Received: by smtp36.i.mail.ru with esmtpa (envelope-from ) id 1mGRni-0000Ub-4z; Wed, 18 Aug 2021 23:04:38 +0300 Date: Wed, 18 Aug 2021 23:03:20 +0300 To: Igor Munkin Message-ID: References: <20210816101949.25035-1-skaplun@tarantool.org> <20210818165705.GF5743@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210818165705.GF5743@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD972FF4A7D76DB5E242D14FEF1BD8BF4AC182A05F538085040CFA50F6CF1ADE5954DF66435ECB2E16BDAFD2B878A4F193BAB3FFE99959795E2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BCC85671EC7A750CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637FD60A286D0BA57028638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8D95717C0DB29C5920E14F37CA56044F9117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B60A62CEF541B197C8089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5055458D4952A8EBB9B702BE0C577A24119 X-C1DE0DAB: 0D63561A33F958A5638BEE80CE1C0E0C0938FFBF880422EBF9D004FE949C0D9AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75C29D03FC76C37677410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C1D376EF32BB0896A1E8A104D093A76716483DB05849054E58C8166C2241A1AE5D94C4E542B14B7B1D7E09C32AA3244C88F81AC70F9A10EF5D5AB8F41CFC9C358580396430872480FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGSxK+6r6oBEzDc27m+Wf7w== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4980AE1EE624C80DBAF649E70F82AA8155D388A110735728BF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2] core: fix cur_L restoration on error throw 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" Igor, Thanks for the review! On 18.08.21, Igor Munkin wrote: > Sergey, > > Thanks for the patch! I'm very curious what's wrong with FreeBSD... The > first version is more portable, but since we're going to revert this > commit few patches later, I'm open to any implementation. > > LGTM, with some nits. > > On 18.08.21, Sergey Kaplun wrote: > > Implement cur_L restoration only for arm64 architecture, due to FreeBSD > > issue. > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6189-curL-v2 > > Issues: > > * https://github.com/tarantool/tarantool/issues/6189 > > * https://github.com/tarantool/tarantool/issues/6323 > > * https://github.com/tarantool/tarantool/issues/1516 > > > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6189-curL-v2 > > > > Enable test-run tests on arm64, Odroid with bump to show their > > coverage. > > Please, rebase on the current master: I believe CI should be green! Done. > > > > > P.S. this problem is JIT-related, however, when I turn on `jit.dump()` > > in CI [1], it is disappeared :(. Also, can't reproduce it inside > > sh4/sh8 VM, test fails only in the CI. Red test-run.py suite due to > > fiber.top issue, see also [2]. > > Mystery. Again... Who knows, maybe you hit the root cause, why this hack > with cur_L restoring is forbidden. > > > > > I suppose it would be nice to have a FreeBSD test machine like we have > > for M1 and Odroid. It may be helpful to research the console issue [3] > > too. > > Definitely. Glad Kirill is also in this thread :) > Fixed commit message is the following: =================================================================== arm64: fix cur_L restoration on error throw This change is the follow-up of commits ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 ('Update cur_L on exceptional path') and 97699d9ee2467389b6aea21a098e38aff3469b5f ('Fix cur_L tracking on exceptional path'). When an error is thrown on the coroutine that is not the one being currently executed, `cur_L` is not set up. Hence, when the running trace exits at assertion guard right after the error is caught, Lua state is restored from the incorrect `cur_L`. As a result the resulting stack is inconsistent and the crash occurs. Aforementioned patches fix the behaviour only for x86/x64 architectures. This patch updates the `cur_L` for arm64 architecture too. Nevertheless, throwing an error at non-currently executed coroutine is a violation of Lua/C API. So, in the nearest possible future this patch should be replaced with the corresponding assert in `lj_err_throw()`. Resolves tarantool/tarantool#6189 Relates to tarantool/tarantool#6323 Follows up tarantool/tarantool#1516 =================================================================== > > > > =================================================================== > > commit 0f555bf79fefa1016849577500aec52719378ca5 > > Author: Sergey Kaplun > > Date: Sun Aug 15 15:47:13 2021 +0300 > > > > arm64: fix cur_L restoration on error throw > > > > This change is a kind of follow-up of commits > > Minor: not kind of, but just follow-up. Fixed. > > > ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 ('Update cur_L on exceptional > > path') and 97699d9ee2467389b6aea21a098e38aff3469b5f ('Fix cur_L tracking > > on exceptional path'). > > > > When an error is thrown on the coroutine that is not the one being > > currently executed, `cur_L` is not set up. Hence, when the running trace > > exits at assertion guard right after the error is caught, Lua state is > > restored from the incorrect `cur_L`. As a result the resulting stack is > > inconsistent and the crash occurs. > > > > Aforementioned patches fix the behaviour only for x86/x64 architectures. > > This patch updates the `cur_L` for arm64 architecture too. > > > > Nevertheless, throwing an error at non-currently executed coroutine is a > > violation of Lua/C API. So, in the nearest possible future this patch > > Minor: It would be great to refer Roberto's answer. Feel free to ignore, > since anyone can find it in your PR in LuaJIT repo. It's just confirming Mike's answer. Ignoring. > > > should be replaced within the corresponding assert in `lj_err_throw()`. > > Typo: s/within/with/. Fixed. > > > > > Resolves tarantool/tarantool#6189 > > Relates to tarantool/tarantool#6323 > > Follows up tarantool/tarantool#1516 > > > > > > > diff --git a/test/tarantool-tests/gh-6189-cur_L.test.lua b/test/tarantool-tests/gh-6189-cur_L.test.lua > > new file mode 100644 > > index 00000000..8521af9a > > --- /dev/null > > +++ b/test/tarantool-tests/gh-6189-cur_L.test.lua > > @@ -0,0 +1,25 @@ > > +local libcur_L = require('libcur_L') > > +local tap = require('tap') > > + > > +local test = tap.test('gh-6189-cur_L') > > +test:plan(1) > > + > > +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') > > +cbool(true) > > +cbool(true) > > Minor: Please add a comment why two calls are needed. Added. > > > + > > +pcall(libcur_L.error_from_other_thread) > > Minor: It would be nice to add an assert that pcall yields false here. Added. =================================================================== diff --git a/test/tarantool-tests/gh-6189-cur_L.test.lua b/test/tarantool-tests/gh-6189-cur_L.test.lua index 8521af9a..7f2184ec 100644 --- a/test/tarantool-tests/gh-6189-cur_L.test.lua +++ b/test/tarantool-tests/gh-6189-cur_L.test.lua @@ -14,10 +14,13 @@ 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) -pcall(libcur_L.error_from_other_thread) +assert(pcall(libcur_L.error_from_other_thread) == false, "return from error") -- Call with restoration from a snapshot with wrong cur_L. cbool(false) =================================================================== > > > +-- Call with restoration from a snapshot with wrong cur_L. > > +cbool(false) > > + > > +test:ok(true) > > +os.exit(test:check() and 0 or 1) > > > > > diff --git a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c > > new file mode 100644 > > index 00000000..2d58d2e7 > > --- /dev/null > > +++ b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c > > > > > +static int error_from_other_thread(lua_State *L) > > +{ > > + lua_State *next_cur_L = lua_newthread(L); > > + old_L = L; > > + /* Remove thread. */ > > + lua_pop(L, 1); > > + /* Do not show frame slot as return result after error. */ > > + lua_pushnil(L); > > + lua_pushcfunction(next_cur_L, throw_error_at_old_thread); > > + lua_call(next_cur_L, 0, 0); > > + /* Unreachable. */ > > Then it's worth to add an assert here to be sure we never return here. Added. =================================================================== diff --git a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c index 2d58d2e7..e39b607d 100644 --- a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c +++ b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c @@ -1,6 +1,9 @@ #include #include +#undef NDEBUG +#include + static lua_State *old_L = NULL; int throw_error_at_old_thread(lua_State *cur_L) @@ -21,6 +24,7 @@ static int error_from_other_thread(lua_State *L) lua_pushcfunction(next_cur_L, throw_error_at_old_thread); lua_call(next_cur_L, 0, 0); /* Unreachable. */ + assert(0); return 0; } =================================================================== > > > + return 0; > > +} > > + > > > > > > =================================================================== > > > > [1]: https://github.com/tarantool/tarantool/runs/3349429293#step:5:4569 > > [2]: https://github.com/tarantool/tarantool/pull/6303 > > [3]: https://github.com/tarantool/tarantool/issues/6231 > > > > -- > > Best regards, > > Sergey Kaplun > > -- > Best regards, > IM -- Best regards, Sergey Kaplun