Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit v2] core: fix cur_L restoration on error throw
Date: Wed, 18 Aug 2021 19:57:05 +0300	[thread overview]
Message-ID: <20210818165705.GF5743@tarantool.org> (raw)
In-Reply-To: <YRzJs+jloZZaB8oD@root>

Sergey,

Thanks for the patch! I'm very curious what's wrong with FreeBSD... The
first version is more portable, but since we're going to revert this
commit few patches later, I'm open to any implementation.

LGTM, with some nits.

On 18.08.21, Sergey Kaplun wrote:
> Implement cur_L restoration only for arm64 architecture, due to FreeBSD
> issue.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6189-curL-v2
> Issues:
> * https://github.com/tarantool/tarantool/issues/6189
> * https://github.com/tarantool/tarantool/issues/6323
> * https://github.com/tarantool/tarantool/issues/1516
> 
> Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6189-curL-v2
> 
> Enable test-run tests on arm64, Odroid with bump to show their
> coverage.

Please, rebase on the current master: I believe CI should be green!

> 
> P.S. this problem is JIT-related, however, when I turn on `jit.dump()`
> in CI [1], it is disappeared :(. Also, can't reproduce it inside
> sh4/sh8 VM, test fails only in the CI. Red test-run.py suite due to
> fiber.top issue, see also [2].

Mystery. Again... Who knows, maybe you hit the root cause, why this hack
with cur_L restoring is forbidden.

> 
> I suppose it would be nice to have a FreeBSD test machine like we have
> for M1 and Odroid. It may be helpful to research the console issue [3]
> too.

Definitely. Glad Kirill is also in this thread :)

> 
> ===================================================================
> commit 0f555bf79fefa1016849577500aec52719378ca5
> Author: Sergey Kaplun <skaplun@tarantool.org>
> Date:   Sun Aug 15 15:47:13 2021 +0300
> 
> arm64: fix cur_L restoration on error throw
> 
> This change is a kind of follow-up of commits

Minor: not kind of, but just follow-up.

> ed412cd9f55fe87fd32a69c86e1732690fc5c1b0 ('Update cur_L on exceptional
> path') and 97699d9ee2467389b6aea21a098e38aff3469b5f ('Fix cur_L tracking
> on exceptional path').
> 
> When an error is thrown on the coroutine that is not the one being
> currently executed, `cur_L` is not set up. Hence, when the running trace
> exits at assertion guard right after the error is caught, Lua state is
> restored from the incorrect `cur_L`. As a result the resulting stack is
> inconsistent and the crash occurs.
> 
> Aforementioned patches fix the behaviour only for x86/x64 architectures.
> This patch updates the `cur_L` for arm64 architecture too.
> 
> Nevertheless, throwing an error at non-currently executed coroutine is a
> violation of Lua/C API. So, in the nearest possible future this patch

Minor: It would be great to refer Roberto's answer. Feel free to ignore,
since anyone can find it in your PR in LuaJIT repo.

> should be replaced within the corresponding assert in `lj_err_throw()`.

Typo: s/within/with/.

> 
> Resolves tarantool/tarantool#6189
> Relates to tarantool/tarantool#6323
> Follows up tarantool/tarantool#1516
> 

<snipped>

> diff --git a/test/tarantool-tests/gh-6189-cur_L.test.lua b/test/tarantool-tests/gh-6189-cur_L.test.lua
> new file mode 100644
> index 00000000..8521af9a
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6189-cur_L.test.lua
> @@ -0,0 +1,25 @@
> +local libcur_L = require('libcur_L')
> +local tap = require('tap')
> +
> +local test = tap.test('gh-6189-cur_L')
> +test:plan(1)
> +
> +local function cbool(cond)
> +  if cond then
> +    return 1
> +  else
> +    return 0
> +  end
> +end
> +
> +-- Compile function to trace with snapshot.
> +jit.opt.start('hotloop=1')
> +cbool(true)
> +cbool(true)

Minor: Please add a comment why two calls are needed.

> +
> +pcall(libcur_L.error_from_other_thread)

Minor: It would be nice to add an assert that pcall yields false here.

> +-- Call with restoration from a snapshot with wrong cur_L.
> +cbool(false)
> +
> +test:ok(true)
> +os.exit(test:check() and 0 or 1)

<snipped>

> diff --git a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c
> new file mode 100644
> index 00000000..2d58d2e7
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c

<snipped>

> +static int error_from_other_thread(lua_State *L)
> +{
> +	lua_State *next_cur_L = lua_newthread(L);
> +	old_L = L;
> +	/* Remove thread. */
> +	lua_pop(L, 1);
> +	/* Do not show frame slot as return result after error. */
> +	lua_pushnil(L);
> +	lua_pushcfunction(next_cur_L, throw_error_at_old_thread);
> +	lua_call(next_cur_L, 0, 0);
> +	/* Unreachable. */

Then it's worth to add an assert here to be sure we never return here.

> +	return 0;
> +}
> +

<snipped>


> ===================================================================
> 
> [1]: https://github.com/tarantool/tarantool/runs/3349429293#step:5:4569
> [2]: https://github.com/tarantool/tarantool/pull/6303
> [3]: https://github.com/tarantool/tarantool/issues/6231
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

  reply	other threads:[~2021-08-18 17:22 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16 10:19 [Tarantool-patches] [PATCH luajit] " Sergey Kaplun via Tarantool-patches
2021-08-18  8:49 ` [Tarantool-patches] [PATCH luajit v2] " Sergey Kaplun via Tarantool-patches
2021-08-18 16:57   ` Igor Munkin via Tarantool-patches [this message]
2021-08-18 20:03     ` Sergey Kaplun via Tarantool-patches
2021-08-18 20:26       ` Igor Munkin via Tarantool-patches
2021-08-19  8:23   ` Igor Munkin via Tarantool-patches
2021-08-19  7:42 ` [Tarantool-patches] [PATCH luajit] " Kirill Yukhin via Tarantool-patches
2021-08-19  7:56   ` Vitaliia Ioffe 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=20210818165705.GF5743@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v2] core: fix cur_L restoration on error throw' \
    /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