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

Tue May 11 11:28:21 MSK 2021

Sergey,

Thanks for your review!

On 11.05.21, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM, with a few ignorable questions.

Added your tag (but you can revoke it, considering the answers below):
| Reviewed-by: Sergey Kaplun <skaplun at tarantool.org>

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

Well, I don't know how to write it in a common way: if I understand
everything right, there should be one sentence per issue, so in the
commit closing #5983 we can simply add a oneliner with something like:
| Support M1 blah-blah-blah (gh-5983).

Thanks to GitHub, we can group all related changes via issue mentions,
so if one really needs to see the list of the changes, he can surf
through the issue.

Anyway, this is only my personal opinion, so I'll ask other maintainers
in our core chat.

> 
> Feel free to ignore.

Ignoring for now.

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

Honestly, I don't know what "preliminary" means in this way. Moreover,
if this has been already declared, then this is a separate bug to be
fixed via another commit (at least). If one requires jit.dump on RPi,
please file an issue and fix it. I believe, this is definitely out of
the M1 support scope.

>                                             Also, as far as we support
> 64-bit arm architecture should we add the <dis_arm64be.lua> too?

I have only two stands for the tests:
* M1 machintoch:
| $ uname -mrs
| Darwin 20.3.0 arm64
* Odroid Linux:
| $ uname -msr
| Linux 4.9.213-67 aarch64

AFAIU, both of these hosts are LE. Omitting the fact this is not related
to M1 support again, I have no machine to test this (CI job in other
words). So, apply the reasoning related to 32-bit ARM also to this case.

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

Again, no money -- no honey: I see no reason to support something, that
can't be tested. However, nobody can stop you from fixing this.

All these questions are good, but the only reason why I sent this patch
to ml -- I'm tired of cherry-picking it into my working branches to fix
JIT issue. This is why it's so simple and small. If you think this
should be done in another way, then let's discard this patch, until you
implement it the way you want. For me it will be much more expensive
than continue cherry-picking this.

> 
> Feel free to ignore.

Ignoring.

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

No no no, please, no! Please, imagine what a CMake if/elseif/else mess
it would be? Furthermore, we needed to wrap the code below with #ifdef,
since only arch-specific files would be preprocessed via CMake.
Actually, you can file an issue, if this bothers you, but I doubt we
need such fine tuning ever.

> 
> Feel free to ignore.

Ignoring.

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

-- 
Best regards,
IM