Re: [Tarantool-patches] [PATCH luajit 2/2] jit: abort trace execution on JIT mode change

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch, LGTM.

Sergos.

On 27 мар 13:47, Igor Munkin wrote:
> Current luaJIT_setmode implementation aborts trace recording but nothing
> prevents calling it on already compiled trace. E.g. if one conditionally
> calls an FFI function having luaJIT_setmode with LUAJIT_MODE_FLUSH mode
> underneath, the trace being executed can be purged and the return
> address is invalidated as a result (since the mcode is released).
> 
> This changeset prohibits luaJIT_setmode call while mcode is being
> executed leading to platform panic if the call occurs.
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  src/lj_dispatch.c                     |  5 +++++
>  src/lj_errmsg.h                       |  1 +
>  test/lj-flush-on-trace/CMakeLists.txt |  1 +
>  test/lj-flush-on-trace/libflush.c     | 31 +++++++++++++++++++++++++++
>  test/lj-flush-on-trace/test.lua       | 25 +++++++++++++++++++++
>  5 files changed, 63 insertions(+)
>  create mode 100644 test/lj-flush-on-trace/CMakeLists.txt
>  create mode 100644 test/lj-flush-on-trace/libflush.c
>  create mode 100644 test/lj-flush-on-trace/test.lua
> 
> diff --git a/src/lj_dispatch.c b/src/lj_dispatch.c
> index 5d6795f..b3448c8 100644
> --- a/src/lj_dispatch.c
> +++ b/src/lj_dispatch.c
> @@ -240,6 +240,11 @@ int luaJIT_setmode(lua_State *L, int idx, int mode)
>  {
>    global_State *g = G(L);
>    int mm = mode & LUAJIT_MODE_MASK;
> +  if (tvref(g->jit_base)) {
> +    setstrV(L, L->top++, lj_err_str(L, LJ_ERR_JITMODE));
> +    if (g->panic) g->panic(L);
> +    exit(EXIT_FAILURE);
> +  }
>    lj_trace_abort(g);  /* Abort recording on any state change. */
>    /* Avoid pulling the rug from under our own feet. */
>    if ((g->hookmask & HOOK_GC))
> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h
> index 1580385..de7b867 100644
> --- a/src/lj_errmsg.h
> +++ b/src/lj_errmsg.h
> @@ -113,6 +113,7 @@ ERRDEF(NOJIT,	"JIT compiler permanently disabled by build option")
>  #endif
>  ERRDEF(JITOPT,	"unknown or malformed optimization flag " LUA_QS)
>  ERRDEF(JITCALL,	"Lua VM re-entrancy is detected while executing the trace")
> +ERRDEF(JITMODE,	"JIT mode change is detected while executing the trace")
>  
>  /* Lexer/parser errors. */
>  ERRDEF(XMODE,	"attempt to load chunk with wrong mode")
> diff --git a/test/lj-flush-on-trace/CMakeLists.txt b/test/lj-flush-on-trace/CMakeLists.txt
> new file mode 100644
> index 0000000..a90452d
> --- /dev/null
> +++ b/test/lj-flush-on-trace/CMakeLists.txt
> @@ -0,0 +1 @@
> +build_lualib(libflush libflush.c)
> diff --git a/test/lj-flush-on-trace/libflush.c b/test/lj-flush-on-trace/libflush.c
> new file mode 100644
> index 0000000..177409a
> --- /dev/null
> +++ b/test/lj-flush-on-trace/libflush.c
> @@ -0,0 +1,31 @@
> +#include <lua.h>
> +#include <luajit.h>
> +
> +struct flush {
> +	lua_State *L; /* Coroutine saved to change JIT mode */
> +	int trigger;  /* Trigger for flushing all traces */
> +};
> +
> +void flush(struct flush *state, int i)
> +{
> +	if (i < state->trigger)
> +		return;
> +
> +	/* Trace flushing is triggered */
> +	(void)luaJIT_setmode(state->L, 0, LUAJIT_MODE_ENGINE|LUAJIT_MODE_FLUSH);
> +}
> +
> +static int init(lua_State *L)
> +{
> +	struct flush *state = lua_newuserdata(L, sizeof(struct flush));
> +
> +	state->L = L;
> +	state->trigger = lua_tonumber(L, 1);
> +	return 1;
> +}
> +
> +LUA_API int luaopen_libflush(lua_State *L)
> +{
> +	lua_pushcfunction(L, init);
> +	return 1;
> +}
> diff --git a/test/lj-flush-on-trace/test.lua b/test/lj-flush-on-trace/test.lua
> new file mode 100644
> index 0000000..ff6e0b6
> --- /dev/null
> +++ b/test/lj-flush-on-trace/test.lua
> @@ -0,0 +1,25 @@
> +local cfg = {
> +  hotloop = arg[1] or 1,
> +  trigger = arg[2] or 1,
> +}
> +
> +local ffi = require('ffi')
> +local ffiflush = ffi.load('libflush')
> +ffi.cdef('void flush(struct flush *state, int i)')
> +
> +-- Save the current coroutine and set the value to trigger ipp
> +-- call the Lua routine instead of pure C implementation.
> +local flush = require('libflush')(cfg.trigger)
> +
> +-- Depending on trigger and hotloop values the following contexts
> +-- are possible:
> +-- * if trigger <= hotloop -> trace recording is aborted
> +-- * if trigger >  hotloop -> trace is recorded but execution
> +--   leads to panic
> +jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop))
> +
> +for i = 0, cfg.trigger + cfg.hotloop do
> +  ffiflush.flush(flush, i)
> +end
> +-- Panic didn't occur earlier.
> +print('OK')
> -- 
> 2.25.0
>