compiler: Eliminate internal consistency failure in binary matching

The following code:

  check(<<"string">>, a1) ->
    one;
  check(_, a2) ->
    two;
  check(undefined, a3) ->
    three.

produces an internal consistency failure:

  check: function check/2+17:
    Internal consistency check failed - please report this bug.
    Instruction: {test,is_eq_exact,{f,7},[{x,0},{atom,undefined}]}
    Error:       {match_context,{x,0}}:

Actually, in the current implementation of the run-time system,
comparing a match context to an atom is safe, so I briefly considered
updating the beam_validator to let this code pass through. I
abandoned that approach because not all terms would be safe to
compare to a match context, and the implementation might change
in the future.

Therefore, fix this problem by not allowing any matching of non-variables
(in the argument position for binary being matched) following binary
matching. That solution is simple and safe, and since this kind of
code seems to be rare in practice, there is no need to pursue any
more compilicated solution.

Reported-by: Viktor Sovietov

2 files changed, 73 insertions, 12 deletions

diff --git a/lib/compiler/src/sys_core_fold.erl b/lib/compiler/src/sys_core_fold.erl
index f17b0bd130..fbd7452301 100644
--- a/lib/compiler/src/sys_core_fold.erl
+++ b/lib/compiler/src/sys_core_fold.erl
@@ -2672,16 +2672,19 @@ bsm_nonempty([#c_clause{pats=Ps}|Cs], Pos) ->
 bsm_nonempty([], _ ) -> false.
 
 %% bsm_ensure_no_partition(Cs, Pos) -> ok     (exception if problem)
-%%  We must make sure that binary matching is not partitioned between
+%%  We must make sure that matching is not partitioned between
 %%  variables like this:
 %%             foo(<<...>>) -> ...
-%%             foo(Var) when ... -> ...
-%%             foo(<<...>>) ->
+%%             foo(<Variable>) when ... -> ...
+%%             foo(<Any non-variable pattern>) ->
 %%  If there is such partition, we are not allowed to reuse the binary variable
-%%  for the match context. Also, arguments to the left of the argument that
-%%  is matched against a binary, are only allowed to be simple variables, not
-%%  used in guards. The reason is that we must know that the binary is only
-%%  matched in one place.
+%%  for the match context.
+%%
+%%  Also, arguments to the left of the argument that is matched
+%%  against a binary, are only allowed to be simple variables, not
+%%  used in guards. The reason is that we must know that the binary is
+%%  only matched in one place (i.e. there must be only one bs_start_match2
+%%  instruction emitted).
 
 bsm_ensure_no_partition(Cs, Pos) ->
     bsm_ensure_no_partition_1(Cs, Pos, before).
@@ -2689,6 +2692,12 @@ bsm_ensure_no_partition(Cs, Pos) ->
 %% Loop through each clause.
 bsm_ensure_no_partition_1([#c_clause{pats=Ps,guard=G}|Cs], Pos, State0) ->
     State = bsm_ensure_no_partition_2(Ps, Pos, G, simple_vars, State0),
+    case State of
+	'after' ->
+	    bsm_ensure_no_partition_after(Cs, Pos);
+	_ ->
+	    ok
+    end,
     bsm_ensure_no_partition_1(Cs, Pos, State);
 bsm_ensure_no_partition_1([], _, _) -> ok.
 
@@ -2698,8 +2707,7 @@ bsm_ensure_no_partition_2([#c_binary{}=Where|_], 1, _, Vstate, State) ->
 	before when Vstate =:= simple_vars -> within;
 	before -> bsm_problem(Where, Vstate);
 	within when Vstate =:= simple_vars -> within;
-	within -> bsm_problem(Where, Vstate);
-	'after' -> bsm_problem(Where, bin_partition)
+	within -> bsm_problem(Where, Vstate)
     end;
 bsm_ensure_no_partition_2([#c_alias{}=Alias|_], 1, N, Vstate, State) ->
     %% Retrieve the real pattern that the alias refers to and check that.
@@ -2748,6 +2756,15 @@ bsm_ensure_no_partition_2([#c_var{name=V}|Ps], N, G, Vstate, S) ->
 bsm_ensure_no_partition_2([_|Ps], N, G, _, S) ->
     bsm_ensure_no_partition_2(Ps, N-1, G, bin_argument_order, S).
 
+bsm_ensure_no_partition_after([#c_clause{pats=Ps}|Cs], Pos) ->
+    case nth(Pos, Ps) of
+	#c_var{} ->
+	    bsm_ensure_no_partition_after(Cs, Pos);
+	P ->
+	    bsm_problem(P, bin_partition)
+    end;
+bsm_ensure_no_partition_after([], _) -> ok.
+    
 bsm_could_match_binary(#c_alias{pat=P}) -> bsm_could_match_binary(P);
 bsm_could_match_binary(#c_cons{}) -> false;
 bsm_could_match_binary(#c_tuple{}) -> false;
@@ -2872,7 +2889,7 @@ format_error(useless_building) ->
 format_error(bin_opt_alias) ->
     "INFO: the '=' operator will prevent delayed sub binary optimization";
 format_error(bin_partition) ->
-    "INFO: non-consecutive clauses that match binaries "
+    "INFO: matching non-variables after a previous clause matching a variable "
 	"will prevent delayed sub binary optimization";
 format_error(bin_left_var_used_in_guard) ->
     "INFO: a variable to the left of the binary pattern is used in a guard; "
diff --git a/lib/compiler/test/bs_match_SUITE.erl b/lib/compiler/test/bs_match_SUITE.erl
index d63d2235d7..e8a92c509e 100644
--- a/lib/compiler/test/bs_match_SUITE.erl
+++ b/lib/compiler/test/bs_match_SUITE.erl
@@ -33,7 +33,8 @@
 	 matching_meets_construction/1,simon/1,matching_and_andalso/1,
 	 otp_7188/1,otp_7233/1,otp_7240/1,otp_7498/1,
 	 match_string/1,zero_width/1,bad_size/1,haystack/1,
-	 cover_beam_bool/1,matched_out_size/1,follow_fail_branch/1]).
+	 cover_beam_bool/1,matched_out_size/1,follow_fail_branch/1,
+	 no_partition/1]).
 
 -export([coverage_id/1,coverage_external_ignore/2]).
 
@@ -57,7 +58,8 @@ groups() ->
        matching_meets_construction,simon,
        matching_and_andalso,otp_7188,otp_7233,otp_7240,
        otp_7498,match_string,zero_width,bad_size,haystack,
-       cover_beam_bool,matched_out_size,follow_fail_branch]}].
+       cover_beam_bool,matched_out_size,follow_fail_branch,
+       no_partition]}].
 
 
 init_per_suite(Config) ->
@@ -1133,6 +1135,48 @@ ffb_2(<<_,T/bitstring>>, List, A) ->
 	[_|_] -> bit_size(T)
     end.
 
+no_partition(_) ->
+    one = no_partition_1(<<"string">>, a1),
+    {two,<<"string">>} = no_partition_1(<<"string">>, a2),
+    {two,<<>>} = no_partition_1(<<>>, a2),
+    {two,a} = no_partition_1(a, a2),
+    three = no_partition_1(undefined, a3),
+    {four,a,[]} = no_partition_1([a], a4),
+    {five,a,b} = no_partition_1({a,b}, a5),
+
+    one = no_partition_2(<<"string">>, a1),
+    two = no_partition_2(<<"string">>, a2),
+    two = no_partition_2(<<>>, a2),
+    two = no_partition_2(a, a2),
+    three = no_partition_2(undefined, a3),
+    four = no_partition_2(42, a4),
+    five = no_partition_2([], a5),
+    six = no_partition_2(42.0, a6),
+    ok.
+
+no_partition_1(<<"string">>, a1) ->
+    one;
+no_partition_1(V, a2) ->
+    {two,V};
+no_partition_1(undefined, a3) ->
+    three;
+no_partition_1([H|T], a4) ->
+    {four,H,T};
+no_partition_1({A,B}, a5) ->
+    {five,A,B}.
+
+no_partition_2(<<"string">>, a1) ->
+    one;
+no_partition_2(_, a2) ->
+    two;
+no_partition_2(undefined, a3) ->
+    three;
+no_partition_2(42, a4) ->
+    four;
+no_partition_2([], a5) ->
+    five;
+no_partition_2(42.0, a6) ->
+    six.
 
 check(F, R) ->
     R = F().