From 85701edb6bdfe7c616a8486542dc2ca4bd787113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= Date: Wed, 14 Jan 2015 07:05:21 +0100 Subject: sys_core_fold: Correct optimization of 'case' The optimization of a 'case' statement could lead to incorrect code that would cause an exception at run-time. Here is an example to show how the optimization went wrong. Start with the following code: f({r,#{key:=Val},X}=S) -> case S of {r,_,_} -> setelement(3, Val, S) end. (The record operations have already been translated to the corresponding tuple operations.) The first step in case_opt/3 is to substitute S to obtain: f({r,#{key:=Val},X}=S) -> case {r,#{key:=Val},X} of {r,_,_} -> setelement(3, Val, S) end. After that substitution the 'case' can be simplified to: f({r,#{key:=Val},_}=S) -> case #{key:=Val} of NewVar -> setelement(3, Val, S) end. That is the result from case_opt/3. Now eval_case/2 notices that since there is only one clause left in the 'case', the 'case' can eliminated: f({r,#{key:=Val},_}=S) -> NewVar = #{key:=Val}, setelement(3, Val, S). Since the map construction may have a side effect, it was not eliminated, but assigned to a variable that is never used. The problem is that '#{key:=Val}' is fine as a pattern, but in a construction of a new map, the '=>' operator must be used. So the map construction will fail, generating an exception. As a conservative correction for a maintenance release, we will abort the 'case' optimization if the substitution into the 'case' expression is anything but data items (tuples, conses, or literals) or variables. Reported-by: Dmitry Aleksandrov --- lib/compiler/src/sys_core_fold.erl | 42 ++++++++++++++++++++++++++++++++------ lib/compiler/test/match_SUITE.erl | 16 +++++++++++++-- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/lib/compiler/src/sys_core_fold.erl b/lib/compiler/src/sys_core_fold.erl index 82817a987a..09716d0866 100644 --- a/lib/compiler/src/sys_core_fold.erl +++ b/lib/compiler/src/sys_core_fold.erl @@ -2072,17 +2072,47 @@ maybe_replace_var_1(E, #sub{t=Tdb}) -> false -> E; true -> - cerl_trees:map(fun(C) -> - case cerl:is_c_alias(C) of - false -> C; - true -> cerl:alias_pat(C) - end - end, T0) + %% The pattern was a tuple. Now we must make sure + %% that the elements of the tuple are suitable. In + %% particular, we don't want binary or map + %% construction here, since that means that the + %% binary or map will be constructed in the 'case' + %% argument. That is wasteful for binaries. Even + %% worse is that any map pattern that use the ':=' + %% operator will fail when used in map + %% construction (only the '=>' operator is allowed + %% when constructing a map from scratch). + ToData = fun coerce_to_data/1, + try + cerl_trees:map(ToData, T0) + catch + throw:impossible -> + %% Something unsuitable was found (map or + %% or binary). Keep the variable. + E + end end; error -> E end. +%% coerce_to_data(Core) -> Core' +%% Coerce an element originally from a pattern to an data item or or +%% variable. Throw an 'impossible' exception if non-data Core Erlang +%% terms such as binary construction or map construction are +%% encountered. + +coerce_to_data(C) -> + case cerl:is_c_alias(C) of + false -> + case cerl:is_data(C) orelse cerl:is_c_var(C) of + true -> C; + false -> throw(impossible) + end; + true -> + coerce_to_data(cerl:alias_pat(C)) + end. + %% case_opt_lit(Literal, Clauses0, LitExpr) -> %% {ok,[],Clauses} | error %% The current part of the case expression is a literal. That diff --git a/lib/compiler/test/match_SUITE.erl b/lib/compiler/test/match_SUITE.erl index ae7d764535..e5aaf49d6f 100644 --- a/lib/compiler/test/match_SUITE.erl +++ b/lib/compiler/test/match_SUITE.erl @@ -22,7 +22,7 @@ init_per_group/2,end_per_group/2, pmatch/1,mixed/1,aliases/1,match_in_call/1, untuplify/1,shortcut_boolean/1,letify_guard/1, - selectify/1,underscore/1,coverage/1]). + selectify/1,underscore/1,match_map/1,coverage/1]). -include_lib("test_server/include/test_server.hrl"). @@ -35,7 +35,8 @@ all() -> groups() -> [{p,test_lib:parallel(), [pmatch,mixed,aliases,match_in_call,untuplify, - shortcut_boolean,letify_guard,selectify,underscore,coverage]}]. + shortcut_boolean,letify_guard,selectify, + underscore,match_map,coverage]}]. init_per_suite(Config) -> @@ -400,6 +401,17 @@ underscore(Config) when is_list(Config) -> _ = is_list(Config), ok. +-record(s, {map,t}). + +match_map(Config) when is_list(Config) -> + Map = #{key=>{x,y},ignore=>anything}, + #s{map=Map,t={x,y}} = do_match_map(#s{map=Map}), + ok. + +do_match_map(#s{map=#{key:=Val}}=S) -> + %% Would crash with a 'badarg' exception. + S#s{t=Val}. + coverage(Config) when is_list(Config) -> %% Cover beam_dead. ok = coverage_1(x, a), -- cgit v1.2.3 From bd3ba08000a258818e52410c73fcb195db88356f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= Date: Tue, 13 Jan 2015 05:27:04 +0100 Subject: beam_bool: Correct indentation for try...catch Old versions of the Erlang mode for Emacs used to indent try...catch strangely - the first clause following the 'catch' would be indented with one space less than the following clauses. If we are to use the new Erlang mode when we add more clauses, they would be indented with one space less than the preceding clauses. That would look silly. --- lib/compiler/src/beam_bool.erl | 52 +++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/compiler/src/beam_bool.erl b/lib/compiler/src/beam_bool.erl index 5a4621dc37..dc4d7927e9 100644 --- a/lib/compiler/src/beam_bool.erl +++ b/lib/compiler/src/beam_bool.erl @@ -126,43 +126,43 @@ bopt_block(Reg, Fail, OldIs, [{block,Bl0}|Acc0], St0) -> %% There was a reference to a boolean expression %% from inside a protected block (try/catch), to %% a boolean expression outside. - throw:protected_barrier -> + throw:protected_barrier -> failed; - %% The 'xor' operator was used. We currently don't - %% find it worthwile to translate 'xor' operators - %% (the code would be clumsy). - throw:'xor' -> + %% The 'xor' operator was used. We currently don't + %% find it worthwile to translate 'xor' operators + %% (the code would be clumsy). + throw:'xor' -> failed; - %% The block does not contain a boolean expression, - %% but only a call to a guard BIF. - %% For instance: ... when element(1, T) -> - throw:not_boolean_expr -> + %% The block does not contain a boolean expression, + %% but only a call to a guard BIF. + %% For instance: ... when element(1, T) -> + throw:not_boolean_expr -> failed; - %% The block contains a 'move' instruction that could - %% not be handled. - throw:move -> + %% The block contains a 'move' instruction that could + %% not be handled. + throw:move -> failed; - %% The optimization is not safe. (A register - %% used by the instructions following the - %% optimized code is either not assigned a - %% value at all or assigned a different value.) - throw:all_registers_not_killed -> + %% The optimization is not safe. (A register + %% used by the instructions following the + %% optimized code is either not assigned a + %% value at all or assigned a different value.) + throw:all_registers_not_killed -> failed; - throw:registers_used -> + throw:registers_used -> failed; - %% A protected block refered to the value - %% returned by another protected block, - %% probably because the Core Erlang code - %% used nested try/catches in the guard. - %% (v3_core never produces nested try/catches - %% in guards, so it must have been another - %% Core Erlang translator.) - throw:protected_violation -> + %% A protected block refered to the value + %% returned by another protected block, + %% probably because the Core Erlang code + %% used nested try/catches in the guard. + %% (v3_core never produces nested try/catches + %% in guards, so it must have been another + %% Core Erlang translator.) + throw:protected_violation -> failed end end. -- cgit v1.2.3 From 971fc7b39934e9d3e0927d95c45bea6ad7e90566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= Date: Tue, 13 Jan 2015 05:37:26 +0100 Subject: beam_bool: Correct live calculation for GC BIFs When optimizing boolean expressions, it is not always possible to find a number of live registers for a GC BIF that both preserves all source registers that will be tested and at the same time does not include registers that are not initialized. As currently implemented, we have incomplete information about the register calculated from the free variables. Some registers are marked as "reserved". Reserved registers means that we don't know anything about them; they may or may not be initialized. As a conservative correction (suitable for a maintenance release), we will abort the optimization if we find any reserved registers when calculating the number of live registers. We will not attempt to improve the information about the registers in this commit. By examining the coverage when running the existing compiler test suite we find that the optimization is aborted 15 times (before adding any new test cases). To put that in perspective, the optimization is successfully applied 4927 times, and aborted for other reasons 547 times. Reported-by: Ulf Norell Reported-by: Anthony Ramine --- lib/compiler/src/beam_bool.erl | 23 +++++++++++++++++++---- lib/compiler/test/guard_SUITE.erl | 18 ++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/lib/compiler/src/beam_bool.erl b/lib/compiler/src/beam_bool.erl index dc4d7927e9..a452d30b61 100644 --- a/lib/compiler/src/beam_bool.erl +++ b/lib/compiler/src/beam_bool.erl @@ -163,7 +163,16 @@ bopt_block(Reg, Fail, OldIs, [{block,Bl0}|Acc0], St0) -> %% in guards, so it must have been another %% Core Erlang translator.) throw:protected_violation -> + failed; + + %% Failed to work out the live registers for a GC + %% BIF. For example, if the number of live registers + %% needed to be 4 because {x,3} was a source register, + %% but {x,2} was not known to be initialized, this + %% exception would be thrown. + throw:gc_bif_alloc_failure -> failed + end end. @@ -665,10 +674,16 @@ put_reg_1(V, [], I) -> [{I,V}]. fetch_reg(V, [{I,V}|_]) -> {x,I}; fetch_reg(V, [_|SRs]) -> fetch_reg(V, SRs). -live_regs(Regs) -> - foldl(fun ({I,_}, _) -> - I - end, -1, Regs)+1. +live_regs([{_,reserved}|_]) -> + %% We are not sure that this register is initialized, so we must + %% abort the optimization. + throw(gc_bif_alloc_failure); +live_regs([{I,_}]) -> + I+1; +live_regs([{_,_}|Regs]) -> + live_regs(Regs); +live_regs([]) -> + 0. %%% diff --git a/lib/compiler/test/guard_SUITE.erl b/lib/compiler/test/guard_SUITE.erl index eb205d09a7..34bfdeb1e5 100644 --- a/lib/compiler/test/guard_SUITE.erl +++ b/lib/compiler/test/guard_SUITE.erl @@ -1556,6 +1556,24 @@ bad_constants(Config) when is_list(Config) -> bad_guards(Config) when is_list(Config) -> if erlang:float(self()); true -> ok end, + + fc(catch bad_guards_1(1, [])), + fc(catch bad_guards_1(1, [2])), + fc(catch bad_guards_1(atom, [2])), + + fc(catch bad_guards_2(#{a=>0,b=>0}, [])), + fc(catch bad_guards_2(#{a=>0,b=>0}, [x])), + fc(catch bad_guards_2(not_a_map, [x])), + fc(catch bad_guards_2(42, [x])), + ok. + +%% beam_bool used to produce GC BIF instructions whose +%% Live operands included uninitialized registers. + +bad_guards_1(X, [_]) when {{X}}, -X -> + ok. + +bad_guards_2(M, [_]) when M#{a := 0, b => 0}, map_size(M) -> ok. %% Call this function to turn off constant propagation. -- cgit v1.2.3 From 50a92094372b45c9864afe3418b79605da549122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= Date: Fri, 16 Jan 2015 09:21:50 +0100 Subject: Update primary bootstrap --- bootstrap/bin/start.boot | Bin 5261 -> 5259 bytes bootstrap/bin/start_clean.boot | Bin 5261 -> 5259 bytes bootstrap/lib/compiler/ebin/beam_asm.beam | Bin 11676 -> 11676 bytes bootstrap/lib/compiler/ebin/beam_bool.beam | Bin 16340 -> 16308 bytes bootstrap/lib/compiler/ebin/compiler.app | 2 +- bootstrap/lib/compiler/ebin/sys_core_fold.beam | Bin 50268 -> 50524 bytes bootstrap/lib/kernel/ebin/hipe_unified_loader.beam | Bin 13516 -> 13516 bytes bootstrap/lib/kernel/ebin/inet_dns.beam | Bin 19776 -> 19812 bytes bootstrap/lib/kernel/ebin/kernel.app | 2 +- bootstrap/lib/stdlib/ebin/io_lib_pretty.beam | Bin 15196 -> 15248 bytes bootstrap/lib/stdlib/ebin/stdlib.app | 2 +- 11 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bootstrap/bin/start.boot b/bootstrap/bin/start.boot index f4fb1cd9b5..6cc4fa1f43 100644 Binary files a/bootstrap/bin/start.boot and b/bootstrap/bin/start.boot differ diff --git a/bootstrap/bin/start_clean.boot b/bootstrap/bin/start_clean.boot index f4fb1cd9b5..6cc4fa1f43 100644 Binary files a/bootstrap/bin/start_clean.boot and b/bootstrap/bin/start_clean.boot differ diff --git a/bootstrap/lib/compiler/ebin/beam_asm.beam b/bootstrap/lib/compiler/ebin/beam_asm.beam index 0758874641..e6c5cc9028 100644 Binary files a/bootstrap/lib/compiler/ebin/beam_asm.beam and b/bootstrap/lib/compiler/ebin/beam_asm.beam differ diff --git a/bootstrap/lib/compiler/ebin/beam_bool.beam b/bootstrap/lib/compiler/ebin/beam_bool.beam index f339e76fce..6454719448 100644 Binary files a/bootstrap/lib/compiler/ebin/beam_bool.beam and b/bootstrap/lib/compiler/ebin/beam_bool.beam differ diff --git a/bootstrap/lib/compiler/ebin/compiler.app b/bootstrap/lib/compiler/ebin/compiler.app index 5f15db4437..ea1be86c83 100644 --- a/bootstrap/lib/compiler/ebin/compiler.app +++ b/bootstrap/lib/compiler/ebin/compiler.app @@ -18,7 +18,7 @@ {application, compiler, [{description, "ERTS CXC 138 10"}, - {vsn, "5.0.2"}, + {vsn, "5.0.3"}, {modules, [ beam_a, beam_asm, diff --git a/bootstrap/lib/compiler/ebin/sys_core_fold.beam b/bootstrap/lib/compiler/ebin/sys_core_fold.beam index cce0ac7832..5a091fac34 100644 Binary files a/bootstrap/lib/compiler/ebin/sys_core_fold.beam and b/bootstrap/lib/compiler/ebin/sys_core_fold.beam differ diff --git a/bootstrap/lib/kernel/ebin/hipe_unified_loader.beam b/bootstrap/lib/kernel/ebin/hipe_unified_loader.beam index e6141fd7f1..6df9ac9564 100644 Binary files a/bootstrap/lib/kernel/ebin/hipe_unified_loader.beam and b/bootstrap/lib/kernel/ebin/hipe_unified_loader.beam differ diff --git a/bootstrap/lib/kernel/ebin/inet_dns.beam b/bootstrap/lib/kernel/ebin/inet_dns.beam index dff31e9d76..9fd86024e0 100644 Binary files a/bootstrap/lib/kernel/ebin/inet_dns.beam and b/bootstrap/lib/kernel/ebin/inet_dns.beam differ diff --git a/bootstrap/lib/kernel/ebin/kernel.app b/bootstrap/lib/kernel/ebin/kernel.app index 1b39f7df07..3388f28e91 100644 --- a/bootstrap/lib/kernel/ebin/kernel.app +++ b/bootstrap/lib/kernel/ebin/kernel.app @@ -21,7 +21,7 @@ {application, kernel, [ {description, "ERTS CXC 138 10"}, - {vsn, "3.0.3"}, + {vsn, "3.1"}, {modules, [application, application_controller, application_master, diff --git a/bootstrap/lib/stdlib/ebin/io_lib_pretty.beam b/bootstrap/lib/stdlib/ebin/io_lib_pretty.beam index fa7e6fcbc3..9504baab1b 100644 Binary files a/bootstrap/lib/stdlib/ebin/io_lib_pretty.beam and b/bootstrap/lib/stdlib/ebin/io_lib_pretty.beam differ diff --git a/bootstrap/lib/stdlib/ebin/stdlib.app b/bootstrap/lib/stdlib/ebin/stdlib.app index ba000e691f..e92798b83d 100644 --- a/bootstrap/lib/stdlib/ebin/stdlib.app +++ b/bootstrap/lib/stdlib/ebin/stdlib.app @@ -19,7 +19,7 @@ %% {application, stdlib, [{description, "ERTS CXC 138 10"}, - {vsn, "2.2"}, + {vsn, "2.3"}, {modules, [array, base64, beam_lib, -- cgit v1.2.3