From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A2F706F3C4; Tue, 11 May 2021 11:49:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A2F706F3C4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1620722977; bh=4YIVDarGwni4leqkFVWKW8qSnZHEkbYDpR7sSlQwKmY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=uml/tnKjsuLX4LzVO7bUnsjbloBkdiYUI+HGHWPlCOeeAbi/P2CNU2Kgz5mkVF4rd rADHzEvXnQs31XUdWY0Lu8lWBcNFpJ937D4KpI9vhG45EgIp4M1oCC0Ya9zW8PrMs6 BNMe/W9xBUGiowfm4wAdO/Lq4TFmXHe/7d/g/S8I= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 47C166F3C4 for ; Tue, 11 May 2021 11:49:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 47C166F3C4 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lgO58-0007oQ-TG; Tue, 11 May 2021 11:49:35 +0300 Date: Tue, 11 May 2021 11:28:21 +0300 To: Sergey Kaplun Message-ID: <20210511082821.GB3944@tarantool.org> References: <72c91a259dd039fc95961992ae06baee820695be.1620072340.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95978C26455E69BE0B2C7F7C3B0039F1303CE517DE434612D182A05F53808504025E4DEAF9E18B0E5AFF5BDB4D407548739B3B04D8688626F3C9EDFC5BDC99C9F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74FF5DF51D335CFFFEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378629C75C8EFA8C148638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2002E7D6AA347F78B8E0BD5C9F567458F98738A72DB41484ED2E47CDBA5A96583C09775C1D3CA48CF7EF884183F8E4D67117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE77E7E81EEA8A9722B8941B15DA834481F9449624AB7ADAF372E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C30E7A8B1B1FD51AB935872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5FA69EFBB000732700256657C38F7260B5425BF88AB5FDD2CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3416EA6E382A5BB176CB49B0637EF98D2982B5ECD0A60238683F6CAE4B5031BF3DCB482E0DD2ECE2071D7E09C32AA3244CB9DB6E567491FFF7901618CCE427F57A30363D8B7DA7DD44927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPN2Bz4bQWZucalSbKnv4qw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822DB919287E243E04A564852F5CE150968A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > 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 > > --- > > > > 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, > 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 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 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