From 8c0bfd12802af3265d84b8bca247529633d05fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20H=C3=B6gberg?= Date: Thu, 11 Jul 2019 12:21:25 +0200 Subject: beam_validator: Values referenced by other values must be merged This is a more proper fix for ERIERL-348. We used to think that we wouldn't need to update the type of a variable that's no longer referenced by a register ("dead value"), but the attached test case pokes a hole in that assumption. To summarize, the result of '=:='/2 is kept alive longer than one of its arguments, which gets pruned in a state merge leaving us with nothing to work on when we finally compare the result. This is fine for most operations since there's no point in (say) updating the size of a tuple we can no longer reach, but '=:='/2 updates the types of both arguments and we risk missing out on important information when either of them is gone. This commit fixes the problem by merging all values that are *reachable* from a register, rather than just those that *exist* in a register, ensuring that all values stay around at least as long as they're needed. --- lib/compiler/src/beam_validator.erl | 53 ++++++++++++++++-------------- lib/compiler/test/beam_validator_SUITE.erl | 22 +++++++++++++ 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/lib/compiler/src/beam_validator.erl b/lib/compiler/src/beam_validator.erl index ebe9631e09..58250b1314 100644 --- a/lib/compiler/src/beam_validator.erl +++ b/lib/compiler/src/beam_validator.erl @@ -1604,13 +1604,8 @@ infer_types_1(#value{op={bif,'=:='},args=[LHS,RHS]}) -> end; infer_types_1(#value{op={bif,element},args=[{integer,Index}=Key,Tuple]}) -> fun(Val, S) -> - case is_value_alive(Tuple, S) of - true -> - Type = {tuple,[Index], #{ Key => get_term_type(Val, S) }}, - update_type(fun meet/2, Type, Tuple, S); - false -> - S - end + Type = {tuple,[Index], #{ Key => get_term_type(Val, S) }}, + update_type(fun meet/2, Type, Tuple, S) end; infer_types_1(#value{op={bif,is_atom},args=[Src]}) -> infer_type_test_bif({atom,[]}, Src); @@ -1634,10 +1629,7 @@ infer_types_1(#value{op={bif,is_tuple},args=[Src]}) -> infer_type_test_bif({tuple,[0],#{}}, Src); infer_types_1(#value{op={bif,tuple_size}, args=[Tuple]}) -> fun({integer,Arity}, S) -> - case is_value_alive(Tuple, S) of - true -> update_type(fun meet/2, {tuple,Arity,#{}}, Tuple, S); - false -> S - end; + update_type(fun meet/2, {tuple,Arity,#{}}, Tuple, S); (_, S) -> S end; infer_types_1(_) -> @@ -1645,10 +1637,7 @@ infer_types_1(_) -> infer_type_test_bif(Type, Src) -> fun({atom,true}, S) -> - case is_value_alive(Src, S) of - true -> update_type(fun meet/2, Type, Src, S); - false -> S - end; + update_type(fun meet/2, Type, Src, S); (_, S) -> S end. @@ -2285,9 +2274,6 @@ get_raw_type(#value_ref{}=Ref, #vst{current=#st{vs=Vs}}) -> get_raw_type(Src, #vst{}) -> get_literal_type(Src). -is_value_alive(#value_ref{}=Ref, #vst{current=#st{vs=Vs}}) -> - is_map_key(Ref, Vs). - get_literal_type(nil=T) -> T; get_literal_type({atom,A}=T) when is_atom(A) -> T; get_literal_type({float,F}=T) when is_float(F) -> T; @@ -2469,25 +2455,44 @@ merge_vrefs(RefA, RefB, Merge, Counter) -> merge_values(Merge, VsA, VsB) -> maps:fold(fun(Spec, New, Acc) -> - merge_values_1(Spec, New, VsA, VsB, Acc) + mv_1(Spec, New, VsA, VsB, Acc) end, #{}, Merge). -merge_values_1(Same, Same, VsA, VsB, Acc) -> +mv_1(Same, Same, VsA, VsB, Acc0) -> %% We're merging different versions of the same value, so it's safe to %% reuse old entries if the type's unchanged. - #value{type=TypeA}=EntryA = map_get(Same, VsA), - #value{type=TypeB}=EntryB = map_get(Same, VsB), + #value{type=TypeA,args=Args}=EntryA = map_get(Same, VsA), + #value{type=TypeB,args=Args}=EntryB = map_get(Same, VsB), + Entry = case join(TypeA, TypeB) of TypeA -> EntryA; TypeB -> EntryB; JoinedType -> EntryA#value{type=JoinedType} end, - Acc#{ Same => Entry }; -merge_values_1({RefA, RefB}, New, VsA, VsB, Acc) -> + + Acc = Acc0#{ Same => Entry }, + + %% Type inference may depend on values that are no longer reachable from a + %% register, so all arguments must be merged into the new state. + mv_args(Args, VsA, VsB, Acc); +mv_1({RefA, RefB}, New, VsA, VsB, Acc) -> #value{type=TypeA} = map_get(RefA, VsA), #value{type=TypeB} = map_get(RefB, VsB), Acc#{ New => #value{op=join,args=[],type=join(TypeA, TypeB)} }. +mv_args([#value_ref{}=Arg | Args], VsA, VsB, Acc0) -> + case Acc0 of + #{ Arg := _ } -> + mv_args(Args, VsA, VsB, Acc0); + #{} -> + Acc = mv_1(Arg, Arg, VsA, VsB, Acc0), + mv_args(Args, VsA, VsB, Acc) + end; +mv_args([_ | Args], VsA, VsB, Acc) -> + mv_args(Args, VsA, VsB, Acc); +mv_args([], _VsA, _VsB, Acc) -> + Acc. + merge_fragility(FragileA, FragileB) -> cerl_sets:union(FragileA, FragileB). diff --git a/lib/compiler/test/beam_validator_SUITE.erl b/lib/compiler/test/beam_validator_SUITE.erl index 6b1438abdd..20f6cb2691 100644 --- a/lib/compiler/test/beam_validator_SUITE.erl +++ b/lib/compiler/test/beam_validator_SUITE.erl @@ -681,11 +681,16 @@ infer_on_eq_4(T) -> %% ERIERL-348; types were inferred for dead values, causing validation to fail. +-record(idv, {key}). + infer_dead_value(Config) when is_list(Config) -> a = idv_1({a, b, c, d, e, f, g}, {0, 0, 0, 0, 0, 0, 0}), b = idv_1({a, b, c, d, 0, 0, 0}, {a, b, c, d, 0, 0, 0}), c = idv_1({0, 0, 0, 0, 0, f, g}, {0, 0, 0, 0, 0, f, g}), error = idv_1(gurka, gaffel), + + ok = idv_2(id(#idv{})), + ok. idv_1({_A, _B, _C, _D, _E, _F, _G}, @@ -700,6 +705,23 @@ idv_1({_A, _B, _C, _D, _E, F, G}, idv_1(_A, _B) -> error. +%% ERL-995: The first solution to ERIERL-348 was incomplete and caused +%% validation to fail when living values depended on delayed type inference on +%% "dead" values. + +idv_2(State) -> + Flag = (State#idv.key == undefined), + case id(gurka) of + {_} -> id([Flag]); + _ -> ok + end, + if + Flag -> idv_called_once(State); + true -> ok + end. + +idv_called_once(_State) -> ok. + %%%------------------------------------------------------------------------- transform_remove(Remove, Module) -> -- cgit v1.2.3