Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit 1/4] x86/x64: Don't fuse loads across table.clear.
Date: Fri, 10 Jan 2025 16:07:44 +0300	[thread overview]
Message-ID: <9ac2f3e63dc202d948d8b7f38ac3540682fc398b.1736509260.git.skaplun@tarantool.org> (raw)
In-Reply-To: <cover.1736509260.git.skaplun@tarantool.org>

From: Mike Pall <mike>

Reported by Peter Cawley.

(cherry picked from commit 45c88b7963de2969a9a656c03ba06ad995d7fd5f)

Load fusing optimization takes into account only the presence of the
corresponding stores, but not any calls that may affect the table
content. This may lead to the incorrect stores if the fusing
optimization occurs across the `table.clear()` call, leading to
inconsistent behaviour between the JIT and the VM.

This patch adds the corresponding check.

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

Part of tarantool/tarantool#10709
---
 src/lj_asm_x86.h                              |  1 +
 .../lj-1117-fuse-across-table-clear.test.lua  | 36 +++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 test/tarantool-tests/lj-1117-fuse-across-table-clear.test.lua

diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
index 86ce3937..f47c460a 100644
--- a/src/lj_asm_x86.h
+++ b/src/lj_asm_x86.h
@@ -465,6 +465,7 @@ static Reg asm_fuseload(ASMState *as, IRRef ref, RegSet allow)
       }
     } else if (ir->o == IR_ALOAD || ir->o == IR_HLOAD || ir->o == IR_ULOAD) {
       if (noconflict(as, ref, ir->o + IRDELTA_L2S, 0) &&
+	  noconflict(as, ref, IR_CALLS, 0) &&  /* Don't cross table.clear. */
 	  !(LJ_GC64 && irt_isaddr(ir->t))) {
 	asm_fuseahuref(as, ir->op1, xallow);
 	return RID_MRM;
diff --git a/test/tarantool-tests/lj-1117-fuse-across-table-clear.test.lua b/test/tarantool-tests/lj-1117-fuse-across-table-clear.test.lua
new file mode 100644
index 00000000..2f7c91d1
--- /dev/null
+++ b/test/tarantool-tests/lj-1117-fuse-across-table-clear.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+-- Test file to demonstrate LuaJIT's incorrect fusion across
+-- `table.clear()`.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1117.
+local test = tap.test('lj-1117-fuse-across-table-clear'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+local ffi = require('ffi')
+local table_clear = require('table.clear')
+
+test:plan(1)
+
+local tab = {0}
+local alias_tab = tab
+local result_tab = {}
+
+jit.opt.start('hotloop=1')
+
+for i = 1, 4 do
+  -- Load to be fused.
+  local value = tab[1]
+  -- Clear the alias table to trick the code flow analysis.
+  table_clear(alias_tab)
+  -- Need this cast to trigger load fusion. See `asm_comp()` for
+  -- the details. Before the patch, this fusion takes the
+  -- incorrect address of the already cleared table, which leads
+  -- to the failure of the check below.
+  result_tab[i] = ffi.cast('int64_t', value)
+  -- Revive the value.
+  tab[1] = 0
+end
+
+test:samevalues(result_tab, 'no fusion across table.clear')
+
+test:done(true)
-- 
2.47.1


  reply	other threads:[~2025-01-10 13:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-01-10 13:07 [Tarantool-patches] [PATCH luajit 0/4] Fixes for load fusing optimization Sergey Kaplun via Tarantool-patches
2025-01-10 13:07 ` Sergey Kaplun via Tarantool-patches [this message]
2025-01-14 14:10   ` [Tarantool-patches] [PATCH luajit 1/4] x86/x64: Don't fuse loads across table.clear Sergey Bronnikov via Tarantool-patches
2025-01-10 13:07 ` [Tarantool-patches] [PATCH luajit 2/4] Improve last commit Sergey Kaplun via Tarantool-patches
2025-01-14 14:11   ` Sergey Bronnikov via Tarantool-patches
2025-01-10 13:07 ` [Tarantool-patches] [PATCH luajit 3/4] x86/x64: Don't fuse loads across IR_NEWREF Sergey Kaplun via Tarantool-patches
2025-01-14 14:15   ` Sergey Bronnikov via Tarantool-patches
2025-01-10 13:07 ` [Tarantool-patches] [PATCH luajit 4/4] Fix last commit Sergey Kaplun via Tarantool-patches
2025-01-14 14:15   ` Sergey Bronnikov 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=9ac2f3e63dc202d948d8b7f38ac3540682fc398b.1736509260.git.skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 1/4] x86/x64: Don'\''t fuse loads across table.clear.' \
    /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