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 6E2536ECCC; Mon, 1 Aug 2022 12:39:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6E2536ECCC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1659346752; bh=CJ3QIGT8Viei35RvBBVPtAtRJWvwQyt/qf2CTanVqBA=; h=In-Reply-To:Date:References:To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=VdhwcZqElj58tt+BUNuZ8NW6i3GvX3wO3vHYKToSr8FtweOJXAOPPPZKfwC6y6Cbs Axue6CNCoZVSHT97VKD3RInxtx7dmdTxPrdI47jXcb6ZJeA07jeEvgZ61QF1K7nOAH DLLSTZLGjXEGmmFexa6VCfUkbZaMtzyCAhojDOrA= Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 E1B536ECCC for ; Mon, 1 Aug 2022 12:39:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E1B536ECCC Received: by smtp57.i.mail.ru with esmtpa (envelope-from ) id 1oIRtF-0001OY-WD; Mon, 01 Aug 2022 12:39:10 +0300 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) In-Reply-To: <54b6d24e217454e632c9a0e8dbf29dee1c37601a.1659264154.git.skaplun@tarantool.org> Date: Mon, 1 Aug 2022 12:39:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <452CFBEC-6B42-4049-94C7-B1899E008E61@tarantool.org> References: <54b6d24e217454e632c9a0e8dbf29dee1c37601a.1659264154.git.skaplun@tarantool.org> To: Sergey Kaplun X-Mailer: Apple Mail (2.3696.120.41.1.1) X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9626C4810127D410704E254B5F4661BBD0CF335CAC0D0286A182A05F538085040D5D18C51C4DC429DC819AAA2F96C161AF9ABCCDF401E54E6F81DEEB39643862D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70EEC24FFE855BCBBC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7F5B0FEFE70B13B25EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B8859CA687ABA27BA8C31438EF55DC7B0A62E2104AD212390CC7F00164DA146DAFE8445B8C89999728AA50765F7900637A596F99EC23ACBE8389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8EDCF5861DED71B2F389733CBF5DBD5E9B5C8C57E37DE458BD9DD9810294C998ED8FC6C240DEA76428AA50765F79006374FFCCBB9D4B53A2AD81D268191BDAD3DBD4B6F7A4D31EC0BEA7A3FFF5B025636D81D268191BDAD3D78DA827A17800CE711EF62B15521B799EC76A7562686271EEC990983EF5C03292E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3727597FF642BA4D735872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: 4965CFDFE0519134C1FE400A9E48C5401DD40DE57556AFB266D16FC5F53507A1816E0A2A8F779BBED8D40077074E805C66D16FC5F53507A117535B0CF9F6D0C3EE9D5CB6078CC77C434E0BD8A1EC3AFA135824B3FEAB4495 X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0CEC15DA3DDAC97276791A85E4603CDE21CC9ACD28CA1D6009B8FA2C38AF372E89C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3475AE8838CC1BF58E7A9A5DE8D15162C9F4212092F53D6D94F76092B4CFA998AF96FF63D1B52EC82B1D7E09C32AA3244C978A6B371E2F0B56A9B2FA90A7F75FC8725D5B54B2FE4575FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPX9Kc2YQ0o34id4uLTJzmw== X-Mailru-Sender: 5AA3D5B9D8C4864612F3A0B4F632CB49F5EAF4BA4361C7DDF83C7499C86F7FFF133F8D14414CA7C019381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit. 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: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! With some updates to the comments LGTM. Sergos > On 31 Jul 2022, at 13:58, Sergey Kaplun wrote: >=20 > From: Mike Pall >=20 > (cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01) >=20 > This commit changes usage of `lj_err_throw` to `lj_err_run()` in use > `lj_vm_exit_interp()` for not GC64 VMs, so the corresponding error non-GC64 > handler for `xpcall()` is called after trace exit. It allows to avoid > calling of error handler, when restoration from a snapshot is failed = due an error handler call > to the Lua stack overflow. This type of error can be handled by the > compiler itself and user shouldn't worry/know about them. The plural =E2=80=99them=E2=80=99 needs an =E2=80=98Errors of this = type=E2=80=99 in the beginning. >=20 > Also, this makes behaviour and code base for GC64 and not GC64 VMs > consistent. >=20 > Sergey Kaplun: > * added the description and the test for the problem >=20 > Part of tarantool/tarantool#7230 > --- >=20 > The test for this patch is very fragile: if the Lua stack layout is > changed (for example smbd adds a one more parameter in `luaT_call()`) > the test will fail as far as some error not during snapshot = restoration > will raised. So this particular test is skipped for the Tarantool. > Also, it is skipped for *BSD as far as there are no traces compiled = [1], > so there is no restoration from a snapshot, so the errfunc is called. >=20 > [1]: https://github.com/tarantool/tarantool/issues/4819 >=20 > src/lj_debug.c | 1 + > src/lj_dispatch.h | 2 +- > src/lj_err.c | 2 +- > src/lj_err.h | 2 +- > src/lj_trace.c | 4 ++-- > src/vm_arm.dasc | 3 +-- > src/vm_mips.dasc | 5 ++-- > src/vm_ppc.dasc | 3 +-- > src/vm_x86.dasc | 4 +--- > .../lj-603-err-snap-restore.test.lua | 23 ++++++++++++++++++- > 10 files changed, 33 insertions(+), 16 deletions(-) >=20 > diff --git a/src/lj_debug.c b/src/lj_debug.c > index 8eb5983b..654dc913 100644 > --- a/src/lj_debug.c > +++ b/src/lj_debug.c > @@ -93,6 +93,7 @@ static BCPos debug_framepc(lua_State *L, GCfunc *fn, = cTValue *nextframe) > } > } > ins =3D cframe_pc(cf); > + if (!ins) return NO_BCPOS; > } > } > pt =3D funcproto(fn); > diff --git a/src/lj_dispatch.h b/src/lj_dispatch.h > index 5bda51a2..addf5572 100644 > --- a/src/lj_dispatch.h > +++ b/src/lj_dispatch.h > @@ -46,7 +46,7 @@ extern double __divdf3(double a, double b); > _(asin) _(acos) _(atan) _(sinh) _(cosh) _(tanh) _(frexp) _(modf) = _(atan2) \ > _(pow) _(fmod) _(ldexp) _(lj_vm_modi) \ > _(lj_dispatch_call) _(lj_dispatch_ins) _(lj_dispatch_stitch) \ > - _(lj_dispatch_profile) _(lj_err_throw) \ > + _(lj_dispatch_profile) _(lj_err_throw) _(lj_err_run) \ Wrong alpha sorting of function names? > _(lj_ffh_coroutine_wrap_err) _(lj_func_closeuv) _(lj_func_newL_gc) \ > _(lj_gc_barrieruv) _(lj_gc_step) _(lj_gc_step_fixtop) = _(lj_meta_arith) \ > _(lj_meta_call) _(lj_meta_cat) _(lj_meta_comp) _(lj_meta_equal) \ > diff --git a/src/lj_err.c b/src/lj_err.c > index b520b3d3..c310daf6 100644 > --- a/src/lj_err.c > +++ b/src/lj_err.c > @@ -602,7 +602,7 @@ static ptrdiff_t finderrfunc(lua_State *L) > } >=20 > /* Runtime error. */ > -LJ_NOINLINE void lj_err_run(lua_State *L) > +LJ_NOINLINE void LJ_FASTCALL lj_err_run(lua_State *L) > { > ptrdiff_t ef =3D finderrfunc(L); > if (ef) { > diff --git a/src/lj_err.h b/src/lj_err.h > index cba5fb71..aa4b7e0d 100644 > --- a/src/lj_err.h > +++ b/src/lj_err.h > @@ -23,7 +23,7 @@ LJ_DATA const char *lj_err_allmsg; > LJ_FUNC GCstr *lj_err_str(lua_State *L, ErrMsg em); > LJ_FUNCA_NORET void LJ_FASTCALL lj_err_throw(lua_State *L, int = errcode); > LJ_FUNC_NORET void lj_err_mem(lua_State *L); > -LJ_FUNC_NORET void lj_err_run(lua_State *L); > +LJ_FUNCA_NORET void LJ_FASTCALL lj_err_run(lua_State *L); > LJ_FUNC_NORET void lj_err_msg(lua_State *L, ErrMsg em); > LJ_FUNC_NORET void lj_err_lex(lua_State *L, GCstr *src, const char = *tok, > BCLine line, ErrMsg em, va_list argp); > diff --git a/src/lj_trace.c b/src/lj_trace.c > index 68a657a7..bae4ba14 100644 > --- a/src/lj_trace.c > +++ b/src/lj_trace.c > @@ -802,8 +802,8 @@ typedef struct ExitDataCP { > static TValue *trace_exit_cp(lua_State *L, lua_CFunction dummy, void = *ud) > { > ExitDataCP *exd =3D (ExitDataCP *)ud; > - cframe_errfunc(L->cframe) =3D -1; /* Inherit error function. */ > - /* Always catch error here. */ > + /* Always catch error here and don't call error function. */ > + cframe_errfunc(L->cframe) =3D 0; > cframe_nres(L->cframe) =3D -2*LUAI_MAXSTACK*(int)sizeof(TValue); > exd->pc =3D lj_snap_restore(exd->J, exd->exptr); > UNUSED(dummy); > diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc > index 21f7fecb..a29292f1 100644 > --- a/src/vm_arm.dasc > +++ b/src/vm_arm.dasc > @@ -2247,9 +2247,8 @@ static void build_subroutines(BuildCtx *ctx) > | b <2 > | > |9: // Rethrow error from the right C frame. > - | rsb CARG2, CARG1, #0 > | mov CARG1, L > - | bl extern lj_err_throw // (lua_State *L, int errcode) > + | bl extern lj_err_run // (lua_State *L) > |.endif > | > = |//-----------------------------------------------------------------------= > diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc > index ec57d789..93c772ff 100644 > --- a/src/vm_mips.dasc > +++ b/src/vm_mips.dasc > @@ -2512,9 +2512,8 @@ static void build_subroutines(BuildCtx *ctx) > |. addu RA, RA, BASE > | > |9: // Rethrow error from the right C frame. > - | load_got lj_err_throw > - | negu CARG2, CRET1 > - | call_intern lj_err_throw // (lua_State *L, int = errcode) > + | load_got lj_err_run > + | call_intern lj_err_run // (lua_State *L) > |. move CARG1, L > |.endif > | > diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc > index 3f48b7ff..980176a2 100644 > --- a/src/vm_ppc.dasc > +++ b/src/vm_ppc.dasc > @@ -2707,9 +2707,8 @@ static void build_subroutines(BuildCtx *ctx) > | bctr > | > |9: // Rethrow error from the right C frame. > - | neg CARG2, CARG1 > | mr CARG1, L > - | bl extern lj_err_throw // (lua_State *L, int errcode) > + | bl extern lj_err_run // (lua_State *L) > |.endif > | > = |//-----------------------------------------------------------------------= > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc > index 92140cec..22f69edf 100644 > --- a/src/vm_x86.dasc > +++ b/src/vm_x86.dasc > @@ -3040,10 +3040,8 @@ static void build_subroutines(BuildCtx *ctx) > | jmp <2 > | > |9: // Rethrow error from the right C frame. > - | neg RD > | mov FCARG1, L:RB > - | mov FCARG2, RD > - | call extern lj_err_throw@8 // (lua_State *L, int = errcode) > + | call extern lj_err_run@4 // (lua_State *L) > |.endif > | > = |//-----------------------------------------------------------------------= > diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua = b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > index 82ce6a8f..5cb487c3 100644 > --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua > @@ -4,11 +4,27 @@ local tap =3D require('tap') > -- error is raised on restoration from the snapshot. > -- See also https://github.com/LuaJIT/LuaJIT/issues/603. > local test =3D tap.test('lj-603-err-snap-restore.test.lua') > -test:plan(1) > +test:plan(2) >=20 > +-- XXX: This is fragile. We need a specific amount of Lua stack > +-- slots is used to the error on restoration from a snapshot is XXX ^ cause XXX > +-- raised and error handler isn't called according to the new XXXXXXXX ^ without an ^^^^^^^^^^^ call > +-- behaviour. With another amount of used stack slots another one Different ^ can raise =20 > +-- error may be raised (`LJ_ERR_STKOV` ("stack overflow") during XXXXXXXXXXXX > +-- growing stack while trying to push error message, > +-- `LJ_ERR_ERRERR` ("error in error handling"), etc.). > +-- This amount is suited well for GC64 and not GC64 mode. > +-- luacheck: no unused > +local _, _, _, _, _, _ > + > +local handler_is_called =3D false > local recursive_f > local function errfunc() > xpcall(recursive_f, errfunc) > + -- Since this error is occured on snapshot restoration and may can be > + -- handled by compiler itself, we shouldn't bother a user with > + -- it. > + handler_is_called =3D true > end >=20 > -- A recursive call to itself leads to trace with up-recursion. > @@ -22,6 +38,11 @@ end > recursive_f() >=20 > test:ok(true) > +-- Disabled on *BSD due to #4819. > +-- XXX: The different amount of stack slots is in-use for > +-- Tarantool at start, so just skip test for it. > +-- luacheck: no global > +test:ok(jit.os =3D=3D 'BSD' or _TARANTOOL or not handler_is_called) >=20 > -- XXX: Don't use `os.exit()` here intense. When error on snap ??? just drop? > -- restoration is raised, `err_unwind()` doesn't stop on correct unfinished phrase? > --=20 > 2.34.1 >=20