beam_validator: fix incorrect assumptions about GC guard BIFs

The beam_validator pass incorrectly assumes that a GC guard
BIF (such as length/1) may first do a garbage collection
and then fail. That assumption is not correct (guards BIF
only do garbage collection when it is known that the BIF
call will succeed), and will cause the compiler to reject
valid programs.

Modify the beam_validator to assume that if the branch is
taken for a gc_bif instruction, all registers are unchanged
and no garbage collection has occurred. Also add a comment
in the emulator about that assumption.

3 files changed, 40 insertions, 6 deletions

diff --git a/erts/emulator/beam/erl_bif_guard.c b/erts/emulator/beam/erl_bif_guard.c
index 8b47db10dd..9e15f4e399 100644
--- a/erts/emulator/beam/erl_bif_guard.c
+++ b/erts/emulator/beam/erl_bif_guard.c
@@ -318,6 +318,10 @@ double_to_integer(Process* p, double x)
  * The following code is used when a guard that may build on the
  * heap is called directly. They must not use HAlloc(), but must
  * do a garbage collection if there is insufficient heap space.
+ *
+ * Important note: All error checking MUST be done before doing
+ * a garbage collection. The compiler assumes that all registers
+ * are still valid if a guard BIF generates an exception.
  */
 
 #define ERTS_NEED_GC(p, need) ((HEAP_LIMIT((p)) - HEAP_TOP((p))) <= (need))
diff --git a/lib/compiler/src/beam_validator.erl b/lib/compiler/src/beam_validator.erl
index 08ba9c3ee4..5ddc534abc 100644
--- a/lib/compiler/src/beam_validator.erl
+++ b/lib/compiler/src/beam_validator.erl
@@ -604,9 +604,9 @@ valfun_4({gc_bif,Op,{f,Fail},Live,Src,Dst}, #vst{current=St0}=Vst0) ->
     St = kill_heap_allocation(St0),
     Vst1 = Vst0#vst{current=St},
     verify_live(Live, Vst1),
-    Vst2 = prune_x_regs(Live, Vst1),
-    validate_src(Src, Vst2),
-    Vst = branch_state(Fail, Vst2),
+    Vst2 = branch_state(Fail, Vst1),
+    Vst = prune_x_regs(Live, Vst2),
+    validate_src(Src, Vst),
     Type = bif_type(Op, Src, Vst),
     set_type_reg(Type, Dst, Vst);
 valfun_4(return, #vst{current=#st{numy=none}}=Vst) ->
diff --git a/lib/compiler/test/beam_validator_SUITE.erl b/lib/compiler/test/beam_validator_SUITE.erl
index ef8feb8a27..0659d5575e 100644
--- a/lib/compiler/test/beam_validator_SUITE.erl
+++ b/lib/compiler/test/beam_validator_SUITE.erl
@@ -28,7 +28,7 @@
 	 freg_range/1,freg_uninit/1,freg_state/1,
 	 bin_match/1,bin_aligned/1,bad_dsetel/1,
 	 state_after_fault_in_catch/1,no_exception_in_catch/1,
-	 undef_label/1,illegal_instruction/1]).
+	 undef_label/1,illegal_instruction/1,failing_gc_guard_bif/1]).
 	 
 -include("test_server.hrl").
 
@@ -52,13 +52,13 @@ all(suite) ->
      freg_range,freg_uninit,freg_state,
      bin_match,bin_aligned,
      bad_dsetel,state_after_fault_in_catch,no_exception_in_catch,
-     undef_label,illegal_instruction].
+     undef_label,illegal_instruction,failing_gc_guard_bif].
 
 beam_files(Config) when is_list(Config) ->
     ?line {ok,Cwd} = file:get_cwd(),
     ?line Parent = filename:dirname(Cwd),
     ?line Wc = filename:join([Parent,"*","*.beam"]),
-    %% Must have at least two files here, or there will could be
+    %% Must have at least two files here, or there will be
     %% a grammatical error in the output of the io:format/2 call below. ;-)
     ?line [_,_|_] = Fs = filelib:wildcard(Wc),
     ?line io:format("~p files\n", [length(Fs)]),
@@ -356,6 +356,36 @@ illegal_instruction(Config) when is_list(Config) ->
      {{'_',y,0},{[],0,illegal_instruction}}] = Errors,
     ok.
 
+%% The beam_validator used to assume that a GC guard BIF could
+%% do a garbage collection even if it failed. That assumption
+%% is not correct, and will cause the beam_validator to reject
+%% valid programs such as this test case.
+%%
+%% (Thanks to Kiran Khaladkar.)
+%%
+failing_gc_guard_bif(Config) when is_list(Config) ->
+    ?line ok = process_request(lists:seq(1, 36)),
+    ?line error = process_request([]),
+    ?line error = process_request(not_a_list),
+    ok.
+
+process_request(ConfId) ->
+    case process_request_foo(ConfId) of
+	false ->
+	    if
+		length(ConfId) == 36 ->
+		    Response = ok;
+		true ->
+		    Response = error
+	    end
+    end,
+    process_request_bar(self(), [Response]).
+
+process_request_foo(_) ->
+    false.
+
+process_request_bar(Pid, [Response]) when is_pid(Pid) ->
+    Response.
 
 
 %%%-------------------------------------------------------------------------