Re: [Tarantool-patches] [PATCH v2] lua: assert on lua_gettop() negative return value

[Igor Munkin <imun@tarantool.org>]

Ilya,

Thanks for the patch! As I wrote in the previous series, your assumption
is a bit insane. So if you're right it means the complete mess in the
internal state of the given coroutine.

There are little places where the pointers to top and base Lua stack
slots are invalidated, *but* all such occurrences are enclosed in VM.
These places are unreachable for the "client" code (e.g. via Lua C API).

However, since Tarantool bundles LuaJIT and <struct lua_State> is not
opaque anymore, there *might* be places where the mentioned pointers are
managed manually. I glanced the sources and failed to find top or base
pointer used as an assignment destination.

Besides, Tarantool doesn't respect the native way Lua coroutines switch.
We already faced several issues with JIT machinery caused by this
peculiarity (e.g. so called FFI sandwiches), so maybe your case also
relates to fiber switch specifics.

Considering everything above it looks more valuable to add the assertion
right to <lua_gettop> interface. Negative result violates Lua reference
manual[1], so it's an extra check the platform works fine. Thoughts?

On 21.07.20, Ilya Kosarev wrote:
> In case lua_gettop() called from encode_lua_call() returns negative
> value, we will segfault in iproto_reply_error() with empty diag, as far
> as it is unexpected error path not covered with diagnostics. Thus
> corresponding sane check with assert is introduced.
> 
> Closes #4649

Side note: I doubt this patch closes #4649. It just adds more
diagnostics but the doesn't fix the root issue. I guess "Relates to" tag
is much better here.

> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4649-sane-check-on-lua_gettop
> Issue: https://github.com/tarantool/tarantool/issues/4649
> 
> Changes in v2:
> - added reasoning in comment
> 
>  src/box/lua/call.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

<snipped>

> -- 
> 2.17.1
> 

-- 
Best regards,
IM