Eliminate crash in v3_codegen

The following code would crash v3_codegen:

  order(From) ->
    catch
     if
      From#{[] => sufficient} ->
       saint
     end.

Before explaining the crash, first some background on the stack
frame and the Y registers.

Certain instructions, most notably the 'call' instructions, clobber
all X registers. Before any such instruction, all X registers that
have values that will be used after the call must be saved to Y
registers (i.e. to the stack frame). adjust_stack/4 will be called
when X registers must be saved.

There is also another situation when X registers must be saved, namely
within a 'catch' if we are about to execute any instruction that may
cause an exception. Examples of such instructions are some guard BIFs
(such as length/1) and construction of binaries or maps.  Within a
'catch', X registers must be be saved because if an exception is
thrown and catched all X registers will be destroyed.  The same
adjust_stack/4 function will be called for those instructions, but
only if they occur within a 'catch'.

There is actually one more complication. If there is code in
a guard within a catch, the X registers should not be saved, because
the code in a guard never clobbers any X registers that were alive
before the guard code was entered. v3_codegen is written with the
implicit assumption that code in guards never cause anything
to be saved to Y registers.

The code for building maps and binaries would incorrectly save X
registers within a guard inside a 'catch'.

For construction of binaries, that would mean that a useless but
harmelss 'move' instruction was generated.

But for construction of maps, the saving of the Y register would not
be harmless. There would be a crash when attempting to merge #sr{}
records. #sr{} records keeps track of the contents of X and Y
registers. When two separate code paths are joined (e.g. at the end of
'case' statement), the register descriptors must be reconciled.

Basically, the register descriptors for both paths must be identical.
The #sr{} record for one path must not claim that {y,0} contains
a certain value, while another path claims that {y,0} is dead.

Thus, the crash occurs in sr_merge/2 when failing to reconcile the
Y registers.

To fix this bug this bug we will introduce a new function called
maybe_adjust_stack/5. It will save X registers on the stack only
if the code is inside a catch but not inside a guard. We will
change all existing code to use this new function when appropriate.

Reported-by: Thomas Arts

2 files changed, 82 insertions, 32 deletions

diff --git a/lib/compiler/src/v3_codegen.erl b/lib/compiler/src/v3_codegen.erl
index 34c67b16ca..2a89305f4d 100644
--- a/lib/compiler/src/v3_codegen.erl
+++ b/lib/compiler/src/v3_codegen.erl
@@ -1327,12 +1327,13 @@ bif_cg(Bif, As, [{var,V}], Le, Vdb, Bef, St0) ->
     %% that we save any variable that will be live after this BIF call.
 
     MayFail = not erl_bifs:is_safe(erlang, Bif, length(As)),
-    {Sis,Int0} = case St0#cg.in_catch andalso
-		     St0#cg.bfail =:= 0 andalso
-		     MayFail of
-		     true -> adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb);
-		     false -> {[],Bef}
-		 end,
+    {Sis,Int0} =
+	case MayFail of
+	    true ->
+		maybe_adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb, St0);
+	    false ->
+		{[],Bef}
+	end,
     Int1 = clear_dead(Int0, Le#l.i, Vdb),
     Reg = put_reg(V, Int1#sr.reg),
     Int = Int1#sr{reg=Reg},
@@ -1363,11 +1364,7 @@ gc_bif_cg(Bif, As, [{var,V}], Le, Vdb, Bef, St0) ->
     %%   Currently, we are somewhat pessimistic in
     %% that we save any variable that will be live after this BIF call.
 
-    {Sis,Int0} =
-	case St0#cg.in_catch andalso St0#cg.bfail =:= 0 of
-	    true -> adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb);
-	    false -> {[],Bef}
-	end,
+    {Sis,Int0} = maybe_adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb, St0),
 
     Int1 = clear_dead(Int0, Le#l.i, Vdb),
     Reg = put_reg(V, Int1#sr.reg),
@@ -1512,8 +1509,7 @@ set_cg([{var,R}], {cons,Es}, Le, Vdb, Bef, St) ->
     Int1 = Int0#sr{reg=put_reg(R, Int0#sr.reg)},
     Ret = fetch_reg(R, Int1#sr.reg),
     {[{put_list,S1,S2,Ret}], Int1, St};
-set_cg([{var,R}], {binary,Segs}, Le, Vdb, Bef,
-       #cg{in_catch=InCatch, bfail=Bfail}=St) ->
+set_cg([{var,R}], {binary,Segs}, Le, Vdb, Bef, #cg{bfail=Bfail}=St) ->
     %% At run-time, binaries are constructed in three stages:
     %% 1) First the size of the binary is calculated.
     %% 2) Then the binary is allocated.
@@ -1532,11 +1528,7 @@ set_cg([{var,R}], {binary,Segs}, Le, Vdb, Bef,
     %% First generate the code that constructs each field.
     Fail = {f,Bfail},
     PutCode = cg_bin_put(Segs, Fail, Bef),
-    {Sis,Int1} =
-	case InCatch of
-	    true -> adjust_stack(Int0, Le#l.i, Le#l.i+1, Vdb);
-	    false -> {[],Int0}
-	end,
+    {Sis,Int1} = maybe_adjust_stack(Int0, Le#l.i, Le#l.i+1, Vdb, St),
     MaxRegs = max_reg(Bef#sr.reg),
     Aft = clear_dead(Int1, Le#l.i, Vdb),
 
@@ -1545,14 +1537,11 @@ set_cg([{var,R}], {binary,Segs}, Le, Vdb, Bef,
     {Sis++Code,Aft,St};
 % Map single variable key
 set_cg([{var,R}], {map,Op,Map,[{map_pair,{var,_}=K,V}]}, Le, Vdb, Bef,
-       #cg{in_catch=InCatch,bfail=Bfail}=St) ->
+       #cg{bfail=Bfail}=St) ->
 
     Fail = {f,Bfail},
-    {Sis,Int0} =
-	case InCatch of
-	    true -> adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb);
-	    false -> {[],Bef}
-	end,
+    {Sis,Int0} = maybe_adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb, St),
+
     SrcReg = cg_reg_arg(Map,Int0),
     Line = line(Le#l.a),
 
@@ -1573,17 +1562,13 @@ set_cg([{var,R}], {map,Op,Map,[{map_pair,{var,_}=K,V}]}, Le, Vdb, Bef,
 
 % Map (possibly) multiple literal keys
 set_cg([{var,R}], {map,Op,Map,Es}, Le, Vdb, Bef,
-       #cg{in_catch=InCatch,bfail=Bfail}=St) ->
+       #cg{bfail=Bfail}=St) ->
 
     %% assert key literals
     [] = [Var||{map_pair,{var,_}=Var,_} <- Es],
 
     Fail = {f,Bfail},
-    {Sis,Int0} =
-	case InCatch of
-	    true -> adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb);
-	    false -> {[],Bef}
-	end,
+    {Sis,Int0} = maybe_adjust_stack(Bef, Le#l.i, Le#l.i+1, Vdb, St),
     SrcReg = cg_reg_arg(Map,Int0),
     Line = line(Le#l.a),
 
@@ -2038,6 +2023,19 @@ trim_free([R|Rs0]) ->
     end;
 trim_free([]) -> [].
 
+%% maybe_adjust_stack(Bef, FirstBefore, LastFrom, Vdb, St) -> {[Ainstr],Aft}.
+%%  Adjust the stack, but only if the code is inside a catch and not
+%%  inside a guard.  Use this funtion before instructions that may
+%%  cause an exception.
+
+maybe_adjust_stack(Bef, Fb, Lf, Vdb, St) ->
+    case St of
+	#cg{in_catch=true,bfail=0} ->
+	    adjust_stack(Bef, Fb, Lf, Vdb);
+	#cg{} ->
+	    {[],Bef}
+    end.
+
 %% adjust_stack(Bef, FirstBefore, LastFrom, Vdb) -> {[Ainstr],Aft}.
 %%  Do complete stack adjustment by compressing stack and adding
 %%  variables to be saved.  Try to optimise ordering on stack by
diff --git a/lib/compiler/test/guard_SUITE.erl b/lib/compiler/test/guard_SUITE.erl
index 3f073d79cb..47eb1ba78b 100644
--- a/lib/compiler/test/guard_SUITE.erl
+++ b/lib/compiler/test/guard_SUITE.erl
@@ -34,7 +34,8 @@
 	 tricky/1,rel_ops/1,rel_op_combinations/1,literal_type_tests/1,
 	 basic_andalso_orelse/1,traverse_dcd/1,
 	 check_qlc_hrl/1,andalso_semi/1,t_tuple_size/1,binary_part/1,
-	 bad_constants/1,bad_guards/1,scotland/1]).
+	 bad_constants/1,bad_guards/1,scotland/1,
+	 guard_in_catch/1]).
 
 suite() -> [{ct_hooks,[ts_install_cth]}].
 
@@ -52,7 +53,7 @@ groups() ->
        rel_ops,rel_op_combinations,
        literal_type_tests,basic_andalso_orelse,traverse_dcd,
        check_qlc_hrl,andalso_semi,t_tuple_size,binary_part,
-       bad_constants,bad_guards,scotland]}].
+       bad_constants,bad_guards,scotland,guard_in_catch]}].
 
 init_per_suite(Config) ->
     Config.
@@ -1852,6 +1853,57 @@ do_scotland(Echo) ->
 
 found(_, _) -> million.
 
+%% Building maps in a guard in a 'catch' would crash v3_codegen.
+
+guard_in_catch(_Config) ->
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_1(#{}),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_1(#{a=>b}),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_1(atom),
+
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_2(#{}),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_2(#{a=>b}),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_map_2(atom),
+
+    {'EXIT',{if_clause,_}} = (catch do_guard_in_catch_map_3()),
+
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_bin(42),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_bin(<<1,2,3>>),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_bin(atom),
+    {'EXIT',{if_clause,_}} = do_guard_in_catch_bin(#{}),
+
+    ok.
+
+do_guard_in_catch_map_1(From) ->
+    catch
+	if
+	    From#{[] => sufficient} ->
+		saint
+	end.
+
+do_guard_in_catch_map_2(From) ->
+    catch
+	if
+	    From#{From => sufficient} ->
+		saint
+	end.
+
+do_guard_in_catch_map_3() ->
+    try
+	if [] -> solo end
+    catch
+	Friendly when Friendly#{0 => []} -> minutes
+    after
+	membership
+    end.
+
+do_guard_in_catch_bin(From) ->
+    %% Would not crash v3_codegen, but there would be an unnecessary
+    %% 'move' to a Y register.
+    catch
+	if
+	    <<From:32>> ->
+		saint
+	end.
 
 
 %% Call this function to turn off constant propagation.