[Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit.
Sergey Kaplun
skaplun at tarantool.org
Mon Aug 1 13:23:22 MSK 2022
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 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
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
More information about the Tarantool-patches
mailing list