[Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore

[Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore

[Sergey Kaplun via Tarantool-patches]

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-603-err-snap-restore-full-ci
Tarantool PR: https://github.com/tarantool/tarantool/pull/7499
Issues:
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/LuaJIT/LuaJIT/issues/703

Mike Pall (2):
  Fix handling of errors during snapshot restore.
  Call error function on rethrow after trace exit.

 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          | 51 +++++++++++++++++++
 10 files changed, 63 insertions(+), 14 deletions(-)
 create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8)

When an error is raised during snapshot restore, `err_unwind()` skipped the
correct cframe to stop unwinding. It happens due this frame is C frame
without Lua frame and the special negative value of `cfram_nres()` for
this frame isn't set.

This patch sets `cframe_nres()` for cframe with snap restoration to
`-2*LUAI_MAXSTACK` to guarantee that an error will be always caught
here.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#7230
---
 src/lj_trace.c                                |  2 ++
 .../lj-603-err-snap-restore.test.lua          | 30 +++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua

diff --git a/src/lj_trace.c b/src/lj_trace.c
index d7a78d4d..68a657a7 100644
--- a/src/lj_trace.c
+++ b/src/lj_trace.c
@@ -803,6 +803,8 @@ 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. */
+  cframe_nres(L->cframe) = -2*LUAI_MAXSTACK*(int)sizeof(TValue);
   exd->pc = lj_snap_restore(exd->J, exd->exptr);
   UNUSED(dummy);
   return NULL;
diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
new file mode 100644
index 00000000..82ce6a8f
--- /dev/null
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -0,0 +1,30 @@
+local tap = require('tap')
+
+-- Test file to demonstrate the incorrect JIT behaviour when an
+-- 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)
+
+local recursive_f
+local function errfunc()
+  xpcall(recursive_f, errfunc)
+end
+
+-- A recursive call to itself leads to trace with up-recursion.
+-- When the Lua stack can't be grown more, error is raised on
+-- restoration from the snapshot.
+recursive_f = function()
+  xpcall(recursive_f, errfunc)
+  errfunc = function() end
+  recursive_f = function() end
+end
+recursive_f()
+
+test:ok(true)
+
+-- XXX: Don't use `os.exit()` here intense. When error on snap
+-- restoration is raised, `err_unwind()` doesn't stop on correct
+-- cframe. So later, on exit from VM this corrupted cframe chain
+-- shows itself. `os.exit()` literally calls `exit()` and doesn't
+-- show the issue.
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!
With some comments fixes LGTM.

Sergos


> On 31 Jul 2022, at 13:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> (cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8)
> 
> When an error is raised during snapshot restore, `err_unwind()` skipped the
                                the               the
> correct cframe to stop unwinding. It happens due this frame is C frame
                that should stop   The reason is                a     
> without Lua frame and the special negative value of `cfram_nres()` for
        ^^^^^^^                                            ^e
       not sure if I got it right - wrapping frame? Anyways an article is missing

> this frame isn't set.
> 
> This patch sets `cframe_nres()` for cframe with snap restoration to
                the                 a       that contains a
> `-2*LUAI_MAXSTACK` to guarantee that an error will be always caught
> here.
Not quite clear why this should be done always, since you mentioned before the
Lua frame presence mitigates the problem. Does it mean the cframe_nres() is
ignored if Lua frame is present? Mention it if it is true.

> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7230
> ---
> src/lj_trace.c                                |  2 ++
> .../lj-603-err-snap-restore.test.lua          | 30 +++++++++++++++++++
> 2 files changed, 32 insertions(+)
> create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua
> 
> diff --git a/src/lj_trace.c b/src/lj_trace.c
> index d7a78d4d..68a657a7 100644
> --- a/src/lj_trace.c
> +++ b/src/lj_trace.c
> @@ -803,6 +803,8 @@ 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. */
> +  cframe_nres(L->cframe) = -2*LUAI_MAXSTACK*(int)sizeof(TValue);
>   exd->pc = lj_snap_restore(exd->J, exd->exptr);
>   UNUSED(dummy);
>   return NULL;
> diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> new file mode 100644
> index 00000000..82ce6a8f
> --- /dev/null
> +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> @@ -0,0 +1,30 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate the incorrect JIT behaviour when an
           ??? 

> +-- 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)
> +
> +local recursive_f
> +local function errfunc()
> +  xpcall(recursive_f, errfunc)
> +end
> +
> +-- A recursive call to itself leads to trace with up-recursion.
> +-- When the Lua stack can't be grown more, error is raised on
> +-- restoration from the snapshot.
> +recursive_f = function()
> +  xpcall(recursive_f, errfunc)
> +  errfunc = function() end
> +  recursive_f = function() end
> +end
> +recursive_f()
> +
> +test:ok(true)
> +
> +-- XXX: Don't use `os.exit()` here intense. When error on snap
                                      ^^^ explicitly? | by intention?
                                            
> +-- restoration is raised, `err_unwind()` doesn't stop on correct
> +-- cframe. So later, on exit from VM this corrupted cframe chain
> +-- shows itself. `os.exit()` literally calls `exit()` and doesn't
> +-- show the issue.
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 01.08.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> With some comments fixes LGTM.
> 
> Sergos
> 
> 
> > On 31 Jul 2022, at 13:58, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > (cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8)
> > 
> > When an error is raised during snapshot restore, `err_unwind()` skipped the
>                                 the               the
> > correct cframe to stop unwinding. It happens due this frame is C frame
>                 that should stop   The reason is                a     
> > without Lua frame and the special negative value of `cfram_nres()` for
>         ^^^^^^^                                            ^e
>        not sure if I got it right - wrapping frame? Anyways an article is missing
> 
> > this frame isn't set.
> > 
> > This patch sets `cframe_nres()` for cframe with snap restoration to
>                 the                 a       that contains a
> > `-2*LUAI_MAXSTACK` to guarantee that an error will be always caught
> > here.
> Not quite clear why this should be done always, since you mentioned before the
> Lua frame presence mitigates the problem. Does it mean the cframe_nres() is
> ignored if Lua frame is present? Mention it if it is true.

Sorry, my bad wording. I've meant that this is just pseudo frame without
true honestly frame set up like it is done for `lua_cpcall()`. I've
reformulated the commit message as the following, considerring your
comments:


| Fix handling of errors during snapshot restore.
|
| (cherry picked from commit 12ab596997b9cb27846a5b254d11230c3f9c50c8)
|
| When an error is raised during a snapshot restore, the `err_unwind()`
| skips the correct cframe that should stop unwinding. The reason is this
| frame is the pseudo C frame for error handling without an actual frame
| and the special negative value of `cframe_nres()` for this frame isn't
| set.
|
| This patch sets `cframe_nres()` for a cframe that contains a snap
| restoration to `-2*LUAI_MAXSTACK` to guarantee that an error will be
| always caught here.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#7230

> 
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#7230
> > ---
> > src/lj_trace.c                                |  2 ++
> > .../lj-603-err-snap-restore.test.lua          | 30 +++++++++++++++++++
> > 2 files changed, 32 insertions(+)
> > create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua
> > 
> > diff --git a/src/lj_trace.c b/src/lj_trace.c
> > index d7a78d4d..68a657a7 100644
> > --- a/src/lj_trace.c
> > +++ b/src/lj_trace.c
> > @@ -803,6 +803,8 @@ 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. */
> > +  cframe_nres(L->cframe) = -2*LUAI_MAXSTACK*(int)sizeof(TValue);
> >   exd->pc = lj_snap_restore(exd->J, exd->exptr);
> >   UNUSED(dummy);
> >   return NULL;
> > diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> > new file mode 100644
> > index 00000000..82ce6a8f
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> > @@ -0,0 +1,30 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate the incorrect JIT behaviour when an
>            ??? 
> 
> > +-- error is raised on restoration from the snapshot.
> > +-- See also https://github.com/LuaJIT/LuaJIT/issues/603.

<snipped>

> > +
> > +test:ok(true)
> > +
> > +-- XXX: Don't use `os.exit()` here intense. When error on snap
>                                       ^^^ explicitly? | by intention?
>                                             
> > +-- restoration is raised, `err_unwind()` doesn't stop on correct
> > +-- cframe. So later, on exit from VM this corrupted cframe chain
> > +-- shows itself. `os.exit()` literally calls `exit()` and doesn't
> > +-- show the issue.

Fixed. See the iterative patch below.

===================================================================
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..d20c4d6a 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -1,7 +1,7 @@
 local tap = require('tap')
 
--- Test file to demonstrate the incorrect JIT behaviour when an
--- error is raised on restoration from the snapshot.
+-- Test to demonstrate the incorrect JIT behaviour when an 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)
@@ -23,8 +23,8 @@ recursive_f()
 
 test:ok(true)
 
--- XXX: Don't use `os.exit()` here intense. When error on snap
--- restoration is raised, `err_unwind()` doesn't stop on correct
--- cframe. So later, on exit from VM this corrupted cframe chain
--- shows itself. `os.exit()` literally calls `exit()` and doesn't
--- show the issue.
+-- XXX: Don't use `os.exit()` here by intention. When error on
+-- snap restoration is raised, `err_unwind()` doesn't stop on
+-- correct cframe. So later, on exit from VM this corrupted cframe
+-- chain shows itself. `os.exit()` literally calls `exit()` and
+-- doesn't show the issue.
===================================================================

> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM with the fixes for Sergos' review comments.

-- 
Best regards,
IM

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01)

This commit changes usage of `lj_err_throw` to `lj_err_run()` in
`lj_vm_exit_interp()` for not GC64 VMs, so the corresponding error
handler for `xpcall()` is called after trace exit. It allows to avoid
calling of error handler, when restoration from a snapshot is failed due
to the Lua stack overflow. This type of error can be handled by the
compiler itself and user shouldn't worry/know about them.

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) \
   _(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
+-- raised and error handler isn't called according to the new
+-- behaviour. With another amount of used stack slots another one
+-- error may be raised (`LJ_ERR_STKOV` ("stack overflow") during
+-- 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
+  -- 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
 -- restoration is raised, `err_unwind()` doesn't stop on correct
-- 
2.34.1

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

[sergos via Tarantool-patches]

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
> 

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 01.08.22, sergos wrote:
> 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

Fixed.

> 
> > `lj_vm_exit_interp()` for not GC64 VMs, so the corresponding error
>                            non-GC64

Fixed.

> 
> > 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

Fixed, thanks!

> 
> > 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.

Fixed, s/them/it/.

> 
> > 
> > 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
> > 

The new commit message is the following:


| Call error function on rethrow after trace exit.
|
| (cherry picked from commit e296f56b825c688c3530a981dc6b495d972f3d01)
|
| This commit changes use of `lj_err_throw` to `lj_err_run()` in the
| `lj_vm_exit_interp()` for non-GC64 VMs, so the corresponding error
| handler for `xpcall()` is called after trace exit. It allows to avoid
| an error handler call, when restoration from a snapshot is failed due
| to the Lua stack overflow. This type of error can be handled by the
| compiler itself and user shouldn't worry/know about it.
|
| 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

<snipped>

> > 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?

Yes, but this is consistent with upstream, so I left it as is.

> 
> >   _(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) \

<snipped>

> > 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.

Fixed comments cosidering your suggestions:

===================================================================
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 3c55b090..e9f48ec9 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -7,12 +7,12 @@ local test = tap.test('lj-603-err-snap-restore.test.lua')
 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
--- raised and error handler isn't called according to the new
--- behaviour. With another amount of used stack slots another one
--- error may be raised (`LJ_ERR_STKOV` ("stack overflow") during
--- growing stack while trying to push error message,
--- `LJ_ERR_ERRERR` ("error in error handling"), etc.).
+-- slots used to cause the error on restoration from a snapshot
+-- and without error handler call according to the new behaviour.
+-- Different amount of used stack slots can raise another one
+-- error (`LJ_ERR_STKOV` ("stack overflow") during 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 _, _, _, _, _, _
@@ -21,9 +21,9 @@ 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
-  -- handled by compiler itself, we shouldn't bother a user with
-  -- it.
+  -- Since this error is occured on snapshot restoration and can
+  -- be handled by compiler itself, we shouldn't bother a user
+  -- with it.
   handler_is_called = true
 end
 
===================================================================

> > +  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?

Fixed in the previous commit. (Not dropped, I suppose this is some
problem with mail client.)

> > -- restoration is raised, `err_unwind()` doesn't stop on correct
> 
> unfinished phrase?

This is just the old comment.

> 
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM with the fixes for Sergos' review comments.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, 2.10 and 1.10.

On 31.07.22, Sergey Kaplun wrote:
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-603-err-snap-restore-full-ci
> Tarantool PR: https://github.com/tarantool/tarantool/pull/7499
> Issues:
> * https://github.com/tarantool/tarantool/issues/7230
> * https://github.com/LuaJIT/LuaJIT/issues/703
> 
> Mike Pall (2):
>   Fix handling of errors during snapshot restore.
>   Call error function on rethrow after trace exit.
> 
>  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          | 51 +++++++++++++++++++
>  10 files changed, 63 insertions(+), 14 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-603-err-snap-restore.test.lua
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM