Skip to content

Commit 3a2e484

Browse files
Mike Palligormunkin
authored andcommitted
Detect inconsistent renames even in the presence of sunk values.
Reported by Igor Munkin. (cherry picked from commit 33e3f4b) Side exits with the same exitno use the same snapshot for restoring guest stack values. This obliges all guards related to the particular snapshot use the same RegSP mapping for the values to be restored at the trace exit. RENAME emitted prior to the guard for the same snapshot leads to the aforementioned invariant violation. The easy way to save the snapshot consistency is spilling the renamed IR reference, that is done in scope of <asm_snap_checkrename>. However, the previous <asm_snap_checkrename> implementation considers only the IR references explicitly mentioned in the snapshot. E.g. if there is a sunk[1] object to be restored at the trace exit, and the renamed reference is a *STORE to that object, the spill slot is not allocated. As a result an invalid value is stored while unsinking that object at all corresponding side exits prior to the emitted renaming. To handle also those IR references implicitly used in the snapshot, all non-constant and non-sunk references are added to the Bloom filter (it's worth to mention that two hash functions are used to reduce collisions for the cases when the number of IR references emitted between two different snapshots exceeds the filter size). New <asm_snap_checkrename> implementation tests whether the renamed IR reference is in the filter and forces a spill slot for it as a result. [1]: http://wiki.luajit.org/Allocation-Sinking-Optimization Igor Munkin: * added the description and the test for the problem Resolves tarantool/tarantool#5118 Follows up tarantool/tarantool#4252 Reviewed-by: Sergey Ostanevich <sergos@tarantool.org> Reviewed-by: Sergey Kaplun <skaplun@tarantool.org> Signed-off-by: Igor Munkin <imun@tarantool.org>
1 parent e7f7016 commit 3a2e484

File tree

2 files changed

+109
-13
lines changed

2 files changed

+109
-13
lines changed

src/lj_asm.c

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ typedef struct ASMState {
7272
IRRef snaprename; /* Rename highwater mark for snapshot check. */
7373
SnapNo snapno; /* Current snapshot number. */
7474
SnapNo loopsnapno; /* Loop snapshot number. */
75+
BloomFilter snapfilt1, snapfilt2; /* Filled with snapshot refs. */
7576

7677
IRRef fuseref; /* Fusion limit (loopref, 0 or FUSE_DISABLED). */
7778
IRRef sectref; /* Section base reference (loopref or 0). */
@@ -876,7 +877,10 @@ static int asm_sunk_store(ASMState *as, IRIns *ira, IRIns *irs)
876877
static void asm_snap_alloc1(ASMState *as, IRRef ref)
877878
{
878879
IRIns *ir = IR(ref);
879-
if (!irref_isk(ref) && (!(ra_used(ir) || ir->r == RID_SUNK))) {
880+
if (!irref_isk(ref) && ir->r != RID_SUNK) {
881+
bloomset(as->snapfilt1, ref);
882+
bloomset(as->snapfilt2, hashrot(ref, ref + HASH_BIAS));
883+
if (ra_used(ir)) return;
880884
if (ir->r == RID_SINK) {
881885
ir->r = RID_SUNK;
882886
#if LJ_HASFFI
@@ -933,6 +937,7 @@ static void asm_snap_alloc(ASMState *as)
933937
SnapShot *snap = &as->T->snap[as->snapno];
934938
SnapEntry *map = &as->T->snapmap[snap->mapofs];
935939
MSize n, nent = snap->nent;
940+
as->snapfilt1 = as->snapfilt2 = 0;
936941
for (n = 0; n < nent; n++) {
937942
SnapEntry sn = map[n];
938943
IRRef ref = snap_ref(sn);
@@ -955,18 +960,12 @@ static void asm_snap_alloc(ASMState *as)
955960
*/
956961
static int asm_snap_checkrename(ASMState *as, IRRef ren)
957962
{
958-
SnapShot *snap = &as->T->snap[as->snapno];
959-
SnapEntry *map = &as->T->snapmap[snap->mapofs];
960-
MSize n, nent = snap->nent;
961-
for (n = 0; n < nent; n++) {
962-
SnapEntry sn = map[n];
963-
IRRef ref = snap_ref(sn);
964-
if (ref == ren || (LJ_SOFTFP && (sn & SNAP_SOFTFPNUM) && ++ref == ren)) {
965-
IRIns *ir = IR(ref);
966-
ra_spill(as, ir); /* Register renamed, so force a spill slot. */
967-
RA_DBGX((as, "snaprensp $f $s", ref, ir->s));
968-
return 1; /* Found. */
969-
}
963+
if (bloomtest(as->snapfilt1, ren) &&
964+
bloomtest(as->snapfilt2, hashrot(ren, ren + HASH_BIAS))) {
965+
IRIns *ir = IR(ren);
966+
ra_spill(as, ir); /* Register renamed, so force a spill slot. */
967+
RA_DBGX((as, "snaprensp $f $s", ren, ir->s));
968+
return 1; /* Found. */
970969
}
971970
return 0; /* Not found. */
972971
}
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
local tap = require('tap')
2+
3+
local test = tap.test('lj-584-bad-renames-for-sunk-values')
4+
test:plan(1)
5+
6+
-- Test file to demonstrate LuaJIT assembler misbehaviour.
7+
-- For more info, proceed to the issues:
8+
-- * https://github.com/LuaJIT/LuaJIT/issues/584
9+
-- * https://github.com/tarantool/tarantool/issues/4252
10+
11+
----- Related part of luafun.lua. --------------------------------
12+
13+
local iterator_mt = {
14+
__call = function(self, param, state) return self.gen(param, state) end,
15+
}
16+
17+
local wrap = function(gen, param, state)
18+
return setmetatable({
19+
gen = gen,
20+
param = param,
21+
state = state
22+
}, iterator_mt), param, state
23+
end
24+
25+
-- These functions call each other to implement a flat iterator
26+
-- over the several iterable objects.
27+
local chain_gen_r1, chain_gen_r2
28+
29+
chain_gen_r2 = function(param, state, state_x, ...)
30+
if state_x ~= nil then return { state[1], state_x }, ... end
31+
local i = state[1] + 1
32+
if param[3 * i - 1] == nil then return nil end
33+
return chain_gen_r1(param, { i, param[3 * i] })
34+
end
35+
36+
chain_gen_r1 = function(param, state)
37+
local i, state_x = state[1], state[2]
38+
local gen_x, param_x = param[3 * i - 2], param[3 * i - 1]
39+
return chain_gen_r2(param, state, gen_x(param_x, state_x))
40+
end
41+
42+
local chain = function(...)
43+
local param = { }
44+
for i = 1, select('#', ...) do
45+
-- Put gen, param, state into param table.
46+
param[3 * i - 2], param[3 * i - 1], param[3 * i]
47+
= wrap(ipairs(select(i, ...)))
48+
end
49+
return wrap(chain_gen_r1, param, { 1, param[3] })
50+
end
51+
52+
----- Reproducer. ------------------------------------------------
53+
54+
-- XXX: Here one can find the rationale for the 'hotloop' value.
55+
-- 1. The most inner while loop on the line 86 starts recording
56+
-- for the third element (i.e. 'c') and successfully compiles
57+
-- as TRACE 1. However, its execution stops, since type guard
58+
-- for <gen_x> result value on line 39 is violated (nil is
59+
-- returned from <ipairs_aux>) and trace execution is stopped.
60+
-- 2. Next time TRACE 1 enters the field is iterating through the
61+
-- second table given to <chain>. Its execution also stops at
62+
-- the similar assertion but in the variant part this time.
63+
-- 3. <wrap> function becomes hot enough while building new
64+
-- <chain> iterator, and it is compiled as TRACE 2.
65+
-- There are also other attempts, but all of them failed.
66+
-- 4. Again, TRACE 1 reigns while iterating through the first
67+
-- table given to <chain> and finishes at the same guard the
68+
-- previous run does. Anyway, everything above is just an
69+
-- auxiliary activity preparing the JIT environment for the
70+
-- following result.
71+
-- 5. Here we finally come: <chain_gen_r1> is finally ready to be
72+
-- recorded. It successfully compiles as TRACE 3. However, the
73+
-- boundary case is recorded, so the trace execution stops
74+
-- since nil *is not* returned from <ipairs_aux> on the next
75+
-- iteration.
76+
--
77+
-- JIT fine tuning via 'hotloop' option allows to catch this
78+
-- elusive case, we achieved in a last bullet. The reason, why
79+
-- this case leads to a misbehaviour while restoring the guest
80+
-- stack at the trace exit, is described in the following LuaJIT
81+
-- issue: https://github.com/LuaJIT/LuaJIT/issues/584.
82+
jit.opt.start('hotloop=3')
83+
84+
xpcall(function()
85+
for _ = 1, 3 do
86+
local gen_x, param_x, state_x = chain({ 'a', 'b', 'c' }, { 'q', 'w', 'e' })
87+
while true do
88+
state_x = gen_x(param_x, state_x)
89+
if state_x == nil then break end
90+
end
91+
end
92+
test:ok('All emitted RENAMEs are fine')
93+
end, function()
94+
test:fail('Invalid Lua stack has been restored')
95+
end)
96+
97+
os.exit(test:check() and 0 or 1)

0 commit comments

Comments
 (0)