Skip to content

Commit aed147c

Browse files
Mike Palligormunkin
authored andcommitted
Fix HREFK forwarding vs. table.clear().
Reported by XmiliaH. (cherry-picked from commit d5a237e) When performing HREFK (and also ALOAD, HLOAD) forwarding optimization, the `table.clear()` function call may be performed on the table operand from HREFK between table creation and IR, from which value is forwarded. This call isn't taken in the account, so it may lead to too optimistic value-forwarding from NEWREF (and also ASTORE, HSTORE), or the omitted type guard for HREFK operation. Therefore, this leads to incorrect trace behaviour (for example, taking a non-nil value from the cleared table). This patch adds necessary checks for `table.clear()` calls. Sergey Kaplun: * added the description and the test for the problem Part of tarantool/tarantool#9145 Reviewed-by: Maxim Kokryashkin <[email protected]> Reviewed-by: Sergey Bronnikov <[email protected]> Signed-off-by: Igor Munkin <[email protected]>
1 parent e895818 commit aed147c

File tree

2 files changed

+209
-31
lines changed

2 files changed

+209
-31
lines changed

src/lj_opt_mem.c

Lines changed: 32 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,34 @@ static AliasRet aa_table(jit_State *J, IRRef ta, IRRef tb)
7272
return aa_escape(J, taba, tabb);
7373
}
7474

75+
/* Check whether there's no aliasing table.clear. */
76+
static int fwd_aa_tab_clear(jit_State *J, IRRef lim, IRRef ta)
77+
{
78+
IRRef ref = J->chain[IR_CALLS];
79+
while (ref > lim) {
80+
IRIns *calls = IR(ref);
81+
if (calls->op2 == IRCALL_lj_tab_clear &&
82+
(ta == calls->op1 || aa_table(J, ta, calls->op1) != ALIAS_NO))
83+
return 0; /* Conflict. */
84+
ref = calls->prev;
85+
}
86+
return 1; /* No conflict. Can safely FOLD/CSE. */
87+
}
88+
89+
/* Check whether there's no aliasing NEWREF/table.clear for the left operand. */
90+
int LJ_FASTCALL lj_opt_fwd_tptr(jit_State *J, IRRef lim)
91+
{
92+
IRRef ta = fins->op1;
93+
IRRef ref = J->chain[IR_NEWREF];
94+
while (ref > lim) {
95+
IRIns *newref = IR(ref);
96+
if (ta == newref->op1 || aa_table(J, ta, newref->op1) != ALIAS_NO)
97+
return 0; /* Conflict. */
98+
ref = newref->prev;
99+
}
100+
return fwd_aa_tab_clear(J, lim, ta);
101+
}
102+
75103
/* Alias analysis for array and hash access using key-based disambiguation. */
76104
static AliasRet aa_ahref(jit_State *J, IRIns *refa, IRIns *refb)
77105
{
@@ -154,7 +182,8 @@ static TRef fwd_ahload(jit_State *J, IRRef xref)
154182
IRIns *ir = (xr->o == IR_HREFK || xr->o == IR_AREF) ? IR(xr->op1) : xr;
155183
IRRef tab = ir->op1;
156184
ir = IR(tab);
157-
if (ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) {
185+
if ((ir->o == IR_TNEW || (ir->o == IR_TDUP && irref_isk(xr->op2))) &&
186+
fwd_aa_tab_clear(J, tab, tab)) {
158187
/* A NEWREF with a number key may end up pointing to the array part.
159188
** But it's referenced from HSTORE and not found in the ASTORE chain.
160189
** Or a NEWREF may rehash the table and move unrelated number keys.
@@ -275,7 +304,7 @@ TRef LJ_FASTCALL lj_opt_fwd_hrefk(jit_State *J)
275304
while (ref > tab) {
276305
IRIns *newref = IR(ref);
277306
if (tab == newref->op1) {
278-
if (fright->op1 == newref->op2)
307+
if (fright->op1 == newref->op2 && fwd_aa_tab_clear(J, ref, tab))
279308
return ref; /* Forward from NEWREF. */
280309
else
281310
goto docse;
@@ -285,7 +314,7 @@ TRef LJ_FASTCALL lj_opt_fwd_hrefk(jit_State *J)
285314
ref = newref->prev;
286315
}
287316
/* No conflicting NEWREF: key location unchanged for HREFK of TDUP. */
288-
if (IR(tab)->o == IR_TDUP)
317+
if (IR(tab)->o == IR_TDUP && fwd_aa_tab_clear(J, tab, tab))
289318
fins->t.irt &= ~IRT_GUARD; /* Drop HREFK guard. */
290319
docse:
291320
return CSEFOLD;
@@ -319,34 +348,6 @@ int LJ_FASTCALL lj_opt_fwd_href_nokey(jit_State *J)
319348
return 1; /* No conflict. Can fold to niltv. */
320349
}
321350

322-
/* Check whether there's no aliasing table.clear. */
323-
static int fwd_aa_tab_clear(jit_State *J, IRRef lim, IRRef ta)
324-
{
325-
IRRef ref = J->chain[IR_CALLS];
326-
while (ref > lim) {
327-
IRIns *calls = IR(ref);
328-
if (calls->op2 == IRCALL_lj_tab_clear &&
329-
(ta == calls->op1 || aa_table(J, ta, calls->op1) != ALIAS_NO))
330-
return 0; /* Conflict. */
331-
ref = calls->prev;
332-
}
333-
return 1; /* No conflict. Can safely FOLD/CSE. */
334-
}
335-
336-
/* Check whether there's no aliasing NEWREF/table.clear for the left operand. */
337-
int LJ_FASTCALL lj_opt_fwd_tptr(jit_State *J, IRRef lim)
338-
{
339-
IRRef ta = fins->op1;
340-
IRRef ref = J->chain[IR_NEWREF];
341-
while (ref > lim) {
342-
IRIns *newref = IR(ref);
343-
if (ta == newref->op1 || aa_table(J, ta, newref->op1) != ALIAS_NO)
344-
return 0; /* Conflict. */
345-
ref = newref->prev;
346-
}
347-
return fwd_aa_tab_clear(J, lim, ta);
348-
}
349-
350351
/* ASTORE/HSTORE elimination. */
351352
TRef LJ_FASTCALL lj_opt_dse_ahstore(jit_State *J)
352353
{
Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
local tap = require('tap')
2+
3+
-- Test file to demonstrate LuaJIT incorrect optimizations across
4+
-- the `table.clear()` call.
5+
-- See also: https://github.com/LuaJIT/LuaJIT/issues/792.
6+
7+
local test = tap.test('lj-792-hrefk-table-clear'):skipcond({
8+
['Test requires JIT enabled'] = not jit.status(),
9+
})
10+
local table_clear = require('table.clear')
11+
12+
test:plan(7)
13+
14+
local NITERATIONS = 4
15+
local MAGIC = 42
16+
17+
local function test_aref_fwd_tnew(tab_number)
18+
local field_value_after_clear
19+
for i = 1, NITERATIONS do
20+
-- Create a table on trace to make the optimization work.
21+
-- Initialize the first field to work with the array part.
22+
local tab = {i}
23+
-- Use an additional table to alias the created table with the
24+
-- `1` key.
25+
local tab_array = {tab, {0}}
26+
-- AREF to be forwarded.
27+
tab[1] = MAGIC
28+
table_clear(tab_array[tab_number])
29+
-- It should be `nil`, since table is cleared.
30+
field_value_after_clear = tab[1]
31+
end
32+
return field_value_after_clear
33+
end
34+
35+
local function test_aref_fwd_tdup(tab_number)
36+
local field_value_after_clear
37+
for _ = 1, NITERATIONS do
38+
-- Create a table on trace to make the optimization work.
39+
local tab = {nil}
40+
-- Use an additional table to alias the created table with the
41+
-- `1` key.
42+
local tab_array = {tab, {0}}
43+
-- AREF to be forwarded.
44+
tab[1] = MAGIC
45+
table_clear(tab_array[tab_number])
46+
-- It should be `nil`, since table is cleared.
47+
field_value_after_clear = tab[1]
48+
end
49+
return field_value_after_clear
50+
end
51+
52+
local function test_href_fwd_tnew(tab_number)
53+
local field_value_after_clear
54+
for _ = 1, NITERATIONS do
55+
-- Create a table on trace to make the optimization work.
56+
local tab = {}
57+
-- Use an additional table to alias the created table with the
58+
-- `8` key.
59+
local tab_array = {tab, {0}}
60+
-- HREF to be forwarded. Use 8 to be in the hash part.
61+
tab[8] = MAGIC
62+
table_clear(tab_array[tab_number])
63+
-- It should be `nil`, since table is cleared.
64+
field_value_after_clear = tab[8]
65+
end
66+
return field_value_after_clear
67+
end
68+
69+
local function test_href_fwd_tdup(tab_number)
70+
local field_value_after_clear
71+
for _ = 1, NITERATIONS do
72+
-- Create a table on trace to make the optimization work.
73+
local tab = {nil}
74+
-- Use an additional table to alias the created table with the
75+
-- `8` key.
76+
local tab_array = {tab, {0}}
77+
-- HREF to be forwarded. Use 8 to be in the hash part.
78+
tab[8] = MAGIC
79+
table_clear(tab_array[tab_number])
80+
-- It should be `nil`, since table is cleared.
81+
field_value_after_clear = tab[8]
82+
end
83+
return field_value_after_clear
84+
end
85+
86+
local function test_not_forwarded_hrefk_val_from_newref(tab_number)
87+
local field_value_after_clear
88+
for _ = 1, NITERATIONS do
89+
-- Create a table on trace to make the optimization work.
90+
local tab = {}
91+
-- NEWREF to be forwarded.
92+
tab.hrefk = MAGIC
93+
-- Use an additional table to alias the created table with the
94+
-- `hrefk` key.
95+
local tab_array = {tab, {hrefk = 0}}
96+
table_clear(tab_array[tab_number])
97+
-- It should be `nil`, since it is cleared.
98+
field_value_after_clear = tab.hrefk
99+
end
100+
return field_value_after_clear
101+
end
102+
103+
local function test_not_dropped_guard_on_hrefk(tab_number)
104+
local tab, field_value_after_clear
105+
for _ = 1, NITERATIONS do
106+
-- Create a table on trace to make the optimization work.
107+
tab = {hrefk = MAGIC}
108+
-- Use an additional table to alias the created table with the
109+
-- `hrefk` key.
110+
local tab_array = {tab, {hrefk = 0}}
111+
table_clear(tab_array[tab_number])
112+
-- It should be `nil`, since it is cleared.
113+
-- If the guard is dropped for HREFK, the value from the TDUP
114+
-- table is taken instead, without the type check. This leads
115+
-- to incorrectly returned (swapped) values.
116+
field_value_after_clear = tab.hrefk
117+
tab.hrefk = MAGIC
118+
end
119+
return field_value_after_clear, tab.hrefk
120+
end
121+
122+
jit.opt.start('hotloop=1')
123+
124+
-- First, compile the trace that clears the not-interesting table.
125+
test_aref_fwd_tnew(2)
126+
-- Now run the trace and clear the table, from which we take AREF.
127+
test:is(test_aref_fwd_tnew(1), nil, 'AREF forward from TNEW')
128+
129+
-- XXX: Reset hotcounters to avoid collisions.
130+
jit.opt.start('hotloop=1')
131+
132+
-- First, compile the trace that clears the not-interesting table.
133+
test_aref_fwd_tdup(2)
134+
-- Now run the trace and clear the table, from which we take AREF.
135+
test:is(test_aref_fwd_tdup(1), nil, 'AREF forward from TDUP')
136+
137+
-- XXX: Reset hotcounters to avoid collisions.
138+
jit.opt.start('hotloop=1')
139+
140+
-- First, compile the trace that clears the not-interesting table.
141+
test_href_fwd_tnew(2)
142+
-- Now run the trace and clear the table, from which we take HREF.
143+
test:is(test_href_fwd_tnew(1), nil, 'HREF forward from TNEW')
144+
145+
-- XXX: Reset hotcounters to avoid collisions.
146+
jit.opt.start('hotloop=1')
147+
148+
-- First, compile the trace that clears the not-interesting table.
149+
test_href_fwd_tdup(2)
150+
-- Now run the trace and clear the table, from which we take HREF.
151+
test:is(test_href_fwd_tdup(1), nil, 'HREF forward from TDUP')
152+
153+
-- XXX: Reset hotcounters to avoid collisions.
154+
jit.opt.start('hotloop=1')
155+
156+
-- First, compile the trace that clears the not-interesting table.
157+
test_not_forwarded_hrefk_val_from_newref(2)
158+
-- Now run the trace and clear the table, from which we take
159+
-- HREFK.
160+
local value_from_cleared_tab = test_not_forwarded_hrefk_val_from_newref(1)
161+
162+
test:is(value_from_cleared_tab, nil,
163+
'not forward the field value across table.clear')
164+
165+
-- XXX: Reset hotcounters to avoid collisions.
166+
jit.opt.start('hotloop=1')
167+
168+
-- First, compile the trace that clears the not-interesting table.
169+
test_not_dropped_guard_on_hrefk(2)
170+
-- Now run the trace and clear the table, from which we take
171+
-- HREFK.
172+
local field_value_after_clear, tab_hrefk = test_not_dropped_guard_on_hrefk(1)
173+
174+
test:is(field_value_after_clear, nil, 'correct field value after table.clear')
175+
test:is(tab_hrefk, MAGIC, 'correct value set in the table that was cleared')
176+
177+
test:done(true)

0 commit comments

Comments
 (0)