Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: sergos <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.
Date: Mon, 1 Aug 2022 13:04:57 +0300	[thread overview]
Message-ID: <YuelSbsrzBeLNpw5@root> (raw)
In-Reply-To: <BA9D940C-1922-4C36-8AB0-32E363EB2600@tarantool.org>

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

  reply	other threads:[~2022-08-01 10:07 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-31 10:58 [Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on " Sergey Kaplun via Tarantool-patches
2022-07-31 10:58 ` [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during " Sergey Kaplun via Tarantool-patches
2022-08-01  9:38   ` sergos via Tarantool-patches
2022-08-01 10:04     ` Sergey Kaplun via Tarantool-patches [this message]
2022-08-02 22:03   ` Igor Munkin via Tarantool-patches
2022-07-31 10:58 ` [Tarantool-patches] [PATCH luajit 2/2] Call error function on rethrow after trace exit Sergey Kaplun via Tarantool-patches
2022-08-01  9:39   ` sergos via Tarantool-patches
2022-08-01 10:23     ` Sergey Kaplun via Tarantool-patches
2022-08-10 14:36   ` Igor Munkin via Tarantool-patches
2022-08-10 14:35 ` [Tarantool-patches] [PATCH luajit 0/2] Fix handling errors on snapshot restore Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YuelSbsrzBeLNpw5@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 1/2] Fix handling of errors during snapshot restore.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox