Re: [Tarantool-patches] [PATCH luajit v2] core: fix cur_L restoration on error throw

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

Igor,

Thanks for the review!

On 18.08.21, Igor Munkin wrote:
> 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!

Done.

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

Fixed commit message is the following:

===================================================================
arm64: fix cur_L restoration on error throw

This change is the follow-up of commits
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
should be replaced with the corresponding assert in `lj_err_throw()`.

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

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

Fixed.

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

It's just confirming Mike's answer.
Ignoring.

> 
> > should be replaced within the corresponding assert in `lj_err_throw()`.
> 
> Typo: s/within/with/.

Fixed.

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

Added.

> 
> > +
> > +pcall(libcur_L.error_from_other_thread)
> 
> Minor: It would be nice to add an assert that pcall yields false here.

Added.

===================================================================
diff --git a/test/tarantool-tests/gh-6189-cur_L.test.lua b/test/tarantool-tests/gh-6189-cur_L.test.lua
index 8521af9a..7f2184ec 100644
--- a/test/tarantool-tests/gh-6189-cur_L.test.lua
+++ b/test/tarantool-tests/gh-6189-cur_L.test.lua
@@ -14,10 +14,13 @@ end
 
 -- Compile function to trace with snapshot.
 jit.opt.start('hotloop=1')
+-- First call makes `cbool()` hot enough to be recorded next time.
 cbool(true)
+-- Second call records `cbool()` body (i.e. `if` branch). This is
+-- a root trace for `cbool()`.
 cbool(true)
 
-pcall(libcur_L.error_from_other_thread)
+assert(pcall(libcur_L.error_from_other_thread) == false, "return from error")
 -- Call with restoration from a snapshot with wrong cur_L.
 cbool(false)
 
===================================================================

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

Added.

===================================================================
diff --git a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c
index 2d58d2e7..e39b607d 100644
--- a/test/tarantool-tests/gh-6189-cur_L/libcur_L.c
+++ b/test/tarantool-tests/gh-6189-cur_L/libcur_L.c
@@ -1,6 +1,9 @@
 #include <lua.h>
 #include <lauxlib.h>
 
+#undef NDEBUG
+#include <assert.h>
+
 static lua_State *old_L = NULL;
 
 int throw_error_at_old_thread(lua_State *cur_L)
@@ -21,6 +24,7 @@ static int error_from_other_thread(lua_State *L)
         lua_pushcfunction(next_cur_L, throw_error_at_old_thread);
         lua_call(next_cur_L, 0, 0);
         /* Unreachable. */
+        assert(0);
         return 0;
 }
===================================================================
 
> 
> > +	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

-- 
Best regards,
Sergey Kaplun