From bad27f1cdb7cf3534490dcec17b4e4a49754e4ce Mon Sep 17 00:00:00 2001 From: Frej Drejhammar Date: Mon, 13 Feb 2023 15:33:29 +0100 Subject: [PATCH] compiler: Don't generate unsafe code for binary append The private-append optimization worked on the assumption that the `private_append` version of `bs_create_bin` checked that the type of the first argument was a bitstring. As it doesn't, the compiler shouldn't do the optimization when it cannot guarantee that the type is and only is a bitstring. Fixes #6851 Fixes #6848 --- lib/compiler/src/beam_ssa_private_append.erl | 3 - .../private_append.erl | 87 ++++++++++++------- 2 files changed, 58 insertions(+), 32 deletions(-) diff --git a/lib/compiler/src/beam_ssa_private_append.erl b/lib/compiler/src/beam_ssa_private_append.erl index 0ace5cca622a..e47c6857fd96 100644 --- a/lib/compiler/src/beam_ssa_private_append.erl +++ b/lib/compiler/src/beam_ssa_private_append.erl @@ -109,9 +109,6 @@ is_appendable(Anno, #b_literal{val=[SegmentUnit|_]}) case Anno of #{arg_types:=#{2:=#t_bitstring{appendable=true,size_unit=SizeUnit}}} -> SizeUnit rem SegmentUnit == 0; - #{arg_types:=#{2:=#t_union{other=#t_bitstring{appendable=true, - size_unit=SizeUnit}}}} -> - SizeUnit rem SegmentUnit == 0; _ -> false end. diff --git a/lib/compiler/test/beam_ssa_check_SUITE_data/private_append.erl b/lib/compiler/test/beam_ssa_check_SUITE_data/private_append.erl index d0ac4fb5c848..bf0381e72467 100644 --- a/lib/compiler/test/beam_ssa_check_SUITE_data/private_append.erl +++ b/lib/compiler/test/beam_ssa_check_SUITE_data/private_append.erl @@ -41,7 +41,6 @@ transformable14/1, transformable15/1, transformable16/1, - transformable17/1, transformable18/2, transformable19/1, transformable20/1, @@ -67,7 +66,10 @@ not_transformable7/1, not_transformable8/1, not_transformable9/1, - not_transformable10/1]). + not_transformable10/1, + not_transformable11/0, + not_transformable12/1, + not_transformable13/1]). %% Trivial smoke test transformable0(L) -> @@ -427,25 +429,6 @@ transformable16([A,B|T], {{Acc0}, Acc1}) -> transformable16([], {{Acc0}, Acc1}) -> {Acc0,Acc1}. -%% The order of fields in the accumulator is swapped compared to -%% transformable7. -transformable17(L) -> -%ssa% (_) when post_ssa_opt -> -%ssa% A = bs_init_writable(_), -%ssa% B = put_list(_, A), -%ssa% _ = call(fun transformable17/2, _, B). - transformable17(L, [0|<<>>]). - -transformable17([H|T], [N|Acc]) -> -%ssa% (_, Arg1) when post_ssa_opt -> -%ssa% A = get_tl(Arg1), -%ssa% B = bs_create_bin(private_append, _, A, ...), -%ssa% C = put_list(_, B), -%ssa% _ = call(fun transformable17/2, _, C). - transformable17(T, [N+1|<>]); -transformable17([], Acc) -> - Acc. - %% We should use type information to figure out that {<<>>, X} is not %% aliased, but as of now we don't have the information at this pass, %% nor do we track alias status at the sub-term level. @@ -544,13 +527,15 @@ make_empty_binary_tuple() -> %ssa% ret(C). {<<>>, <<>>}. -%% Check that the conversion works for both elements of the list. -%% +%% Check that the conversion works for the first element of the list. +%% Type analysis does not understand that the second field of the cons +%% isn't an improper list. As the private append variant of +%% bs_create_bin can't deal with non-bitstrings, check that we +%% conservatively refuse to rewrite the second element of the cons. transformable22(L) -> %ssa% (_) when post_ssa_opt -> %ssa% A = bs_init_writable(_), -%ssa% B = bs_init_writable(_), -%ssa% C = put_list(A, B), +%ssa% C = put_list(A, _), %ssa% _ = call(fun transformable22/2, _, C). transformable22(L, [<<>>|<<>>]). @@ -559,7 +544,7 @@ transformable22([H|T], [AccA|AccB]) -> %ssa% A = get_hd(Arg1), %ssa% B = get_tl(Arg1), %ssa% C = bs_create_bin(private_append, _, A, ...), -%ssa% D = bs_create_bin(private_append, _, B, ...), +%ssa% D = bs_create_bin(append, _, B, ...), %ssa% E = put_list(C, D), %ssa% _ = call(fun transformable22/2, _, E). transformable22(T, [<>|<>]); @@ -710,8 +695,7 @@ transformable29([], Acc) -> transformable30(L) -> %ssa% (_) when post_ssa_opt -> %ssa% A = bs_init_writable(_), -%ssa% B = bs_init_writable(_), -%ssa% C = put_list(A, B), +%ssa% C = put_list(A, _), %ssa% _ = call(fun transformable30/2, _, C). transformable30(L, [<<>>|<<>>]). @@ -720,7 +704,7 @@ transformable30([H|T], X) -> %ssa% B = get_tl(Arg1), %ssa% A = get_hd(Arg1), %ssa% C = bs_create_bin(private_append, _, A, ...), -%ssa% D = bs_create_bin(private_append, _, B, ...), +%ssa% D = bs_create_bin(append, _, B, ...), %ssa% E = put_list(C, D), %ssa% _ = call(fun transformable30/2, _, E). AccB = tl(X), @@ -889,3 +873,48 @@ not_transformable10([H|T], Acc) -> not_transformable10(T, <>); not_transformable10([], Acc) -> Acc. + +%% Check that we don't transform when we can't guarantee that the +%% first fragment to bs_create_bin is a bitstring. +not_transformable11() -> +%ssa% fail () when post_ssa_opt -> +%ssa% _ = bs_init_writable(_). + not_transformable11(not_ok). + +not_transformable11(X) -> +%ssa% (_) when post_ssa_opt -> +%ssa% _ = bs_create_bin(append, ...). + <<(case ok of + X -> <<>>; + _ -> ok + end)/bytes>>. + +%% Check that we don't transform when we can't guarantee that the +%% first fragment to bs_create_bin is a bitstring. +not_transformable12(A) -> +%ssa% (_) when post_ssa_opt -> +%ssa% _ = bs_create_bin(append, ...). + <<(case ok of + A -> + not is_alive(); + _ -> + <<>> + end)/bitstring>>. + +%% The order of fields in the accumulator is swapped compared to +%% transformable7. Type analysis does not understand that the second +%% field of the cons isn't an improper list. As the private append +%% variant of bs_create_bin can't deal with non-bitstrings, check that +%% we conservatively refuse to rewrite this case. +not_transformable13(L) -> + not_transformable13(L, [0|<<>>]). + +not_transformable13([H|T], [N|Acc]) -> +%ssa% (_, Arg1) when post_ssa_opt -> +%ssa% A = get_tl(Arg1), +%ssa% B = bs_create_bin(append, _, A, ...), +%ssa% C = put_list(_, B), +%ssa% _ = call(fun not_transformable13/2, _, C). + not_transformable13(T, [N+1|<>]); +not_transformable13([], Acc) -> + Acc.