Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Ostanevich <sergos@tarantool.org>,
	Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
Date: Fri,  2 Dec 2022 11:42:20 +0300	[thread overview]
Message-ID: <20221202084220.23122-1-skaplun@tarantool.org> (raw)

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit 05fbdf565c700365d22e38f11478101a0d92a23e)

To reproduce the issue we need to assemble `IR_SLOAD` with
`IRSLOAD_TYPECHECK` flag set. Also, this IR shouldn't be used later and
be not pri, not any number, not any integer type. For GC64 mode, we get
the following mcode for this typecheck only variant of the IR:

| 55557f6bffc9  mov rdi, [rdx+0x4]
| 55557f6bffcd  sar rdi, 0x2f
| 55557f6bffd1  cmp edi, -0x0b
| 55557f6bffd4  jnz 0x55557f6b0010        ->0

This 0x4 addition is excess and crucial:
We got the invalid irtype value to compare (due wrong addressing) -- so
the assertion guard is always failed and we always exit from the trace.

This patch removes this addition.

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

Part of tarantool/tarantool#7230
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-350-fix-sload-typecheck-full-ci
Issues:
* https://github.com/tarantool/tarantool/issues/7230
* https://github.com/LuaJIT/LuaJIT/pull/350
Tarantool PR: https://github.com/tarantool/tarantool/pull/7995

 src/lj_asm_x86.h                              |  2 +-
 .../lj-350-sload-typecheck.test.lua           | 42 +++++++++++++++++++
 .../lj-408-tonumber-cdata-record.test.lua     | 10 -----
 3 files changed, 43 insertions(+), 11 deletions(-)
 create mode 100644 test/tarantool-tests/lj-350-sload-typecheck.test.lua

diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
index 8a4d4025..8efda8e5 100644
--- a/src/lj_asm_x86.h
+++ b/src/lj_asm_x86.h
@@ -1759,7 +1759,7 @@ static void asm_sload(ASMState *as, IRIns *ir)
       emit_i8(as, irt_toitype(t));
       emit_rr(as, XO_ARITHi8, XOg_CMP, tmp);
       emit_shifti(as, XOg_SAR|REX_64, tmp, 47);
-      emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs+4);
+      emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs);
 #else
     } else {
       emit_i8(as, irt_toitype(t));
diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
new file mode 100644
index 00000000..6ffc61fb
--- /dev/null
+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
@@ -0,0 +1,42 @@
+local tap = require('tap')
+local traceinfo = require('jit.util').traceinfo
+
+-- Test file to demonstrate the incorrect GC64 JIT asembling
+-- `IR_SLOAD`.
+-- See also https://github.com/LuaJIT/LuaJIT/pull/350.
+local test = tap.test('lj-350-sload-typecheck')
+
+test:plan(1)
+
+-- Contains only IR_SLOAD after recording.
+local function sload(arg)
+  return arg
+end
+
+local tab_arg = {}
+
+-- Reset JIT, remove any other traces.
+jit.off()
+jit.flush()
+
+assert(not traceinfo(1), 'no traces compiled after flush')
+
+-- Try to executed compiled trace wiht IR_SLOAD, if emitted mcode
+-- is incorrect, assertion guard type check will failed even for
+-- correct type of argument and a new trace is recorded.
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+jit.on()
+
+-- Make the function hot.
+sload(tab_arg)
+-- Compile the trace.
+sload(tab_arg)
+-- Execute trace and try to compile a trace from the side exit.
+sload(tab_arg)
+
+jit.off()
+
+test:ok(not traceinfo(2), 'the second trace should not be compiled')
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
index bf9e8e46..a8235e93 100644
--- a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
+++ b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
@@ -6,7 +6,6 @@ local tap = require('tap')
 -- conversions.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/408,
 -- https://github.com/LuaJIT/LuaJIT/pull/412,
--- https://github.com/LuaJIT/LuaJIT/pull/412,
 -- https://github.com/tarantool/tarantool/issues/7655.
 local test = tap.test('lj-408-tonumber-cdata-record')
 
@@ -14,15 +13,6 @@ local NULL = ffi.cast('void *', 0)
 
 test:plan(4)
 
--- This test won't fail for GC64 on x86_64. This happens due to
--- wrong instruction emitting for SLOAD IR -- we always exit by
--- the assertion guard on the argument type check. See also
--- https://github.com/LuaJIT/LuaJIT/pull/350.
--- The test fails without fix in the current commit, if the
--- following commit is backported:
--- https://github.com/LuaJIT/LuaJIT/commit/05fbdf56
--- Feel free to remove this comment after backporting of the
--- aforementioned commit.
 local function check(x)
   -- Don't use a tail call to avoid "leaving loop in root trace"
   -- error, so the trace will be compiled.
-- 
2.34.1


             reply	other threads:[~2022-12-02  8:45 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-02  8:42 Sergey Kaplun via Tarantool-patches [this message]
2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
2022-12-06 13:06     ` Maxim Kokryashkin via Tarantool-patches
2022-12-15 15:49     ` sergos via Tarantool-patches
2022-12-16  8:13       ` Sergey Kaplun via Tarantool-patches
2023-01-12 14:55 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221202084220.23122-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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