Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
@ 2021-05-04  8:29 Igor Munkin via Tarantool-patches
  2021-05-11  7:08 ` Sergey Kaplun via Tarantool-patches
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-04  8:29 UTC (permalink / raw)
  To: Sergey Ostanevich, Sergey Kaplun; +Cc: tarantool-patches

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-04  8:29 [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 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
  2021-05-14 13:13 ` Sergey Ostanevich via Tarantool-patches
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-05-11  7:08 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-11  7:08 ` Sergey Kaplun via Tarantool-patches
@ 2021-05-11  8:28   ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-11  8:28 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 <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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-04  8:29 [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 Igor Munkin via Tarantool-patches
  2021-05-11  7:08 ` Sergey Kaplun via Tarantool-patches
@ 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-19 17:20 ` Igor Munkin via Tarantool-patches
  3 siblings, 1 reply; 12+ messages in thread
From: Sergey Ostanevich via Tarantool-patches @ 2021-05-14 13:13 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Thanks for the patch,

LGTM.

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.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-14 13:13 ` Sergey Ostanevich via Tarantool-patches
@ 2021-05-17  8:39   ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-17  8:39 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-04  8:29 [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 Igor Munkin via Tarantool-patches
  2021-05-11  7:08 ` Sergey Kaplun via Tarantool-patches
  2021-05-14 13:13 ` Sergey Ostanevich 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-19 17:20 ` Igor Munkin via Tarantool-patches
  3 siblings, 1 reply; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-17 16:34 UTC (permalink / raw)
  To: Sergey Ostanevich, Sergey Kaplun; +Cc: tarantool-patches

Oops, sorry guys, but the test seems to be noop for this fix (in other
works it's OK even without patch), so I've reimplemented it. Consider
the description in the test. Diff is below:

================================================================================

diff --git a/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
new file mode 100644
index 000000000..2a2ec4d97
--- /dev/null
+++ b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:
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..72caec2f9
--- /dev/null
+++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
@@ -0,0 +1,44 @@
+#!/usr/bin/env tarantool
+
+-- Just check whether all Lua sources related to jit.dump are
+-- bundled to the binary. Otherwise, jit.dump module raises
+-- an error that is handled via <pcall>.
+-- XXX: pure require for jit.dump doesn't cover all the cases,
+-- since dis_<arch>.lua are loaded at runtime. Furthermore, this
+-- error is handled by JIT engine, so we can't use <pcall> for it.
+-- Hence, simply sniff the output of the test to check that all
+-- phases of trace compilation are dumped.
+
+if #arg == 0 then
+  local tap = require('tap')
+  local test = tap.test('gh-5983-jit-library-smoke-tests')
+
+  test:plan(1)
+
+  -- XXX: Shell argument <test> is necessary to differ test case
+  -- from the test runner.
+  local cmd = string.gsub('<LUABIN> 2>&1 <SCRIPT> test', '%<(%w+)>', {
+      LUABIN = arg[-1],
+      SCRIPT = arg[0],
+  })
+  local proc = io.popen(cmd)
+  local got = proc:read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
+  local expected = table.concat({
+      '---- TRACE %d start',
+      '---- TRACE %d IR',
+      '---- TRACE %d mcode',
+      '---- TRACE %d stop',
+      '---- TRACE %d exit',
+  }, '.+')
+
+  test:like(got, expected , 'jit.dump smoke tests')
+
+  os.exit(test:check() and 0 or 1)
+end
+
+-- Use *all* jit.dump options, so the test can check them all.
+require('jit.dump').start('+tbisrmXaT')
+-- Tune JIT engine to make the test faster and more robust.
+jit.opt.start('hotloop=1')
+-- Record primitive loop.
+for _ = 1, 3 do end

================================================================================

CI is green[1] now.

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

[1]: https://github.com/tarantool/tarantool/commit/18a4018

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  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 12:19     ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 12+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-05-18  7:14 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Igor,

On 17.05.21, Igor Munkin wrote:
> Oops, sorry guys, but the test seems to be noop for this fix (in other
> works it's OK even without patch), so I've reimplemented it. Consider
> the description in the test. Diff is below:

Thanks for update!
Please consider my comments below.

Side note: Please, rebase your branch to master to make it buildable.

> 
> ================================================================================
> 
> diff --git a/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
> new file mode 100644
> index 000000000..2a2ec4d97
> --- /dev/null
> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
> @@ -0,0 +1,7 @@
> +import platform
> +
> +# Disabled on FreeBSD due to #4819.
> +if platform.system() == 'FreeBSD':
> +    self.skip = 1
> +
> +# vim: set ft=python:
> 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..72caec2f9
> --- /dev/null
> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> @@ -0,0 +1,44 @@
> +#!/usr/bin/env tarantool
> +
> +-- Just check whether all Lua sources related to jit.dump are
> +-- bundled to the binary. Otherwise, jit.dump module raises
> +-- an error that is handled via <pcall>.
> +-- XXX: pure require for jit.dump doesn't cover all the cases,
> +-- since dis_<arch>.lua are loaded at runtime. Furthermore, this
> +-- error is handled by JIT engine, so we can't use <pcall> for it.
> +-- Hence, simply sniff the output of the test to check that all
> +-- phases of trace compilation are dumped.
> +
> +if #arg == 0 then
> +  local tap = require('tap')
> +  local test = tap.test('gh-5983-jit-library-smoke-tests')
> +
> +  test:plan(1)
> +
> +  -- XXX: Shell argument <test> is necessary to differ test case
> +  -- from the test runner.
> +  local cmd = string.gsub('<LUABIN> 2>&1 <SCRIPT> test', '%<(%w+)>', {
> +      LUABIN = arg[-1],
> +      SCRIPT = arg[0],
> +  })
> +  local proc = io.popen(cmd)
> +  local got = proc:read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
> +  local expected = table.concat({
> +      '---- TRACE %d start',
> +      '---- TRACE %d IR',
> +      '---- TRACE %d mcode',
> +      '---- TRACE %d stop',
> +      '---- TRACE %d exit',
> +  }, '.+')
> +
> +  test:like(got, expected , 'jit.dump smoke tests')
> +
> +  os.exit(test:check() and 0 or 1)
> +end
> +
> +-- Use *all* jit.dump options, so the test can check them all.
> +require('jit.dump').start('+tbisrmXaT')
> +-- Tune JIT engine to make the test faster and more robust.

Side note: Do you check that it is really faster than 57 iterations:)?

> +jit.opt.start('hotloop=1')

Also, this line leads to test failure:

| $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
| ...
| [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
| [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
| [001] TAP version 13
| [001] 1..1
| [001] not ok - jit.dump smoke tests
| [001]   ---
| [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
| [001]   trace:
| [001]   - line: 0
| [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
| [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
| [001]     what: main
| [001]     namewhat: 
| [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
| [001]   line: 0
| [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
| [001]     %d stop.+---- TRACE %d exit'
| [001]   got: 'LuajitError: ...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua:42:
| [001]     attempt to index field ''opt'' (a nil value)
| [001]     fatal error, exiting the event loop'
| [001]   ...
| [001] # failed subtest: 1

So, I suggest to drop it for now.

But even with the following patch

===================================================================
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
index 72caec2f9..8ff234a12 100755
--- a/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
+++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
@@ -39,6 +39,5 @@ end
 -- Use *all* jit.dump options, so the test can check them all.
 require('jit.dump').start('+tbisrmXaT')
 -- Tune JIT engine to make the test faster and more robust.
-jit.opt.start('hotloop=1')
 -- Record primitive loop.
-for _ = 1, 3 do end
+for _ = 1, 100 do end
===================================================================

this test fails:

| $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
| ...
| [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
| [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
| [001] TAP version 13
| [001] 1..1
| [001] not ok - jit.dump smoke tests
| [001]   ---
| [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
| [001]   trace:
| [001]   - line: 0
| [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
| [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
| [001]     what: main
| [001]     namewhat: 
| [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
| [001]   line: 0
| [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
| [001]     %d stop.+---- TRACE %d exit'
| [001]   got: 
| [001]   ...
| [001] # failed subtest: 1

What am I doing wrong?

> +-- Record primitive loop.
> +for _ = 1, 3 do end
> 
> ================================================================================
> 
> CI is green[1] now.
> 
> 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 ++++++++++++++
> >  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
> > 
> 
> [1]: https://github.com/tarantool/tarantool/commit/18a4018
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  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
  1 sibling, 1 reply; 12+ messages in thread
From: Sergey Ostanevich via Tarantool-patches @ 2021-05-18 15:34 UTC (permalink / raw)
  To: Sergey Kaplun, Igor Munkin; +Cc: tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 10800 bytes --]

Hi! 

Thanks for the patch!

My test run passes:
$ ./test-run.py 5983
Statistics:
* pass: 1

As for comment on opt.start - I’m not sure if we will ever change the threshold
for optimizations - better to keep it under control and do not rely on a magic 100
loop counter.

But the essence of the test is not clear to me: we just verify that on the host
platform (the one testing is done) jit.dump is operable? I believe it should be
put into original message then - ’to enable jit.dump on ARM64…'

Otherwise - LGTM.

Sergos


> On 18 May 2021, at 10:14, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Igor,
> 
> On 17.05.21, Igor Munkin wrote:
>> Oops, sorry guys, but the test seems to be noop for this fix (in other
>> works it's OK even without patch), so I've reimplemented it. Consider
>> the description in the test. Diff is below:
> 
> Thanks for update!
> Please consider my comments below.
> 
> Side note: Please, rebase your branch to master to make it buildable.
> 
>> 
>> ================================================================================
>> 
>> diff --git a/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
>> new file mode 100644
>> index 000000000..2a2ec4d97
>> --- /dev/null
>> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.skipcond
>> @@ -0,0 +1,7 @@
>> +import platform
>> +
>> +# Disabled on FreeBSD due to #4819.
>> +if platform.system() == 'FreeBSD':
>> +    self.skip = 1
>> +
>> +# vim: set ft=python:
>> 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..72caec2f9
>> --- /dev/null
>> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
>> @@ -0,0 +1,44 @@
>> +#!/usr/bin/env tarantool
>> +
>> +-- Just check whether all Lua sources related to jit.dump are
>> +-- bundled to the binary. Otherwise, jit.dump module raises
>> +-- an error that is handled via <pcall>.
>> +-- XXX: pure require for jit.dump doesn't cover all the cases,
>> +-- since dis_<arch>.lua are loaded at runtime. Furthermore, this
>> +-- error is handled by JIT engine, so we can't use <pcall> for it.
>> +-- Hence, simply sniff the output of the test to check that all
>> +-- phases of trace compilation are dumped.
>> +
>> +if #arg == 0 then
>> +  local tap = require('tap')
>> +  local test = tap.test('gh-5983-jit-library-smoke-tests')
>> +
>> +  test:plan(1)
>> +
>> +  -- XXX: Shell argument <test> is necessary to differ test case
>> +  -- from the test runner.
>> +  local cmd = string.gsub('<LUABIN> 2>&1 <SCRIPT> test', '%<(%w+)>', {
>> +      LUABIN = arg[-1],
>> +      SCRIPT = arg[0],
>> +  })
>> +  local proc = io.popen(cmd)
>> +  local got = proc:read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
>> +  local expected = table.concat({
>> +      '---- TRACE %d start',
>> +      '---- TRACE %d IR',
>> +      '---- TRACE %d mcode',
>> +      '---- TRACE %d stop',
>> +      '---- TRACE %d exit',
>> +  }, '.+')
>> +
>> +  test:like(got, expected , 'jit.dump smoke tests')
>> +
>> +  os.exit(test:check() and 0 or 1)
>> +end
>> +
>> +-- Use *all* jit.dump options, so the test can check them all.
>> +require('jit.dump').start('+tbisrmXaT')
>> +-- Tune JIT engine to make the test faster and more robust.
> 
> Side note: Do you check that it is really faster than 57 iterations:)?
> 
>> +jit.opt.start('hotloop=1')
> 
> Also, this line leads to test failure:
> 
> | $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | ...
> | [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
> | [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
> | [001] TAP version 13
> | [001] 1..1
> | [001] not ok - jit.dump smoke tests
> | [001]   ---
> | [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]   trace:
> | [001]   - line: 0
> | [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]     what: main
> | [001]     namewhat: 
> | [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]   line: 0
> | [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
> | [001]     %d stop.+---- TRACE %d exit'
> | [001]   got: 'LuajitError: ...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua:42:
> | [001]     attempt to index field ''opt'' (a nil value)
> | [001]     fatal error, exiting the event loop'
> | [001]   ...
> | [001] # failed subtest: 1
> 
> So, I suggest to drop it for now.
> 
> But even with the following patch
> 
> ===================================================================
> 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
> index 72caec2f9..8ff234a12 100755
> --- a/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> @@ -39,6 +39,5 @@ end
> -- Use *all* jit.dump options, so the test can check them all.
> require('jit.dump').start('+tbisrmXaT')
> -- Tune JIT engine to make the test faster and more robust.
> -jit.opt.start('hotloop=1')
> -- Record primitive loop.
> -for _ = 1, 3 do end
> +for _ = 1, 100 do end
> ===================================================================
> 
> this test fails:
> 
> | $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | ...
> | [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
> | [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
> | [001] TAP version 13
> | [001] 1..1
> | [001] not ok - jit.dump smoke tests
> | [001]   ---
> | [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]   trace:
> | [001]   - line: 0
> | [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]     what: main
> | [001]     namewhat: 
> | [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]   line: 0
> | [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
> | [001]     %d stop.+---- TRACE %d exit'
> | [001]   got: 
> | [001]   ...
> | [001] # failed subtest: 1
> 
> What am I doing wrong?
> 
>> +-- Record primitive loop.
>> +for _ = 1, 3 do end
>> 
>> ================================================================================
>> 
>> CI is green[1] now.
>> 
>> 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 ++++++++++++++
>>> 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
>>> 
>> 
>> [1]: https://github.com/tarantool/tarantool/commit/18a4018
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 76679 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-18  7:14   ` Sergey Kaplun via Tarantool-patches
  2021-05-18 15:34     ` Sergey Ostanevich via Tarantool-patches
@ 2021-05-19 12:19     ` Igor Munkin via Tarantool-patches
  2021-05-19 16:55       ` Sergey Kaplun via Tarantool-patches
  1 sibling, 1 reply; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-19 12:19 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

On 18.05.21, Sergey Kaplun wrote:
> Igor,
> 
> On 17.05.21, Igor Munkin wrote:
> > Oops, sorry guys, but the test seems to be noop for this fix (in other
> > works it's OK even without patch), so I've reimplemented it. Consider
> > the description in the test. Diff is below:
> 
> Thanks for update!
> Please consider my comments below.
> 
> Side note: Please, rebase your branch to master to make it buildable.

It was rebased prior to sending the updates. The reason this patch fails
to build on M1 is that you're missing the other patches (also not in
master branch yet).

> 
> > 
> > ================================================================================
> > 

<snipped>

> > 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..72caec2f9
> > --- /dev/null
> > +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > @@ -0,0 +1,44 @@
> > +#!/usr/bin/env tarantool
> > +
> > +-- Just check whether all Lua sources related to jit.dump are
> > +-- bundled to the binary. Otherwise, jit.dump module raises
> > +-- an error that is handled via <pcall>.
> > +-- XXX: pure require for jit.dump doesn't cover all the cases,
> > +-- since dis_<arch>.lua are loaded at runtime. Furthermore, this
> > +-- error is handled by JIT engine, so we can't use <pcall> for it.
> > +-- Hence, simply sniff the output of the test to check that all
> > +-- phases of trace compilation are dumped.
> > +
> > +if #arg == 0 then
> > +  local tap = require('tap')
> > +  local test = tap.test('gh-5983-jit-library-smoke-tests')
> > +
> > +  test:plan(1)
> > +
> > +  -- XXX: Shell argument <test> is necessary to differ test case
> > +  -- from the test runner.
> > +  local cmd = string.gsub('<LUABIN> 2>&1 <SCRIPT> test', '%<(%w+)>', {
> > +      LUABIN = arg[-1],
> > +      SCRIPT = arg[0],
> > +  })
> > +  local proc = io.popen(cmd)
> > +  local got = proc:read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
> > +  local expected = table.concat({
> > +      '---- TRACE %d start',
> > +      '---- TRACE %d IR',
> > +      '---- TRACE %d mcode',
> > +      '---- TRACE %d stop',
> > +      '---- TRACE %d exit',
> > +  }, '.+')
> > +
> > +  test:like(got, expected , 'jit.dump smoke tests')
> > +
> > +  os.exit(test:check() and 0 or 1)
> > +end
> > +
> > +-- Use *all* jit.dump options, so the test can check them all.
> > +require('jit.dump').start('+tbisrmXaT')
> > +-- Tune JIT engine to make the test faster and more robust.
> 
> Side note: Do you check that it is really faster than 57 iterations:)?

Yes, of course. I'm not that bad in math: 3 is less than 100 and 2 is
less than 57. Furthermore, I don't need the trace being executed, only
recorded, so only 3 iterations are executed. Is it faster? Faster than
light it is! ;)

> 
> > +jit.opt.start('hotloop=1')
> 
> Also, this line leads to test failure:

I know. This is fixed here[1].

> 
> | $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | ...
> | [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
> | [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
> | [001] TAP version 13
> | [001] 1..1
> | [001] not ok - jit.dump smoke tests
> | [001]   ---
> | [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]   trace:
> | [001]   - line: 0
> | [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]     what: main
> | [001]     namewhat: 
> | [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]   line: 0
> | [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
> | [001]     %d stop.+---- TRACE %d exit'
> | [001]   got: 'LuajitError: ...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua:42:
> | [001]     attempt to index field ''opt'' (a nil value)
> | [001]     fatal error, exiting the event loop'
> | [001]   ...
> | [001] # failed subtest: 1
> 
> So, I suggest to drop it for now.

Why? As I see below, even after your changes the test doesn't work.

> 
> But even with the following patch
> 
> ===================================================================
> 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
> index 72caec2f9..8ff234a12 100755
> --- a/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> @@ -39,6 +39,5 @@ end
>  -- Use *all* jit.dump options, so the test can check them all.
>  require('jit.dump').start('+tbisrmXaT')
>  -- Tune JIT engine to make the test faster and more robust.
> -jit.opt.start('hotloop=1')
>  -- Record primitive loop.
> -for _ = 1, 3 do end
> +for _ = 1, 100 do end
> ===================================================================
> 
> this test fails:
> 
> | $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | ...
> | [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
> | [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
> | [001] TAP version 13
> | [001] 1..1
> | [001] not ok - jit.dump smoke tests
> | [001]   ---
> | [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]   trace:
> | [001]   - line: 0
> | [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> | [001]     what: main
> | [001]     namewhat: 
> | [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> | [001]   line: 0
> | [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
> | [001]     %d stop.+---- TRACE %d exit'
> | [001]   got: 
> | [001]   ...
> | [001] # failed subtest: 1
> 
> What am I doing wrong?

Testing this patchset on M1. Use GNU/Linux ARM64 hosts for now.

> 
> > +-- Record primitive loop.
> > +for _ = 1, 3 do end
> > 
> > ================================================================================
> > 
> > CI is green[1] now.
> > 

<snipped>

> > 
> > [1]: https://github.com/tarantool/tarantool/commit/18a4018
> > 
> > -- 
> > Best regards,
> > IM
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://lists.tarantool.org/tarantool-patches/cover.1620678384.git.imun@tarantool.org/T/#t

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-18 15:34     ` Sergey Ostanevich via Tarantool-patches
@ 2021-05-19 13:04       ` Igor Munkin via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-19 13:04 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Sergos,

On 18.05.21, Sergey Ostanevich wrote:
> Hi! 
> 
> Thanks for the patch!
> 
> My test run passes:
> $ ./test-run.py 5983
> Statistics:
> * pass: 1
> 
> As for comment on opt.start - I’m not sure if we will ever change the threshold
> for optimizations - better to keep it under control and do not rely on a magic 100
> loop counter.

Hence, I mentioned that jit.opt.start call makes the test more robust.

> 
> But the essence of the test is not clear to me: we just verify that on the host
> platform (the one testing is done) jit.dump is operable? I believe it should be
> put into original message then - ’to enable jit.dump on ARM64…'

Indeed. We just check, that all compiler phases are dumped (so all
necessary modules are successfully loaded). Thanks for noticing, I've
updated the commit message the following way:
| 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
| works fine on ARM64 hosts as a result.

> 
> Otherwise - LGTM.

Great, thanks!

> 
> Sergos
> 

<snipped>

> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-19 12:19     ` Igor Munkin via Tarantool-patches
@ 2021-05-19 16:55       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 12+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2021-05-19 16:55 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Igor,

LGTM!

On 19.05.21, Igor Munkin wrote:
> Sergey,
> 
> On 18.05.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > On 17.05.21, Igor Munkin wrote:

<snipped>

> > ===================================================================
> > 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
> > index 72caec2f9..8ff234a12 100755
> > --- a/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > +++ b/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > @@ -39,6 +39,5 @@ end
> >  -- Use *all* jit.dump options, so the test can check them all.
> >  require('jit.dump').start('+tbisrmXaT')
> >  -- Tune JIT engine to make the test faster and more robust.
> > -jit.opt.start('hotloop=1')
> >  -- Record primitive loop.
> > -for _ = 1, 3 do end
> > +for _ = 1, 100 do end
> > ===================================================================
> > 
> > this test fails:
> > 
> > | $ ./test-run.py app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > | ...
> > | [001] app-tap/gh-5983-jit-library-smoke-tests.test.l>                 [ fail ]
> > | [001] Test failed! Output from reject file var/rejects/app-tap/gh-5983-jit-library-smoke-tests.reject:
> > | [001] TAP version 13
> > | [001] 1..1
> > | [001] not ok - jit.dump smoke tests
> > | [001]   ---
> > | [001]   filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > | [001]   trace:
> > | [001]   - line: 0
> > | [001]     source: '@/Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> > | [001]     filename: /Users/tntmac07.tarantool.i/s.kaplun/tarantool/master/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua
> > | [001]     what: main
> > | [001]     namewhat: 
> > | [001]     src: '...er/test/app-tap/gh-5983-jit-library-smoke-tests.test.lua'
> > | [001]   line: 0
> > | [001]   expected: '---- TRACE %d start.+---- TRACE %d IR.+---- TRACE %d mcode.+---- TRACE
> > | [001]     %d stop.+---- TRACE %d exit'
> > | [001]   got: 
> > | [001]   ...
> > | [001] # failed subtest: 1
> > 
> > What am I doing wrong?
> 
> Testing this patchset on M1. Use GNU/Linux ARM64 hosts for now.

Yep, thanks!
Checked it on odroid -- it's OK.

> 
> > 
> > > +-- Record primitive loop.
> > > +for _ = 1, 3 do end
> > > 
> > > ================================================================================
> > > 
> > > CI is green[1] now.
> > > 
> 
> <snipped>
> 
> > > 
> > > [1]: https://github.com/tarantool/tarantool/commit/18a4018
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> [1]: https://lists.tarantool.org/tarantool-patches/cover.1620678384.git.imun@tarantool.org/T/#t
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64
  2021-05-04  8:29 [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 Igor Munkin via Tarantool-patches
                   ` (2 preceding siblings ...)
  2021-05-17 16:34 ` Igor Munkin via Tarantool-patches
@ 2021-05-19 17:20 ` Igor Munkin via Tarantool-patches
  3 siblings, 0 replies; 12+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-05-19 17:20 UTC (permalink / raw)
  To: Sergey Ostanevich, Sergey Kaplun; +Cc: tarantool-patches

I've checked the patch into master.

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
> 

<snipped>

> -- 
> 2.25.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-05-19 17:20 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04  8:29 [Tarantool-patches] [PATCH] build: add missing module for jit.dump on ARM64 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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox