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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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