[Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit.
sergos
sergos at tarantool.org
Mon Aug 1 12:39:09 MSK 2022
Hi!
Thanks for the patch!
With some updates to the comments LGTM.
Sergos
> On 31 Jul 2022, at 13:58, Sergey Kaplun <skaplun at 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 at 8 // (lua_State *L, int errcode)
> + | call extern lj_err_run at 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
>
More information about the Tarantool-patches
mailing list