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