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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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