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 8A88D6EC40; Wed, 18 Aug 2021 20:22:17 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8A88D6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629307337; bh=QwgZg0VDkua7rQrfVKAW83u3V5lYwrat/QsEHjGry98=; 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=BwTer7HC3bkyGquPFScAOLbIGf9DP4g04aBBqQlVGVU1fyIZ/l7oH3xm9dl8ekdPZ lYtrwHPXOacYl5vyFwEoD+fjqLfsBpwCX+rdWrWL20xZm5lSi8L0CtHtLByWSoD6Yj 96nOu1ayAxBWTfezYsW6c+YCO7kqyEgtPU0pPk1o= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 BDE0B6EC40 for ; Wed, 18 Aug 2021 20:22:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BDE0B6EC40 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mGPGZ-0003Xz-Dx; Wed, 18 Aug 2021 20:22:15 +0300 Date: Wed, 18 Aug 2021 19:57:05 +0300 To: Sergey Kaplun Message-ID: <20210818165705.GF5743@tarantool.org> References: <20210816101949.25035-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F5380850409FF0063655EB79EAF69965708C61BE87C59E30E158CB4F71626B89AE36394ABE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7EED5D2FAB4CEB1EDEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D07BBD2EBFB7BF888638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DBF1FA569B60903D5054EC63F4F56EAD117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735209ECD01F8117BC8BEA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CE0F3BA37685B2B9043847C11F186F3C59DAA53EE0834AAEE X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5050819DE2B1282BBA88E092B727425EB93 X-C1DE0DAB: 0D63561A33F958A51C1ED35DD30534D35B11CD72EF9A8DCCD4133A7DA1E56AC4D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75C29D03FC76C37677410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B8CF715B894FC711F4D5989F4103E82C9C77085D75A5C8323BA4EEAC4DDF929ADB2DA06F546997D51D7E09C32AA3244C5326BC065744326F22A1633F5B399A10F26BFA4C8A6946B8927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGSxK+6r6oBFo5hs3F5COmw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D7F55D3D3BCA4E73A0577E226D44909FEA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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! > > 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 :) > > =================================================================== > 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. > 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. > should be replaced within the corresponding assert in `lj_err_throw()`. Typo: s/within/with/. > > 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. > + > +pcall(libcur_L.error_from_other_thread) Minor: It would be nice to add an assert that pcall yields false here. > +-- 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. > + 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