From f6a2fb81fd87e671ceed85261a1c28a6082f010c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Tue, 12 Oct 2010 10:08:18 +0200
Subject: beam_block: Do optimizations in the safe order

Moving of allocation instructions upwards in the instruction
stream (in order to enable further optimizations) in beam_block,
is implemented with the assumption that if a register {x,X}
contains a valid term, then all other x register with lower
numbers than X also contain valid terms. That assumption is
true after code generation.

The beam_utils:live_opt/1 optimization, however, may invalidate
that assumption. For instance, if a receive statement exports a
variable that is used, but the return value of the receive statement
is not used, then {x,1} but not {x,0} contains a valid term at the
end of the receive statement. If the receive statement is
followed by

    {bif,self,{f,0},[],{x,0}}.
    {test_heap,NumberOfWords,2}.

moving the allocation upwards will produce

    {test_heap,NumberOfWords,2}.
    {bif,self,{f,0},[],{x,0}}.

which will cause the beam_validator pass to scream loudly that
{x,0} is not live at the test_heap instruction.

Fix the problem by doing the optimizations in reverse order.

Reported-by: Jim Engquist
---
 lib/compiler/src/beam_block.erl     | 35 +++++++++++++++++++++--------------
 lib/compiler/test/receive_SUITE.erl | 25 +++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 16 deletions(-)

(limited to 'lib')

diff --git a/lib/compiler/src/beam_block.erl b/lib/compiler/src/beam_block.erl
index 9c6f835ab0..c45874597a 100644
--- a/lib/compiler/src/beam_block.erl
+++ b/lib/compiler/src/beam_block.erl
@@ -36,12 +36,13 @@ function({function,Name,Arity,CLabel,Is0}, Lc0) ->
 
 	%% Collect basic blocks and optimize them.
 	Is2 = blockify(Is1),
-	Is3 = beam_utils:live_opt(Is2),
-	Is4 = opt_blocks(Is3),
-	Is5 = beam_utils:delete_live_annos(Is4),
+	Is3 = move_allocates(Is2),
+	Is4 = beam_utils:live_opt(Is3),
+	Is5 = opt_blocks(Is4),
+	Is6 = beam_utils:delete_live_annos(Is5),
 
 	%% Optimize bit syntax.
-	{Is,Lc} = bsm_opt(Is5, Lc0),
+	{Is,Lc} = bsm_opt(Is6, Lc0),
 
 	%% Done.
 	{{function,Name,Arity,CLabel,Is},Lc}
@@ -156,11 +157,7 @@ opt_blocks([I|Is]) ->
 opt_blocks([]) -> [].
 
 opt_block(Is0) ->
-    %% We explicitly move any allocate instruction upwards before optimising
-    %% moves, to avoid any potential problems with the calculation of live
-    %% registers.
-    Is1 = move_allocates(Is0),
-    Is = find_fixpoint(fun opt/1, Is1),
+    Is = find_fixpoint(fun opt/1, Is0),
     opt_alloc(Is).
 
 find_fixpoint(OptFun, Is0) ->
@@ -170,11 +167,21 @@ find_fixpoint(OptFun, Is0) ->
     end.
 
 %% move_allocates(Is0) -> Is
-%%  Move allocates upwards in the instruction stream, in the hope of
-%%  getting more possibilities for optimizing away moves later.
-
-move_allocates(Is) ->
-    move_allocates_1(reverse(Is), []).
+%%  Move allocate instructions upwards in the instruction stream, in the
+%%  hope of getting more possibilities for optimizing away moves later.
+%%
+%%  NOTE: Moving allocation instructions is only safe because it is done
+%%  immediately after code generation so that we KNOW that if {x,X} is
+%%  initialized, all x registers with lower numbers are also initialized.
+%%  That assumption may not be true after other optimizations, such as
+%%  the beam_utils:live_opt/1 optimization.
+
+move_allocates([{block,Bl0}|Is]) ->
+    Bl = move_allocates_1(reverse(Bl0), []),
+    [{block,Bl}|move_allocates(Is)];
+move_allocates([I|Is]) ->
+    [I|move_allocates(Is)];
+move_allocates([]) -> [].
 
 move_allocates_1([{set,[],[],{alloc,_,_}=Alloc}|Is0], Acc0) ->
     {Is,Acc} = move_allocates_2(Alloc, Is0, Acc0),
diff --git a/lib/compiler/test/receive_SUITE.erl b/lib/compiler/test/receive_SUITE.erl
index fca3f0387b..2a592dd669 100644
--- a/lib/compiler/test/receive_SUITE.erl
+++ b/lib/compiler/test/receive_SUITE.erl
@@ -21,7 +21,7 @@
 -module(receive_SUITE).
 
 -export([all/1,init_per_testcase/2,fin_per_testcase/2,
-	 recv/1,coverage/1,otp_7980/1,ref_opt/1]).
+	 recv/1,coverage/1,otp_7980/1,ref_opt/1,export/1]).
 
 -include("test_server.hrl").
 
@@ -36,7 +36,7 @@ fin_per_testcase(_Case, Config) ->
 
 all(suite) ->
     test_lib:recompile(?MODULE),
-    [recv,coverage,otp_7980,ref_opt].
+    [recv,coverage,otp_7980,ref_opt,export].
 
 -record(state, {ena = true}).
 
@@ -205,4 +205,25 @@ collect_recv_opt_instrs(Code) ->
 		end] || {function,_,_,_,Is} <- Code],
     lists:append(L).
 
+export(Config) when is_list(Config) ->
+    Ref = make_ref(),
+    ?line self() ! {result,Ref,42},
+    ?line 42 = export_1(Ref),
+    ?line {error,timeout} = export_1(Ref),
+    ok.
+
+export_1(Reference) ->
+    id(Reference),
+    receive
+	{result,Reference,Result} ->
+	    Result
+    after 1 ->
+	    Result = {error,timeout}
+    end,
+    %% Result ({x,1}) is used, but not the return value ({x,0})
+    %% of the receive. Used to be incorrectly optimized
+    %% by beam_block.
+    id({build,self()}),
+    Result.
+
 id(I) -> I.
-- 
cgit v1.2.3