Re: [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit.

[sergos via Tarantool-patches <tarantool-patches@dev.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
>