From: sergos via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit. Date: Mon, 1 Aug 2022 12:39:09 +0300 [thread overview] Message-ID: <452CFBEC-6B42-4049-94C7-B1899E008E61@tarantool.org> (raw) In-Reply-To: <54b6d24e217454e632c9a0e8dbf29dee1c37601a.1659264154.git.skaplun@tarantool.org> Hi! Thanks for the patch! With some updates to the comments LGTM. Sergos > On 31 Jul 2022, at 13:58, Sergey Kaplun <skaplun@tarantool.org> wrote: > > From: Mike Pall <mike> > > (cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01) > > 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 ’them’ needs an ‘Errors of this type’ in the beginning. > > Also, this makes behaviour and code base for GC64 and not GC64 VMs > consistent. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#7230 > --- > > 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. > > [1]: https://github.com/tarantool/tarantool/issues/4819 > > 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(-) > > 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 = cframe_pc(cf); > + if (!ins) return NO_BCPOS; > } > } > pt = 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) > } > > /* 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 = 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 = (ExitDataCP *)ud; > - cframe_errfunc(L->cframe) = -1; /* Inherit error function. */ > - /* Always catch error here. */ > + /* Always catch error here and don't call error function. */ > + cframe_errfunc(L->cframe) = 0; > cframe_nres(L->cframe) = -2*LUAI_MAXSTACK*(int)sizeof(TValue); > exd->pc = 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 = require('tap') > -- error is raised on restoration from the snapshot. > -- See also https://github.com/LuaJIT/LuaJIT/issues/603. > local test = tap.test('lj-603-err-snap-restore.test.lua') > -test:plan(1) > +test:plan(2) > > +-- 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 > +-- 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 = 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 = true > end > > -- A recursive call to itself leads to trace with up-recursion. > @@ -22,6 +38,11 @@ end > recursive_f() > > 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 == 'BSD' or _TARANTOOL or not handler_is_called) > > -- 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? > -- > 2.34.1 >
next prev parent reply other threads:[~2022-08-01 9:39 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-31 10:58 [Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore Sergey Kaplun via Tarantool-patches 2022-07-31 10:58 ` [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during " Sergey Kaplun via Tarantool-patches 2022-08-01 9:38 ` sergos via Tarantool-patches 2022-08-01 10:04 ` Sergey Kaplun via Tarantool-patches 2022-08-02 22:03 ` Igor Munkin via Tarantool-patches 2022-07-31 10:58 ` [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit Sergey Kaplun via Tarantool-patches 2022-08-01 9:39 ` sergos via Tarantool-patches [this message] 2022-08-01 10:23 ` Sergey Kaplun via Tarantool-patches 2022-08-10 14:36 ` Igor Munkin via Tarantool-patches 2022-08-10 14:35 ` [Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=452CFBEC-6B42-4049-94C7-B1899E008E61@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergos@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox