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(-) (limited to 'lib/compiler') 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