Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergos,

Thanks for your review!

On 14.05.21, Sergey Ostanevich wrote:
> Thanks for the patch,
> 
> LGTM.

Added you tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> As for the plurality of dis modules - can we make decision at load time,
> same as in dis_x64? So we’d introduce a set of dis_<target>.lua in Tarantool
> sources that will make a check if target is valid and load the luajit one.
> Definitely - not in this patch.

I tried to follow your thoughts, but get lost... This "decision" is done
within jit/dump.lua: for low-level machinery it loads the necessary
dis_<arch>.lua module, where <arch> is defined by <jit.arch> variable.
Do you propose the same as Sergey: preprocess and compile only the
required dis_<arch>.lua into Tarantool binary considering the target
platform? Anyway, let's discuss this and file the issue if necessary.

> 
> > On 4 May 2021, at 11:29, Igor Munkin <imun@tarantool.org> wrote:
> > 
> > Since commit c9d88d5f48054d78727b587fef7567422cdc39a6 ('Fix #984: add
> > jit.* library to the binary') all required modules implemented in Lua
> > are bundled (i.e. compiled into the executable as a C literal) into
> > Tarantool binary. While making Tarantool work on ARM64 platforms, it
> > turned out the arch-specific module (namely, jit/dis_arm64.lua) is not
> > bundled. Within this patch the missing sources are added and jit.dump
> > loads fine on ARM64 hosts as a result.
> > 
> > Part of #5983
> > Relates to #5629
> > Follows up #984
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Branch: https://github.com/tarantool/tarantool/tree/imun/gh-5983-add-jit-dump-on-arm64
> > Issue: https://github.com/tarantool/tarantool/issues/5983
> > 
> > CI looks to be OK[1] except the known problems with ASAN[2].
> > 
> > [1]: https://github.com/tarantool/tarantool/commit/be184b2
> > [2]: https://github.com/tarantool/tarantool/issues/6031
> > 
> > src/CMakeLists.txt                                 |  1 +
> > src/lua/init.c                                     |  2 ++
> > .../gh-5983-jit-library-smoke-tests.test.lua       | 14 ++++++++++++++
> > 3 files changed, 17 insertions(+)
> > create mode 100755 test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > 
> > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> > index 9005a37d6..f7a776986 100644
> > --- a/src/CMakeLists.txt
> > +++ b/src/CMakeLists.txt
> > @@ -54,6 +54,7 @@ lua_source(lua_sources lua/swim.lua)
> > # LuaJIT jit.* library
> > lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bc.lua)
> > lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bcsave.lua)
> > +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dis_arm64.lua)
> > lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dis_x86.lua)
> > lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dis_x64.lua)
> > lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dump.lua)
> > diff --git a/src/lua/init.c b/src/lua/init.c
> > index 3358b7136..dfae4afb7 100644
> > --- a/src/lua/init.c
> > +++ b/src/lua/init.c
> > @@ -106,6 +106,7 @@ extern char strict_lua[],
> > 	vmdef_lua[],
> > 	bc_lua[],
> > 	bcsave_lua[],
> > +	dis_arm64_lua[],
> > 	dis_x86_lua[],
> > 	dis_x64_lua[],
> > 	dump_lua[],
> > @@ -167,6 +168,7 @@ static const char *lua_modules[] = {
> > 	"jit.vmdef", vmdef_lua,
> > 	"jit.bc", bc_lua,
> > 	"jit.bcsave", bcsave_lua,
> > +	"jit.dis_arm64", dis_arm64_lua,
> > 	"jit.dis_x86", dis_x86_lua,
> > 	"jit.dis_x64", dis_x64_lua,
> > 	"jit.dump", dump_lua,
> > diff --git a/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > new file mode 100755
> > index 000000000..ab42fbebf
> > --- /dev/null
> > +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > @@ -0,0 +1,14 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +
> > +local test = tap.test('gh-5983-jit-library-smoke-tests')
> > +test:plan(1)
> > +
> > +-- Just check whether all Lua sources related to jit.dump are
> > +-- bundled to the binary. Otherwise, loading jit.dump module
> > +-- raises an error that is handled via <pcall> and passed as a
> > +-- second argument to the assertion.
> > +test:ok(pcall(require, 'jit.dump'))
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.25.0
> > 
> 

-- 
Best regards,
IM