Re: [Tarantool-patches] [PATCH luajit] test: disable jit tests at M1 for Tarantool

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

Hi, Sergey!

Thanks for the patch! See some comments below.

> On 17 Aug 2022, at 12:24, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> During running JIT-related tests on Tarantool the trace assembling
> occasionally fails with the error "failed to allocate mcode memory"
> because there are no suitable addresses available.

I would rephrase to be more on the spot:

Trace assembling can fail with an error of mcode memory allocation due to
the branch offset limitation of +-32Mb on an ARM platform. For the time we’re
elaborating a correct fix we disable tests proven to be flaky because of this
reason.

I would propose to separate the refactor commit as a preparational, since
sysprof is out of this patch context.

> 
> This patch disables those tests for Tarantool in a similar way as it is
> done for FreeBSD. Also, this patch refactors `utils.skipcond()` a bit
> to make it call readable with multiple conditions/reasons pairs.
> 
> Relates to tarantool/tarantool#7571
> ---
> 
> Tarantool PR: https://github.com/tarantool/tarantool/pull/7578
> Issue: https://github.com/tarantool/tarantool/issues/7571
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-7571-disable-jit-tests-m1-full-ci
> 
> Side note: Looks like adding the trampoline, as Sergos suggested is the
> correct solution of this issue. For now just disable JIT related tests
> only for Tarantool.

I believe we can pre-allocate a 64Mb mcode buffer ahead of regular Tarantool
memory allocation activities, since it can’t be bigger on ARM anyways?
Trampoline will remove those limitations although.

> 
> .../gh-4199-gc64-fuse.test.lua                |  2 +-
> .../gh-4427-ffi-sandwich.test.lua             |  7 ++++--
> .../gh-5813-resolving-of-c-symbols.test.lua   | 10 ++++-----
> ...4-add-proto-trace-sysprof-default.test.lua | 12 +++++-----
> .../lj-430-maxirconst.test.lua                |  8 +++++--
> ...lj-672-cdata-allocation-recording.test.lua |  8 +++++--
> .../lj-flush-on-trace.test.lua                |  7 ++++--
> .../misclib-getmetrics-capi.test.lua          |  8 +++++--
> .../misclib-getmetrics-lapi.test.lua          |  8 +++++--
> .../misclib-memprof-lapi.test.lua             | 17 +++++++-------
> .../misclib-sysprof-capi.test.lua             | 11 +++++-----
> .../misclib-sysprof-lapi.test.lua             | 11 +++++-----
> test/tarantool-tests/utils.lua                | 22 ++++++++++++++-----
> 13 files changed, 83 insertions(+), 48 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> index 4d69250f..04950fd2 100644
> --- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> +++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> @@ -1,6 +1,6 @@
> -- The test is GC64 only.
> local ffi = require('ffi')
> -require('utils').skipcond(not ffi.abi('gc64'), 'test is GC64 only')
> +require('utils').skipcond({not ffi.abi('gc64'), 'test is GC64 only'})
> 
> local tap = require('tap')
> local test = tap.test('gh-4199-gc64-fuse')
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index dd02130c..fea1d181 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -1,7 +1,10 @@
> local utils = require('utils')
> 
> --- Disabled on *BSD due to #4819.
> -utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> utils.selfrun(arg, {
>   {
> diff --git a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> index d589dddf..ff0c48e6 100644
> --- a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> +++ b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> @@ -1,8 +1,8 @@
> --- Memprof is implemented for x86 and x64 architectures only.
> -require("utils").skipcond(
> -  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux",
> -  jit.arch.." architecture or "..jit.os..
> -  " OS is NIY for memprof c symbols resolving"
> +local utils = require("utils")
> +utils.skipcond(
> +  {jit.arch ~= "x86" and jit.arch ~= "x64", jit.arch ..
> +     " architecture is NIY for memprof c symbols resolving"},
> +  {jit.os ~= "Linux", jit.os .. " OS is NIY for memprof c symbols resolving"}
> )
> 
> local tap = require("tap")
> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> index 15bd0a8b..332d4711 100644
> --- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> @@ -1,9 +1,9 @@
> --- Sysprof is implemented for x86 and x64 architectures only.
> -require('utils').skipcond(
> -  jit.arch ~= 'x86' and jit.arch ~= 'x64' or jit.os ~= 'Linux'
> -    or require('ffi').abi('gc64'),
> -  jit.arch..' architecture or '..jit.os..
> -  ' OS is NIY for sysprof'
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.arch ~= 'x86' and jit.arch ~= 'x64', jit.arch ..
> +     ' architecture is NIY for sysprof'},
> +  {jit.os ~= 'Linux', jit.os .. ' OS is NIY for sysprof'},
> +  {require('ffi').abi('gc64'), 'GC64 mode is NIY for sysprof'}
> )
> 
> local tap = require('tap')
> diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
> index cd3587a8..b95aacbd 100644
> --- a/test/tarantool-tests/lj-430-maxirconst.test.lua
> +++ b/test/tarantool-tests/lj-430-maxirconst.test.lua
> @@ -3,8 +3,12 @@
> jit.off()
> jit.flush()
> 
> --- Disabled on *BSD due to #4819.
> -require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> local tap = require('tap')
> local traceinfo = require('jit.util').traceinfo
> diff --git a/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua b/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua
> index 1dc741d8..c72688d6 100644
> --- a/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua
> +++ b/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua
> @@ -1,5 +1,9 @@
> --- Disabled on *BSD due to #4819.
> -require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> local ffi = require('ffi')
> local traceinfo = require('jit.util').traceinfo
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index c46b93f0..006c61f0 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -1,7 +1,10 @@
> local utils = require('utils')
> 
> --- Disabled on *BSD due to #4819.
> -utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> utils.selfrun(arg, {
>   {
> diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> index 0cbc6cae..f747500b 100644
> --- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> @@ -1,5 +1,9 @@
> --- Disabled on *BSD due to #4819.
> -require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> local path = arg[0]:gsub('%.test%.lua', '')
> local suffix = package.cpath:match('?.(%a+);')
> diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> index 222e7d01..50a4fce3 100644
> --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> @@ -2,8 +2,12 @@
> -- Major portions taken verbatim or adapted from the LuaVela testing suite.
> -- Copyright (C) 2015-2019 IPONWEB Ltd.
> 
> --- Disabled on *BSD due to #4819.
> -require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.os == 'BSD', 'Disabled due to #4819'},
> +  --luacheck: no global
> +  {_TARANTOOL and utils.is_M1(), 'Disabled due to #7571'}
> +)
> 
> local tap = require('tap')
> 
> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index a11f0be1..66fb2108 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -1,7 +1,8 @@
> -- Memprof is implemented for x86 and x64 architectures only.
> -require("utils").skipcond(
> -  jit.arch ~= "x86" and jit.arch ~= "x64",
> -  jit.arch.." architecture is NIY for memprof"
> +local utils = require('utils')
> +utils.skipcond(
> +  {jit.arch ~= "x86" and jit.arch ~= "x64",
> +    jit.arch .. " architecture is NIY for memprof"}
> )
> 
> local tap = require("tap")
> @@ -189,9 +190,9 @@ test:test("output", function(subtest)
>   -- one is the number of allocations. 1 event - alocation of
>   -- table by itself + 1 allocation of array part as far it is
>   -- bigger than LJ_MAX_COLOSIZE (16).
> -  subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 36, linedefined = 34 }, 2))
>   -- 20 strings allocations.
> -  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 20))
> +  subtest:ok(check_alloc_report(alloc, { line = 41, linedefined = 34 }, 20))
> 
>   -- Collect all previous allocated objects.
>   subtest:ok(free.INTERNAL.num == 22)
> @@ -291,10 +292,10 @@ test:test("jit-output", function(subtest)
>   -- 10 allocations in interpreter mode, 1 allocation for a trace
>   -- recording and assembling and next 9 allocations will happen
>   -- while running the trace.
> -  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 11))
> -  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 38 }, 9))
> +  subtest:ok(check_alloc_report(alloc, { line = 41, linedefined = 34 }, 11))
> +  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 39 }, 9))
>   -- See same checks with jit.off().
> -  subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 36, linedefined = 34 }, 2))
> 
>   -- Restore default JIT settings.
>   jit.opt.start(unpack(jit_opt_default))
> diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
> index afb99cf2..32f00ffd 100644
> --- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua
> @@ -1,10 +1,11 @@
> -- Sysprof is implemented for x86 and x64 architectures only.
> local ffi = require("ffi")
> -require("utils").skipcond(
> -  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
> -    or ffi.abi("gc64"),
> -  jit.arch.." architecture or "..jit.os..
> -  " OS is NIY for sysprof"
> +local utils = require("utils")
> +utils.skipcond(
> +  {jit.arch ~= "x86" and jit.arch ~= "x64", jit.arch ..
> +     " architecture is NIY for sysprof"},
> +  {jit.os ~= "Linux", jit.os .. " OS is NIY for sysprof"},
> +  {ffi.abi("gc64"), "GC64 mode is NIY for sysprof"}
> )
> 
> local testsysprof = require("testsysprof")
> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index 9e0a8a77..e9ae182e 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -1,10 +1,11 @@
> -- Sysprof is implemented for x86 and x64 architectures only.
> local ffi = require("ffi")
> -require("utils").skipcond(
> -  jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
> -    or ffi.abi("gc64"),
> -  jit.arch.." architecture or "..jit.os..
> -  " OS is NIY for sysprof"
> +local utils = require("utils")
> +utils.skipcond(
> +  {jit.arch ~= "x86" and jit.arch ~= "x64", jit.arch ..
> +     " architecture is NIY for sysprof"},
> +  {jit.os ~= "Linux", jit.os .. " OS is NIY for sysprof"},
> +  {ffi.abi("gc64"), "GC64 mode is NIY for sysprof"}
> )
> 
> local tap = require("tap")
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index a1906498..d8412e03 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -20,6 +20,10 @@ ffi.cdef([[
>   } GCHeader;
> ]])
> 
> +function M.is_M1()
> +  return jit.os == 'OSX' and jit.arch == 'arm64'
> +end
> +
> function M.gcisblack(obj)
>   local objtype = type(obj)
>   local address = objtype == 'string'
> @@ -95,12 +99,18 @@ function M.selfrun(arg, checks)
>   os.exit(test:check() and 0 or 1)
> end
> 
> -function M.skipcond(condition, message)
> -  if not condition then return end
> -  local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> -  test:plan(1)
> -  test:skip(message)
> -  os.exit(test:check() and 0 or 1)
> +function M.skipcond(...)

Rather crucial change to the interface. Is it documented in some way?
Should it? Some in-line, or a readme? Nottin?.. 

> +  local conditions = {...}
> +  local ncond = #conditions
> +  for i = 1, ncond do
> +    local condition, message = conditions[i][1], conditions[i][2]
> +    if condition then
> +      local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> +      test:plan(1)
> +      test:skip(message)
> +      os.exit(test:check() and 0 or 1)
> +    end
> +  end
> end
> 
> function M.hasbc(f, bytecode)
> -- 
> 2.34.1
>