aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorJohn Högberg <[email protected]>2019-02-26 12:19:44 +0100
committerJohn Högberg <[email protected]>2019-02-26 16:18:46 +0100
commitce60cb4e22f03219452b06db0ac7500a5fc884ea (patch)
tree566f97f480b9a65f6ba9ebf339442c9b62a3341a
parentcd7fa515675adf2551887b6e5ad6ba8d08814413 (diff)
downloadotp-ce60cb4e22f03219452b06db0ac7500a5fc884ea.tar.gz
otp-ce60cb4e22f03219452b06db0ac7500a5fc884ea.tar.bz2
otp-ce60cb4e22f03219452b06db0ac7500a5fc884ea.zip
beam_jump: Fail label of select_val is unsafe for move elimination
Consider the following code: bme(Int) -> TagInt = Int band 2#111, Tag = case TagInt of 0 -> a; 1 -> b; 2 -> c; 3 -> d; 4 -> e; 5 -> f; 6 -> g; 7 -> h end, case Tag of g -> expects_g(TagInt, Tag); h -> expects_h(TagInt, Tag); _ -> Tag = id(Tag), ok end. expects_g(6, Atom) -> Atom = id(g), ok. expects_h(7, Atom) -> Atom = id(h), ok. The type optimization pass would recognize that TagInt can only be [0 .. 7], so the first 'case' would select_val over [0 .. 6] and swap out the fail label with the block for 7. A later optimization would merge this block with 'expects_h' in the second case, as the latter is only reachable from the former. ... but this broke down when the move elimination optimization didn't take the fail label of the first select_val into account. This caused it believe that the only way to reach 'expects_h' was through the second case when 'Tag' =:= 'h', which made it remove the move instruction added in the first case, passing garbage to expects_h/2.
-rw-r--r--lib/compiler/src/beam_jump.erl8
-rw-r--r--lib/compiler/test/beam_jump_SUITE.erl47
2 files changed, 49 insertions, 6 deletions
diff --git a/lib/compiler/src/beam_jump.erl b/lib/compiler/src/beam_jump.erl
index 6f50bfdb9c..74f80ca70e 100644
--- a/lib/compiler/src/beam_jump.erl
+++ b/lib/compiler/src/beam_jump.erl
@@ -179,8 +179,9 @@ function({function,Name,Arity,CLabel,Asm0}, Lc0) ->
eliminate_moves(Is) ->
eliminate_moves(Is, #{}, []).
-eliminate_moves([{select,select_val,Reg,_,List}=I|Is], D0, Acc) ->
- D = update_value_dict(List, Reg, D0),
+eliminate_moves([{select,select_val,Reg,{f,Fail},List}=I|Is], D0, Acc) ->
+ D1 = add_unsafe_label(Fail, D0),
+ D = update_value_dict(List, Reg, D1),
eliminate_moves(Is, D, [I|Acc]);
eliminate_moves([{test,is_eq_exact,_,[Reg,Val]}=I,
{block,BlkIs0}|Is], D0, Acc) ->
@@ -229,6 +230,9 @@ update_value_dict([Lit,{f,Lbl}|T], Reg, D0) ->
update_value_dict(T, Reg, D);
update_value_dict([], _, D) -> D.
+add_unsafe_label(L, D) ->
+ D#{L=>unsafe}.
+
update_unsafe_labels(I, D) ->
Ls = instr_labels(I),
update_unsafe_labels_1(Ls, D).
diff --git a/lib/compiler/test/beam_jump_SUITE.erl b/lib/compiler/test/beam_jump_SUITE.erl
index 759d884dc4..a456f31d79 100644
--- a/lib/compiler/test/beam_jump_SUITE.erl
+++ b/lib/compiler/test/beam_jump_SUITE.erl
@@ -79,12 +79,13 @@ checks(Wanted) ->
{catch case river() of sheet -> begin +Wanted, if "da" -> Wanted end end end, catch case river() of sheet -> begin + Wanted, if "da" -> Wanted end end end}.
unsafe_move_elimination(_Config) ->
- {{left,right,false},false} = unsafe_move_elimination(left, right, false),
- {{false,right,false},false} = unsafe_move_elimination(false, right, true),
- {{true,right,right},right} = unsafe_move_elimination(true, right, true),
+ {{left,right,false},false} = unsafe_move_elimination_1(left, right, false),
+ {{false,right,false},false} = unsafe_move_elimination_1(false, right, true),
+ {{true,right,right},right} = unsafe_move_elimination_1(true, right, true),
+ [ok = unsafe_move_elimination_2(I) || I <- lists:seq(0,16)],
ok.
-unsafe_move_elimination(Left, Right, Simple0) ->
+unsafe_move_elimination_1(Left, Right, Simple0) ->
id(1),
%% The move at label 29 would be removed by beam_jump, which is unsafe because
@@ -115,6 +116,44 @@ unsafe_move_elimination(Left, Right, Simple0) ->
end,
{id({Left,Right,Simple}),Simple}.
+unsafe_move_elimination_2(Int) ->
+ %% The type optimization pass would recognize that TagInt can only be
+ %% [0 .. 7], so the first 'case' would select_val over [0 .. 6] and swap
+ %% out the fail label with the block for 7.
+ %%
+ %% A later optimization would merge this block with 'expects_h' in the
+ %% second case, as the latter is only reachable from the former.
+ %%
+ %% ... but this broke down when the move elimination optimization didn't
+ %% take the fail label of the first select_val into account. This caused it
+ %% to believe that the only way to reach 'expects_h' was through the second
+ %% case when 'Tag' =:= 'h', which made it remove the move instruction
+ %% added in the first case, passing garbage to expects_h/2.
+ TagInt = Int band 2#111,
+ Tag = case TagInt of
+ 0 -> a;
+ 1 -> b;
+ 2 -> c;
+ 3 -> d;
+ 4 -> e;
+ 5 -> f;
+ 6 -> g;
+ 7 -> h
+ end,
+ case Tag of
+ g -> expects_g(TagInt, Tag);
+ h -> expects_h(TagInt, Tag);
+ _ -> Tag = id(Tag), ok
+ end.
+
+expects_g(6, Atom) ->
+ Atom = id(g),
+ ok.
+
+expects_h(7, Atom) ->
+ Atom = id(h),
+ ok.
+
-record(message2, {id, p1}).
-record(message3, {id, p1, p2}).