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

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

Hi, Igor!

Thanks for the patch!

LGTM, with a few ignorable questions.

On 04.05.21, Igor Munkin 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 ++++++++++++++

Should we add the Changelog entry for this?
We can create a special arm64-related file where we will note all
arm64-related changes.

Feel free to ignore.

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

Also I have two questions related to he patch:

1) AFAIK (at least from the activity in the "Red chat"), we declare
preliminary support of 32-bit arm architecture, too. So, <dis_arm.lua>
should be added as prebuild module as well. Also, as far as we support
64-bit arm architecture should we add the <dis_arm64be.lua> too?
And while we are at it: maybe we should add all architectures at once,
to simplify adaptation for other architectures, if we need some? At
least, IINM, Mons and Vlad Grubov discussed oportunity of mips
architecture usage. As a bonus, there is no follow-ups of this patch
in the future.

Feel free to ignore.

2) We don't need to add x86 as a built-in module when we build Tarantool
for arm/arm64/... host. So we can just use the needed one.

Feel free to ignore.

> 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,
Sergey Kaplun