beam_asm: Strenghten the calculation of Uniq for funs

Funs are identified by a triple, <Module,Uniq,Index>, where Module is
the module name, Uniq is a 27 bit hash value of some intermediate
representation of the code for the fun, and index is a small integer.

When a fun is loaded, the triple for the fun will be compared to
previously loaded funs. If all elements in the triple in the newly
loaded fun are the same, the newly loaded fun will replace the previous
fun. The idea is that if Uniq are the same, the code for the fun is also
the same.

The problem is that Uniq is only based on the intermediate representation
of the fun itself. If the fun calls local functions in the same module,
Uniq may remain the same even if the behavior of the fun has been changed.

See

  http://erlang.org/pipermail/erlang-bugs/2007-June/000368.htlm

for an example.

As a long-term plan to fix this problem, the NewIndex and NewUniq
fields was added to each fun in the R8 release (where NewUniq is the
MD5 of the BEAM code for the module). Unfortunately, it turns
out that the compiler does not assign unique value to NewIndex (if it
isn't tested, it doesn't work), so we cannot use the
<Module,NewUniq,NewIndex> triple as identification.

It would be possible to use <Module,NewUniq,Index>, but that seems
ugly. Therefore, fix the problem by making Uniq more unique by
taking 27 bits from the MD5 for the BEAM code. That only requires
a change to the compiler.

Also update a test case for cover, which now fails because of the
stronger Uniq calculation. (The comment in test case about why the
Pid2 process survived is not correct.)

7 files changed, 123 insertions, 52 deletions

diff --git a/erts/doc/src/erlang.xml b/erts/doc/src/erlang.xml
index 45b66a4909..5d7ff7613b 100644
--- a/erts/doc/src/erlang.xml
+++ b/erts/doc/src/erlang.xml
@@ -1196,11 +1196,16 @@ true
           </item>
           <tag><c>{new_uniq, Uniq}</c></tag>
           <item>
-            <p><c>Uniq</c> (a binary) is a unique value for this fun.</p>
+            <p><c>Uniq</c> (a binary) is a unique value for this fun.
+	    It is calculated from the compiled code for the entire module.</p>
           </item>
           <tag><c>{uniq, Uniq}</c></tag>
           <item>
-            <p><c>Uniq</c> (an integer) is a unique value for this fun.</p>
+            <p><c>Uniq</c> (an integer) is a unique value for this fun.
+	    Starting in the R15 release, this integer is calculated from
+	    the compiled code for the entire module. Before R15, this
+	    integer was based on only the body of the fun.
+	    </p>
           </item>
         </taglist>
       </desc>
diff --git a/erts/emulator/test/code_SUITE.erl b/erts/emulator/test/code_SUITE.erl
index 29cbdedd17..6bce448842 100644
--- a/erts/emulator/test/code_SUITE.erl
+++ b/erts/emulator/test/code_SUITE.erl
@@ -25,7 +25,7 @@
 	 t_check_process_code_ets/1,
 	 external_fun/1,get_chunk/1,module_md5/1,make_stub/1,
 	 make_stub_many_funs/1,constant_pools/1,
-	 false_dependency/1,coverage/1]).
+	 false_dependency/1,coverage/1,fun_confusion/1]).
 
 -include_lib("test_server/include/test_server.hrl").
 
@@ -35,7 +35,7 @@ all() ->
     [new_binary_types, t_check_process_code,
      t_check_process_code_ets, t_check_old_code, external_fun, get_chunk,
      module_md5, make_stub, make_stub_many_funs,
-     constant_pools, false_dependency, coverage].
+     constant_pools, false_dependency, coverage, fun_confusion].
 
 groups() -> 
     [].
@@ -555,6 +555,30 @@ coverage(Config) when is_list(Config) ->
     ?line {'EXIT',{badarg,_}} = (catch erlang:module_loaded(42)),
     ok.
 
+fun_confusion(Config) when is_list(Config) ->
+    Data = ?config(data_dir, Config),
+    Src = filename:join(Data, "fun_confusion"),
+    Mod = fun_confusion,
+
+    %% Load first version of module.
+    compile_load(Mod, Src, 1),
+    F1 = Mod:f(),
+    1 = F1(),
+
+    %% Load second version of module.
+    compile_load(Mod, Src, 2),
+    F2 = Mod:f(),
+
+    %% F1 should refer to the old code, not the newly loaded code.
+    1 = F1(),
+    2 = F2(),
+    ok.
+
+compile_load(Mod, Src, Ver) ->
+    {ok,Mod,Code1} = compile:file(Src, [binary,{d,version,Ver}]),
+    {module,Mod} = code:load_binary(Mod, "fun_confusion.beam", Code1),
+    ok.
+
 %% Utilities.
 
 make_sub_binary(Bin) when is_binary(Bin) ->
diff --git a/erts/emulator/test/code_SUITE_data/fun_confusion.erl b/erts/emulator/test/code_SUITE_data/fun_confusion.erl
new file mode 100644
index 0000000000..16000861df
--- /dev/null
+++ b/erts/emulator/test/code_SUITE_data/fun_confusion.erl
@@ -0,0 +1,31 @@
+%%
+%% %CopyrightBegin%
+%% 
+%% Copyright Ericsson AB 2011. All Rights Reserved.
+%% 
+%% The contents of this file are subject to the Erlang Public License,
+%% Version 1.1, (the "License"); you may not use this file except in
+%% compliance with the License. You should have received a copy of the
+%% Erlang Public License along with this software. If not, it can be
+%% retrieved online at http://www.erlang.org/.
+%% 
+%% Software distributed under the License is distributed on an "AS IS"
+%% basis, WITHOUT WARRANTY OF ANY KIND, either express or implied. See
+%% the License for the specific language governing rights and limitations
+%% under the License.
+%% 
+%% %CopyrightEnd%
+%%
+
+-module(fun_confusion).
+
+-export([f/0]).
+
+f() ->
+    fun() -> version() end.
+
+version() ->
+    %% Changing the value returned here should change
+    %% the identity of the fun in f/0.
+    ?version.
+
diff --git a/lib/compiler/src/beam_asm.erl b/lib/compiler/src/beam_asm.erl
index 6e63c4d0f2..9b46abe697 100644
--- a/lib/compiler/src/beam_asm.erl
+++ b/lib/compiler/src/beam_asm.erl
@@ -146,8 +146,10 @@ build_file(Code, Attr, Dict, NumLabels, NumFuncs, Abst, SourceFile, Opts) ->
 
     Essentials0 = [AtomChunk,CodeChunk,StringChunk,ImportChunk,
 		   ExpChunk,LambdaChunk,LiteralChunk],
-    Essentials = [iolist_to_binary(C) || C <- Essentials0],
-    {Attributes,Compile} = build_attributes(Opts, SourceFile, Attr, Essentials),
+    Essentials1 = [iolist_to_binary(C) || C <- Essentials0],
+    MD5 = module_md5(Essentials1),
+    Essentials = finalize_fun_table(Essentials1, MD5),
+    {Attributes,Compile} = build_attributes(Opts, SourceFile, Attr, MD5),
     AttrChunk = chunk(<<"Attr">>, Attributes),
     CompileChunk = chunk(<<"CInf">>, Compile),
 
@@ -166,6 +168,24 @@ build_file(Code, Attr, Dict, NumLabels, NumFuncs, Abst, SourceFile, Opts) ->
 	     end,
     build_form(<<"BEAM">>, Chunks).
 
+%% finalize_fun_table(Essentials, MD5) -> FinalizedEssentials
+%%  Update the 'old_uniq' field in the entry for each fun in the
+%%  'FunT' chunk. We'll use part of the MD5 for the module as a
+%%  unique value.
+
+finalize_fun_table(Essentials, MD5) ->
+    [finalize_fun_table_1(E, MD5) || E <- Essentials].
+
+finalize_fun_table_1(<<"FunT",Keep:8/binary,Table0/binary>>, MD5) ->
+    <<Uniq:27,_:101/bits>> = MD5,
+    Table = finalize_fun_table_2(Table0, Uniq, <<>>),
+    <<"FunT",Keep/binary,Table/binary>>;
+finalize_fun_table_1(Chunk, _) -> Chunk.
+
+finalize_fun_table_2(<<Keep:20/binary,0:32,T/binary>>, Uniq, Acc) ->
+    finalize_fun_table_2(T, Uniq, <<Acc/binary,Keep/binary,Uniq:32>>);
+finalize_fun_table_2(<<>>, _, Acc) -> Acc.
+
 %% Build an IFF form.
 
 build_form(Id, Chunks0) when byte_size(Id) =:= 4, is_list(Chunks0) ->
@@ -202,7 +222,7 @@ flatten_exports(Exps) ->
 flatten_imports(Imps) ->
     list_to_binary(map(fun({M,F,A}) -> <<M:32,F:32,A:32>> end, Imps)).
 
-build_attributes(Opts, SourceFile, Attr, Essentials) ->
+build_attributes(Opts, SourceFile, Attr, MD5) ->
     Misc = case member(slim, Opts) of
 	       false ->
 		   {{Y,Mo,D},{H,Mi,S}} = erlang:universaltime(),
@@ -210,7 +230,7 @@ build_attributes(Opts, SourceFile, Attr, Essentials) ->
 	       true -> []
 	   end,
     Compile = [{options,Opts},{version,?COMPILER_VSN}|Misc],
-    {term_to_binary(calc_vsn(Attr, Essentials)),term_to_binary(Compile)}.
+    {term_to_binary(set_vsn_attribute(Attr, MD5)),term_to_binary(Compile)}.
 
 build_line_table(Dict) ->
     {NumLineInstrs,NumFnames0,Fnames0,NumLines,Lines0} =
@@ -243,32 +263,30 @@ encode_line_items([], _) -> [].
 %% We'll not change an existing 'vsn' attribute.
 %%
 
-calc_vsn(Attr, Essentials0) ->
+set_vsn_attribute(Attr, MD5) ->
     case keymember(vsn, 1, Attr) of
 	true -> Attr;
 	false ->
-	    Essentials = filter_essentials(Essentials0),
-	    <<Number:128>> = erlang:md5(Essentials),
+	    <<Number:128>> = MD5,
 	    [{vsn,[Number]}|Attr]
     end.
 
+module_md5(Essentials0) ->
+    Essentials = filter_essentials(Essentials0),
+    erlang:md5(Essentials).
+
 %% filter_essentials([Chunk]) -> [Chunk']
 %%  Filter essentials so that we obtain the same MD5 as code:module_md5/1 and
-%%  beam_lib:md5/1 would calculate for this module.
+%%  beam_lib:md5/1 would calculate for this module.  Note that at this
+%%  point, the 'old_uniq' entry for each fun in the 'FunT' chunk is zeroed,
+%%  so there is no need to go through the 'FunT' chunk.
 
-filter_essentials([<<"FunT",_Sz:4/binary,Entries:4/binary,Table0/binary>>|T]) ->
-    Table = filter_funtab(Table0, <<0:32>>),
-    [Entries,Table|filter_essentials(T)];
 filter_essentials([<<_Tag:4/binary,Sz:32,Data:Sz/binary,_Padding/binary>>|T]) ->
     [Data|filter_essentials(T)];
 filter_essentials([<<>>|T]) ->
     filter_essentials(T);
 filter_essentials([]) -> [].
 
-filter_funtab(<<Important:20/binary,_OldUniq:4/binary,T/binary>>, Zero) ->
-    [Important,Zero|filter_funtab(T, Zero)];
-filter_funtab(<<>>, _) -> [].
-
 bif_type(fnegate, 1) -> {op,fnegate};
 bif_type(fadd, 2)   -> {op,fadd};
 bif_type(fsub, 2)   -> {op,fsub};
@@ -310,8 +328,8 @@ make_op({test,Cond,Fail,Ops}, Dict) when is_list(Ops) ->
     encode_op(Cond, [Fail|Ops], Dict);
 make_op({test,Cond,Fail,Live,[Op|Ops],Dst}, Dict) when is_list(Ops) ->
     encode_op(Cond, [Fail,Op,Live|Ops++[Dst]], Dict);
-make_op({make_fun2,{f,Lbl},Index,OldUniq,NumFree}, Dict0) ->
-    {Fun,Dict} = beam_dict:lambda(Lbl, Index, OldUniq, NumFree, Dict0),
+make_op({make_fun2,{f,Lbl},Index,_OldUniq,NumFree}, Dict0) ->
+    {Fun,Dict} = beam_dict:lambda(Lbl, Index, NumFree, Dict0),
     make_op({make_fun2,Fun}, Dict);
 make_op({kill,Y}, Dict) ->
     make_op({init,Y}, Dict);
diff --git a/lib/compiler/src/beam_dict.erl b/lib/compiler/src/beam_dict.erl
index ee76623976..f82a8f2e3e 100644
--- a/lib/compiler/src/beam_dict.erl
+++ b/lib/compiler/src/beam_dict.erl
@@ -22,7 +22,7 @@
 
 -export([new/0,opcode/2,highest_opcode/1,
 	 atom/2,local/4,export/4,import/4,
-	 string/2,lambda/5,literal/2,line/2,fname/2,
+	 string/2,lambda/4,literal/2,line/2,fname/2,
 	 atom_table/1,local_table/1,export_table/1,import_table/1,
 	 string_table/1,lambda_table/1,literal_table/1,
 	 line_table/1]).
@@ -133,13 +133,16 @@ string(Str, Dict) when is_list(Str) ->
 	    {NextOffset-Offset,Dict}
     end.
 
-%% Returns the index for a funentry (adding it to the table if necessary).
-%%    lambda(Lbl, Index, Uniq, NumFree, Dict) -> {Index,Dict'}
--spec lambda(label(), non_neg_integer(), integer(), non_neg_integer(), bdict()) ->
+%% Returns the index for a fun entry.
+%%    lambda(Lbl, Index, NumFree, Dict) -> {Index,Dict'}
+-spec lambda(label(), non_neg_integer(), non_neg_integer(), bdict()) ->
         {non_neg_integer(), bdict()}.
 
-lambda(Lbl, Index, OldUniq, NumFree, #asm{lambdas=Lambdas0}=Dict) ->
+lambda(Lbl, Index, NumFree, #asm{lambdas=Lambdas0}=Dict) ->
     OldIndex = length(Lambdas0),
+    %% Initialize OldUniq to 0. It will be set to an unique value
+    %% based on the MD5 checksum of the BEAM code for the module.
+    OldUniq = 0,
     Lambdas = [{Lbl,{OldIndex,Lbl,Index,NumFree,OldUniq}}|Lambdas0],
     {OldIndex,Dict#asm{lambdas=Lambdas}}.
 
diff --git a/lib/tools/test/cover_SUITE.erl b/lib/tools/test/cover_SUITE.erl
index fe7f92de78..881a3c2997 100644
--- a/lib/tools/test/cover_SUITE.erl
+++ b/lib/tools/test/cover_SUITE.erl
@@ -583,21 +583,14 @@ otp_6115_1(Config) ->
     %% called -- running cover compiled code when there is no cover
     %% server and thus no ets tables to bump counters in, makes no
     %% sense.
-    ?line Pid1 = f1:start_fail(),
-
-    %% If f1 is cover compiled, a process P is started with a
-    %% reference to the fun created in start_ok/0, and
-    %% cover:stop() is called, then P should survive.
-    %% This is because (the fun held by) P always references the current
-    %% version of the module, and is thus not affected by the cover
-    %% compiled version being unloaded.
-    ?line Pid2 = f1:start_ok(),
+    Pid1 = f1:start_a(),
+    Pid2 = f1:start_b(),
 
     %% Now stop cover
     ?line cover:stop(),
     
-    %% Ensure that f1 is loaded (and not cover compiled), that Pid1
-    %% is dead and Pid2 is alive, but with no reference to old code
+    %% Ensure that f1 is loaded (and not cover compiled), and that
+    %% both Pid1 and Pid2 are dead.
     case code:which(f1) of
 	Beam when is_list(Beam) ->
 	    ok;
@@ -608,19 +601,15 @@ otp_6115_1(Config) ->
 	undefined ->
 	    ok;
 	_PI1 ->
-	    RefToOldP = erlang:check_process_code(Pid1, f1),
-	    ?line ?t:fail({"Pid1 still alive", RefToOldP})
+	    RefToOldP1 = erlang:check_process_code(Pid1, f1),
+	    ?t:fail({"Pid1 still alive", RefToOldP1})
     end,
     case process_info(Pid2) of
-	PI2 when is_list(PI2) ->
-	    case erlang:check_process_code(Pid2, f2) of
-		false ->
-		    ok;
-		true ->
-		    ?line ?t:fail("Pid2 has ref to old code")
-	    end;
 	undefined ->
-	    ?line ?t:fail("Pid2 has died")
+	    ok;
+	_PI2 ->
+	    RefToOldP2 = erlang:check_process_code(Pid1, f2),
+	    ?t:fail({"Pid2 still alive", RefToOldP2})
     end,
 
     ?line file:set_cwd(CWD),
diff --git a/lib/tools/test/cover_SUITE_data/otp_6115/f1.erl b/lib/tools/test/cover_SUITE_data/otp_6115/f1.erl
index b659e5d818..5399b33f19 100644
--- a/lib/tools/test/cover_SUITE_data/otp_6115/f1.erl
+++ b/lib/tools/test/cover_SUITE_data/otp_6115/f1.erl
@@ -1,12 +1,13 @@
 -module(f1).
--export([start_fail/0, start_ok/0]).
+-export([start_a/0, start_b/0]).
 
-start_fail() ->
+start_a() ->
     f2:start(fun() -> 
-		     io:format("this does not work\n",[]) 
+		     ok
 	     end).
 
-start_ok() ->
+start_b() ->
     f2:start(fun fun1/0).
+
 fun1() ->
-    io:format("this works\n",[]).
+    ok.