From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 Date: Tue, 11 May 2021 11:28:21 +0300 [thread overview] Message-ID: <20210511082821.GB3944@tarantool.org> (raw) In-Reply-To: <YJotgpsSTpftjoRi@root> 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@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@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
next prev parent reply other threads:[~2021-05-11 8:49 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-04 8:29 Igor Munkin via Tarantool-patches 2021-05-11 7:08 ` Sergey Kaplun via Tarantool-patches 2021-05-11 8:28 ` Igor Munkin via Tarantool-patches [this message] 2021-05-14 13:13 ` Sergey Ostanevich via Tarantool-patches 2021-05-17 8:39 ` Igor Munkin via Tarantool-patches 2021-05-17 16:34 ` Igor Munkin via Tarantool-patches 2021-05-18 7:14 ` Sergey Kaplun via Tarantool-patches 2021-05-18 15:34 ` Sergey Ostanevich via Tarantool-patches 2021-05-19 13:04 ` Igor Munkin via Tarantool-patches 2021-05-19 12:19 ` Igor Munkin via Tarantool-patches 2021-05-19 16:55 ` Sergey Kaplun via Tarantool-patches 2021-05-19 17:20 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210511082821.GB3944@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox