[Tarantool-patches] [PATCH luajit 0/3] Fix fix-mips64-spare-side-exit-patching

[Tarantool-patches] [PATCH luajit 0/3] Fix fix-mips64-spare-side-exit-patching

[Sergey Kaplun via Tarantool-patches]

This patchset includes 3 pathes to make our tests great again:
1) 'MIPS: Fix "bad FP FLOAD" assertion.' is needed for easier testing
   the patch's correctness (that test still reproduces the problem on
   MIPS). The patch itself is trivial, as is the test for it.
2) 'test: fix `fillmcode()` generator helper' fixes the correctness of
   fillmcode logic -- somehow the condition of `while` cycle for mcode
   generation was inverted.
3) 'test: fix fix-mips64-spare-side-exit-patching' fixes the possible
   misbehaviours in the test itself (they were more likely observed before
   the second patch but still needed to be fixed).

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-flaky-mips-spare-exit
Tarantool PR: https://github.com/tarantool/tarantool/pull/9090
Related issues for the first patch:
* https://github.com/LuaJIT/LuaJIT/issues/1043
* https://github.com/tarantool/tarantool/issues/8825

Mike Pall (1):
  MIPS: Fix "bad FP FLOAD" assertion.

Sergey Kaplun (2):
  test: fix `fillmcode()` generator helper
  test: fix fix-mips64-spare-side-exit-patching

 src/lj_asm_mips.h                             |  2 +-
 ...x-mips64-spare-side-exit-patching.test.lua | 22 ++++++++++++++---
 .../lj-1043-asm-fload.test.lua                | 24 +++++++++++++++++++
 test/tarantool-tests/utils/jit/generators.lua |  7 +++---
 4 files changed, 47 insertions(+), 8 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1043-asm-fload.test.lua

-- 
2.42.0

[Tarantool-patches] [PATCH luajit 1/3] MIPS: Fix "bad FP FLOAD" assertion.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry-picked from commit 72efc42ef2258086a9cb797c676e2916b0a9e7e1)

This patch is the follow-up for the commit
786dbb2ebdde16eadd7464cd5cbeb5d95a5e46f0 ("Add IR_FLOAD with REF_NIL for
field loads from GG_State."). This commit allows `FLOAD` to be used for
fields loading from `GG_State`. Nevertheless, the aforementioned
assertion hasn't been moved to the `else` branch related to the default
use case. This leads to assertion failure in the case when `FLOAD` is
used for loading some field and has the `num` type.

This patch moves the assertion to the right place.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8825
---
 src/lj_asm_mips.h                             |  2 +-
 .../lj-1043-asm-fload.test.lua                | 24 +++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1043-asm-fload.test.lua

diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h
index ea108aab..ac9090f2 100644
--- a/src/lj_asm_mips.h
+++ b/src/lj_asm_mips.h
@@ -1285,8 +1285,8 @@ static void asm_fload(ASMState *as, IRIns *ir)
       }
     }
     ofs = field_ofs[ir->op2];
+    lj_assertA(!irt_isfp(ir->t), "bad FP FLOAD");
   }
-  lj_assertA(!irt_isfp(ir->t), "bad FP FLOAD");
   emit_tsi(as, mi, dest, idx, ofs);
 }
 
diff --git a/test/tarantool-tests/lj-1043-asm-fload.test.lua b/test/tarantool-tests/lj-1043-asm-fload.test.lua
new file mode 100644
index 00000000..2f381560
--- /dev/null
+++ b/test/tarantool-tests/lj-1043-asm-fload.test.lua
@@ -0,0 +1,24 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT's misbehaviour during the
+-- assembling of the `FLOAD` on MIPS.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1043.
+local test = tap.test('lj-1043-asm-fload'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local math_abs = math.abs
+
+local results = {nil, nil, nil, nil}
+
+-- Disable optimizations to be sure that we assemble `FLOAD`.
+jit.opt.start(0, 'hotloop=1')
+for i = 1, 4 do
+  results[i] = math_abs(i - 10)
+end
+
+test:is_deeply(results, {9, 8, 7, 6}, 'correct assembling of the FLOAD')
+
+test:done(true)
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 2/3] test: fix `fillmcode()` generator helper

[Sergey Kaplun via Tarantool-patches]

This patch fixes the typo in the condition for code generation. Due to
the wrong comparison, the generation was stopped too early.
---
 test/tarantool-tests/utils/jit/generators.lua | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/tarantool-tests/utils/jit/generators.lua b/test/tarantool-tests/utils/jit/generators.lua
index 65abfdaa..0189cd2a 100644
--- a/test/tarantool-tests/utils/jit/generators.lua
+++ b/test/tarantool-tests/utils/jit/generators.lua
@@ -38,7 +38,7 @@ function M.fillmcode(trace_from, size)
 
   -- Addresses of traces may increase or decrease depending on OS,
   -- so use absolute diff.
-  while math.abs(last_addr - addr_from) > required_diff do
+  while math.abs(last_addr - addr_from) < required_diff do
     last_i = last_i + 1
     -- This is quite a heavy workload (though it doesn't look like
     -- one at first). Each load from a table is type guarded. Each
-- 
2.42.0

[Tarantool-patches] [PATCH luajit 3/3] test: fix fix-mips64-spare-side-exit-patching

[Sergey Kaplun via Tarantool-patches]

This test may be flaky due to a non-monotonic filling of the vector with
traces. Hence, `misc.getmetrics().jit_trace_num` (which is the total
number of traces) is not the last trace number, so there is no mcode for
it.

This patch fixes flakyness from two sides:
* The `fillmcode()` function now starts a search from the given trace
  itself, so there is no possible inconsistency with the growing vector.
* The finding of the last trace number in the aforementioned test is now
  more bulletproof: we scan all numbers from the metrics value to the
  maxtrace value. The last number with an existing mcode is chosen as
  the resulting last trace number.
---
 ...x-mips64-spare-side-exit-patching.test.lua | 22 ++++++++++++++++---
 test/tarantool-tests/utils/jit/generators.lua |  5 ++---
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
index 62933df9..703d8e69 100644
--- a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
+++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua
@@ -8,9 +8,26 @@ local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({
 
 local generators = require('utils').jit.generators
 local frontend = require('utils').frontend
+local jutil = require('jit.util')
+
+-- Allow compilation of up to 2000 traces to avoid flushes.
+local MAXTRACE = 2000;
 
 test:plan(1)
 
+local function find_last_trace()
+  local candidate = misc.getmetrics().jit_trace_num
+  for traceno = candidate, MAXTRACE do
+    -- There is no need for heavy calls here. Just use the
+    -- simplest one to invoke `lj_checktrace()`.
+    if jutil.tracemc(traceno) then
+      candidate = traceno
+    end
+  end
+  assert(jutil.tracemc(candidate), 'tracenum candidate is invalid')
+  return candidate
+end
+
 -- Make compiler work hard.
 jit.opt.start(
   -- No optimizations at all to produce more mcode.
@@ -18,8 +35,7 @@ jit.opt.start(
   -- Try to compile all compiled paths as early as JIT can.
   'hotloop=1',
   'hotexit=1',
-  -- Allow compilation of up to 2000 traces to avoid flushes.
-  'maxtrace=2000',
+  ('maxtrace=%d'):format(MAXTRACE),
   -- Allow to compile 8Mb of mcode to be sure the issue occurs.
   'maxmcode=8192',
   -- Use big mcode area for traces to avoid usage of different
@@ -59,7 +75,7 @@ for i = 1, MAX_SPARE_SLOT + 1 do
   parent(i)
   parent(i)
   parent(i)
-  last_traceno = misc.getmetrics().jit_trace_num
+  last_traceno = find_last_trace()
 end
 
 test:ok(true, 'all traces executed correctly')
diff --git a/test/tarantool-tests/utils/jit/generators.lua b/test/tarantool-tests/utils/jit/generators.lua
index 0189cd2a..14e0e3c3 100644
--- a/test/tarantool-tests/utils/jit/generators.lua
+++ b/test/tarantool-tests/utils/jit/generators.lua
@@ -32,9 +32,7 @@ function M.fillmcode(trace_from, size)
   -- Marker to check that traces are not flushed.
   local maxtraceno = getlast_traceno()
   local FLUSH_ERR = 'Traces are flushed, check your maxtrace, maxmcode options'
-
-  local _, last_addr = jutil.tracemc(maxtraceno)
-  last_addr = canonicalize_address(last_addr)
+  local last_addr = addr_from
 
   -- Addresses of traces may increase or decrease depending on OS,
   -- so use absolute diff.
@@ -104,6 +102,7 @@ function M.fillmcode(trace_from, size)
 
     -- Calculate the address of the last trace start.
     maxtraceno = last_traceno
+    local _
     _, last_addr = jutil.tracemc(last_traceno)
     if not last_addr then
       error(FLUSH_ERR)
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit 1/3] MIPS: Fix "bad FP FLOAD" assertion.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

Re: [Tarantool-patches] [PATCH luajit 2/3] test: fix `fillmcode()` generator helper

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix fix-mips64-spare-side-exit-patching

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
LGTM, except for the single nit regarding the commit message below.

On Mon, Sep 04, 2023 at 06:50:26PM +0300, Sergey Kaplun wrote:
> This test may be flaky due to a non-monotonic filling of the vector with
I believe, that 'sparse' is a better fit here then 'non-monotonic'. It
is just a widely adopted term for vectors like this.

> traces. Hence, `misc.getmetrics().jit_trace_num` (which is the total
> number of traces) is not the last trace number, so there is no mcode for
> it.
> 
> This patch fixes flakyness from two sides:
> * The `fillmcode()` function now starts a search from the given trace
>   itself, so there is no possible inconsistency with the growing vector.
> * The finding of the last trace number in the aforementioned test is now
>   more bulletproof: we scan all numbers from the metrics value to the
>   maxtrace value. The last number with an existing mcode is chosen as
>   the resulting last trace number.
> ---
>  ...x-mips64-spare-side-exit-patching.test.lua | 22 ++++++++++++++++---
>  test/tarantool-tests/utils/jit/generators.lua |  5 ++---
>  2 files changed, 21 insertions(+), 6 deletions(-)
<snipped> 

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix fix-mips64-spare-side-exit-patching

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!

On 05.09.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> LGTM, except for the single nit regarding the commit message below.
> 
> On Mon, Sep 04, 2023 at 06:50:26PM +0300, Sergey Kaplun wrote:
> > This test may be flaky due to a non-monotonic filling of the vector with
> I believe, that 'sparse' is a better fit here then 'non-monotonic'. It
> is just a widely adopted term for vectors like this.

Replaced with sparse, thanks!

> 

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/3] MIPS: Fix "bad FP FLOAD" assertion.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM, but I still hardly understand the relevance
of this patch for our fork at this moment: we have no CI for MIPS to
test such patches. Anyway, I'll merge this to the trunk since all in all
we have no option.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/3] test: fix `fillmcode()` generator helper

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM as trivial.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 3/3] test: fix fix-mips64-spare-side-exit-patching

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM. I'd rather moved the helper to the dedicated
utils module, but it's the matter of time, so let's just proceed with
the patchset for now.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/3] Fix fix-mips64-spare-side-exit-patching

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 04.09.23, Sergey Kaplun via Tarantool-patches wrote:
> This patchset includes 3 pathes to make our tests great again:
> 1) 'MIPS: Fix "bad FP FLOAD" assertion.' is needed for easier testing
>    the patch's correctness (that test still reproduces the problem on
>    MIPS). The patch itself is trivial, as is the test for it.
> 2) 'test: fix `fillmcode()` generator helper' fixes the correctness of
>    fillmcode logic -- somehow the condition of `while` cycle for mcode
>    generation was inverted.
> 3) 'test: fix fix-mips64-spare-side-exit-patching' fixes the possible
>    misbehaviours in the test itself (they were more likely observed before
>    the second patch but still needed to be fixed).
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-fix-flaky-mips-spare-exit
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9090
> Related issues for the first patch:
> * https://github.com/LuaJIT/LuaJIT/issues/1043
> * https://github.com/tarantool/tarantool/issues/8825
> 
> Mike Pall (1):
>   MIPS: Fix "bad FP FLOAD" assertion.
> 
> Sergey Kaplun (2):
>   test: fix `fillmcode()` generator helper
>   test: fix fix-mips64-spare-side-exit-patching
> 
>  src/lj_asm_mips.h                             |  2 +-
>  ...x-mips64-spare-side-exit-patching.test.lua | 22 ++++++++++++++---
>  .../lj-1043-asm-fload.test.lua                | 24 +++++++++++++++++++
>  test/tarantool-tests/utils/jit/generators.lua |  7 +++---
>  4 files changed, 47 insertions(+), 8 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1043-asm-fload.test.lua
> 
> -- 
> 2.42.0
> 

-- 
Best regards,
IM