Skip to content

Commit

Permalink
Fix use-def analysis for BC_VARG.
Browse files Browse the repository at this point in the history
Reported by Ryan Lucia.

(cherry-picked from commit 2801500)

Use-def analysis for BC_VARG has too strong limit for the top/maxslot,
so no slots may be considered as used. This leads to an addititional
SLOAD on the trace with an incorrect value used later. This patch
disables the use-def analysis for BC_VARG as NIY.

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

Part of tarantool/tarantool#8516
Relates to tarantool/tarantool#8718
  • Loading branch information
Mike Pall authored and Buristan committed Jun 28, 2023
1 parent 8ddd7bc commit 8bbc8cf
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 1 deletion.
4 changes: 3 additions & 1 deletion src/lj_snap.c
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
if (!(op == BC_ISTC || op == BC_ISFC)) DEF_SLOT(bc_a(ins));
break;
case BCMbase:
if (op >= BC_CALLM && op <= BC_VARG) {
if (op >= BC_CALLM && op <= BC_ITERN) {
BCReg top = (op == BC_CALLM || op == BC_CALLMT || bc_c(ins) == 0) ?
maxslot : (bc_a(ins) + bc_c(ins)+LJ_FR2);
if (LJ_FR2) DEF_SLOT(bc_a(ins)+1);
Expand All @@ -278,6 +278,8 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
for (s = 0; s < bc_a(ins); s++) DEF_SLOT(s);
return 0;
}
} else if (op == BC_VARG) {
return maxslot; /* NYI: punt. */
} else if (op == BC_KNIL) {
for (s = bc_a(ins); s <= bc_d(ins); s++) DEF_SLOT(s);
} else if (op == BC_TSETM) {
Expand Down
65 changes: 65 additions & 0 deletions test/tarantool-tests/lj-704-bc-varg-use-def.test.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
local tap = require('tap')
-- Test file to demonstrate LuaJIT misbehaviour in use-def
-- snapshot analysis for BC_VARG.
-- See also https://github.com/LuaJIT/LuaJIT/issues/704.
local test = tap.test('lj-704-bc-varg-use-def'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
})

test:plan(1)

-- XXX: we don't really need to store these builtins, but this
-- reduces `jitdump()` output for reader significantly.
local fmod = math.fmod
local pcall = pcall

-- Use the 2 values for `fmod()` to produce non-zero value for
-- the call on trace (the last one call). No special meaning.
local ARG_ON_RECORDING = 6
local ON_TRACE_VALUE = ARG_ON_RECORDING + 1

-- The `jitdump()` output was like the following before the patch:
-- 0003 > num SLOAD #1 T
-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|----]
-- 0004 } tab TNEW #3 #0
-- 0005 > num SLOAD #4 T
-- 0006 p32 FLOAD 0004 tab.array
-- 0007 p32 AREF 0006 +1
-- 0008 } num ASTORE 0007 0005
-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0005]
--
-- The first snapshot misses the 0003 IR in the last slot to be
-- used in the `fmod()` later, so it leads to the additional
-- 0005 SLOAD #4, and storing it in the second snapshot.
--
-- The correct snapshot content after the patch is the following:
-- .... SNAP #1 [`wrap()`|---- pcall|`varg()`|0003]
-- ....
-- .... SNAP #2 [`wrap()`|---- pcall|math.fmod|+6 0003]
local function varg(...)
-- Generate snapshot after `pcall()` with missing slot.
-- The snapshot is generated before each TNEW after the commit
-- 7505e78bd6c24cac6e93f5163675021734801b65 ("Handle on-trace
-- OOM errors from helper functions.")
local slot = ({...})[1]
-- Forcify stitch and usage of vararg slot. Any NIY is OK here.
return fmod(ARG_ON_RECORDING, slot)
end

jit.opt.start('hotloop=1')

local _, result
local function wrap(arg)
-- `pcall()` is needed to emit snapshot to handle on-trace
-- errors.
_, result = pcall(varg, arg)
end
-- Record trace with the 0 result.
wrap(ARG_ON_RECORDING)
wrap(ARG_ON_RECORDING)
-- Record trace with the non-zero result.
wrap(ON_TRACE_VALUE)

test:ok(result ~= 0, 'use-def analysis for BC_VARG')

os.exit(test:check() and 0 or 1)

0 comments on commit 8bbc8cf

Please sign in to comment.