erts: Fallback to blocking upgrade when tracing is enabled

5 files changed, 238 insertions, 80 deletions

diff --git a/erts/emulator/beam/beam_bif_load.c b/erts/emulator/beam/beam_bif_load.c
index 6694f3ae87..8d41c7499d 100644
--- a/erts/emulator/beam/beam_bif_load.c
+++ b/erts/emulator/beam/beam_bif_load.c
@@ -37,7 +37,6 @@
 
 static void set_default_trace_pattern(Eterm module, ErtsCodeIndex);
 static Eterm check_process_code(Process* rp, Module* modp);
-static void ensure_no_breakpoints(Process *, ErtsProcLocks, Eterm module);
 static void delete_code(Module* modp);
 static int purge_module(Process*, int module);
 static void decrement_refc(BeamInstr* code);
@@ -55,6 +54,7 @@ load_module_2(BIF_ALIST_2)
     Eterm res;
     byte* temp_alloc = NULL;
     struct LoaderState* stp;
+    int is_blocking = 0;
 
     if (is_not_atom(BIF_ARG_1)) {
     error:
@@ -79,18 +79,32 @@ load_module_2(BIF_ALIST_2)
 	BIF_RET(res);
     }
 
-    /*SVERK
-     * Stop all other processes and finish the loading of the module.
-     *
-    erts_smp_proc_unlock(BIF_P, ERTS_PROC_LOCK_MAIN);
-    erts_smp_thr_progress_block();*/
-
-    ensure_no_breakpoints(BIF_P, 0, BIF_ARG_1);
-
     erts_lock_code_ix();
-    erts_start_staging_code_ix();
+    for(;;) {
+	Module* modp;
+	erts_start_staging_code_ix();
+	modp = erts_put_module(BIF_ARG_1);
+	if (!is_blocking) {
+	    if (modp->curr.num_breakpoints > 0
+		|| modp->curr.num_traced_exports > 0
+		|| erts_is_default_trace_enabled()) {
+		/* we have tracing, retry single threaded */
+		erts_abort_staging_code_ix();
+		erts_unlock_code_ix();
+		erts_smp_proc_unlock(BIF_P, ERTS_PROC_LOCK_MAIN);
+		erts_smp_thr_progress_block();
+		is_blocking = 1;
+		continue;
+	    }
+	}
+	else if (modp->curr.num_breakpoints) {
+	    erts_clear_module_break(modp);
+	    ASSERT(modp->curr.num_breakpoints == 0);
+	}
+	reason = erts_finish_loading(stp, BIF_P, 0, &BIF_ARG_1);
+	break;
+    }
 
-    reason = erts_finish_loading(stp, BIF_P, 0, &BIF_ARG_1);
     if (reason != NIL) {
 	if (reason == am_on_load) {
 	    erts_commit_staging_code_ix();
@@ -105,11 +119,13 @@ load_module_2(BIF_ALIST_2)
 	res = TUPLE2(hp, am_module, BIF_ARG_1);
     }
 
-    erts_unlock_code_ix();
-
-    /*SVERK
-    erts_smp_thr_progress_unblock();
-    erts_smp_proc_lock(BIF_P, ERTS_PROC_LOCK_MAIN);*/
+    if (is_blocking) {
+	erts_smp_thr_progress_unblock();
+	erts_smp_proc_lock(BIF_P, ERTS_PROC_LOCK_MAIN);
+    }
+    else {
+	erts_unlock_code_ix();
+    }
     BIF_RET(res);
 }
 
@@ -162,17 +178,20 @@ BIF_RETTYPE code_is_module_native_1(BIF_ALIST_1)
 
 BIF_RETTYPE code_make_stub_module_3(BIF_ALIST_3)
 {
+    Module* modp;
     Eterm res;
 
-    /*SVERK
     erts_smp_proc_unlock(BIF_P, ERTS_PROC_LOCK_MAIN);
     erts_smp_thr_progress_block();
 
-    erts_export_consolidate(erts_active_code_ix());*/
+    modp = erts_get_module(BIF_ARG_1, erts_active_code_ix());
 
-    ensure_no_breakpoints(BIF_P, 0, BIF_ARG_1);
+    if (modp && modp->curr.num_breakpoints > 0) {
+	ASSERT(modp->curr.code != NULL);
+	erts_clear_module_break(modp);
+	ASSERT(modp->curr.num_breakpoints == 0);
+    }
 
-    erts_lock_code_ix();
     erts_start_staging_code_ix();
 
     res = erts_make_stub_module(BIF_P, BIF_ARG_1, BIF_ARG_2, BIF_ARG_3);
@@ -183,11 +202,8 @@ BIF_RETTYPE code_make_stub_module_3(BIF_ALIST_3)
     else {
 	erts_abort_staging_code_ix();
     }
-    erts_unlock_code_ix();
-
-    /*SVERK
     erts_smp_thr_progress_unblock();
-    erts_smp_proc_lock(BIF_P, ERTS_PROC_LOCK_MAIN);*/
+    erts_smp_proc_lock(BIF_P, ERTS_PROC_LOCK_MAIN);
     return res;
 }
 
@@ -281,7 +297,9 @@ check_process_code_2(BIF_ALIST_2)
 BIF_RETTYPE delete_module_1(BIF_ALIST_1)
 {
     ErtsCodeIndex code_ix;
-    int res;
+    Module* modp;
+    int is_blocking = 0;
+    Eterm res = NIL;
 
     if (is_not_atom(BIF_ARG_1))
 	goto badarg;
@@ -290,13 +308,11 @@ BIF_RETTYPE delete_module_1(BIF_ALIST_1)
     erts_smp_proc_unlock(BIF_P, ERTS_PROC_LOCK_MAIN);
     erts_smp_thr_progress_block();*/
 
-    ensure_no_breakpoints(BIF_P, 0, BIF_ARG_1);
-
     erts_lock_code_ix();
-    erts_start_staging_code_ix();
-    code_ix = erts_staging_code_ix();
-    {
-	Module *modp = erts_get_module(BIF_ARG_1, code_ix);
+    do {
+	erts_start_staging_code_ix();
+	code_ix = erts_staging_code_ix();
+	modp = erts_get_module(BIF_ARG_1, code_ix);
 	if (!modp) {
 	    res = am_undefined;
 	}
@@ -308,10 +324,26 @@ BIF_RETTYPE delete_module_1(BIF_ALIST_1)
 	    res = am_badarg;
 	}
 	else {
+	    if (!is_blocking) {
+		if (modp->curr.num_breakpoints > 0 ||
+		    modp->curr.num_traced_exports > 0) {
+		    /* we have tracing, retry single threaded */
+		    erts_abort_staging_code_ix();
+		    erts_unlock_code_ix();
+		    erts_smp_proc_unlock(BIF_P, ERTS_PROC_LOCK_MAIN);
+		    erts_smp_thr_progress_block();
+		    is_blocking = 1;
+		    continue;
+		}
+	    }
+	    else if (modp->curr.num_breakpoints) {
+		erts_clear_module_break(modp);
+		ASSERT(modp->curr.num_breakpoints == 0);
+	    }
 	    delete_code(modp);
 	    res = am_true;
 	}
-    }
+    } while (res == NIL);
 
     if (res == am_true) {
 	erts_commit_staging_code_ix();
@@ -319,7 +351,13 @@ BIF_RETTYPE delete_module_1(BIF_ALIST_1)
     else {
 	erts_abort_staging_code_ix();
     }
-    erts_unlock_code_ix();
+    if (is_blocking) {
+	erts_smp_thr_progress_unblock();
+	erts_smp_proc_lock(BIF_P, ERTS_PROC_LOCK_MAIN);
+    }
+    else {
+	erts_unlock_code_ix();
+    }
 
     /*SVERK
     erts_smp_thr_progress_unblock();
@@ -725,7 +763,7 @@ purge_module(Process* c_p, int module)
     BeamInstr* code;
     BeamInstr* end;
     Module* modp;
-    int is_blocked = 0;
+    int is_blocking = 0;
     int ret;
 
     erts_lock_code_ix();
@@ -753,13 +791,13 @@ retry:
 	     * Unload any NIF library
 	     */
 	    if (modp->old.nif != NULL) {
-		if (!is_blocked) {
+		if (!is_blocking) {
 		    /*SVERK Do unload nif without blocking */
 		    erts_rwunlock_old_code(code_ix);
 		    erts_unlock_code_ix();
 		    erts_smp_proc_unlock(c_p, ERTS_PROC_LOCK_MAIN);
 		    erts_smp_thr_progress_block();
-		    is_blocked = 1;
+		    is_blocking = 1;
 		    goto retry;
 		}
 
@@ -787,7 +825,7 @@ retry:
 	}
 	erts_rwunlock_old_code(code_ix);
     }
-    if (is_blocked) {
+    if (is_blocking) {
 	erts_smp_thr_progress_unblock();
 	erts_smp_proc_lock(c_p, ERTS_PROC_LOCK_MAIN);
     }
@@ -814,37 +852,6 @@ decrement_refc(BeamInstr* code)
     }
 }
 
-
-/*
- * Clear breakpoints in module if any
- */
-static void ensure_no_breakpoints(Process *c_p, ErtsProcLocks c_p_locks,
-				  Eterm module)
-{
-    ErtsCodeIndex code_ix = erts_active_code_ix();
-    Module* modp = erts_get_module(module, code_ix);
-
-    if (modp && modp->curr.num_breakpoints > 0) {
-	ASSERT(modp->curr.code != NULL);
-#ifdef ERTS_ENABLE_LOCK_CHECK
-#ifdef ERTS_SMP
-	if (c_p && c_p_locks)
-	    erts_proc_lc_chk_only_proc_main(c_p);
-	else
-#endif
-	    erts_lc_check_exact(NULL, 0);
-#endif
-	if (c_p && c_p_locks)
-	    erts_smp_proc_unlock(c_p, ERTS_PROC_LOCK_MAIN);
-	erts_smp_thr_progress_block();
-	erts_clear_module_break(modp);
-	modp->curr.num_breakpoints = 0;
-	erts_smp_thr_progress_unblock();
-	if (c_p && c_p_locks)
-	    erts_smp_proc_lock(c_p, ERTS_PROC_LOCK_MAIN);
-    }
-}
-
 /*
  * Move code from current to old and null all export entries for the module
  */
@@ -859,19 +866,24 @@ delete_code(Module* modp)
     for (i = 0; i < export_list_size(code_ix); i++) {
 	Export *ep = export_list(i, code_ix);
         if (ep != NULL && (ep->code[0] == module)) {
-	    if (ep->addressv[code_ix] == ep->code+3 &&
-		(ep->code[3] == (BeamInstr) em_apply_bif)) {
-		continue;
+	    if (ep->addressv[code_ix] == ep->code+3) {
+		if (ep->code[3] == (BeamInstr) em_apply_bif) {
+		    continue;
+		}
+		else if (ep->code[3] == (BeamInstr) em_call_traced_function) {
+		    ERTS_SMP_LC_ASSERT(erts_smp_thr_progress_is_blocking());
+		    ASSERT(modp->curr.num_traced_exports > 0);
+		    --modp->curr.num_traced_exports;
+		    MatchSetUnref(ep->match_prog_set);
+		    ep->match_prog_set = NULL;
+		}
+		else ASSERT(ep->code[3] == (BeamInstr) em_call_error_handler
+			    || !erts_initialized);
 	    }
 	    ep->addressv[code_ix] = ep->code+3;
-	    if (ep->code[3] == (BeamInstr) em_call_traced_function) {
-		ASSERT(modp->curr.num_traced_exports > 0);
-		--modp->curr.num_traced_exports;
-	    }
 	    ep->code[3] = (BeamInstr) em_call_error_handler;
 	    ep->code[4] = 0;
-	    MatchSetUnref(ep->match_prog_set);
-	    ep->match_prog_set = NULL;
+	    ASSERT(ep->match_prog_set == NULL);
 	}
     }
 
diff --git a/erts/emulator/beam/erl_bif_trace.c b/erts/emulator/beam/erl_bif_trace.c
index 24b16b9679..203ba9004b 100644
--- a/erts/emulator/beam/erl_bif_trace.c
+++ b/erts/emulator/beam/erl_bif_trace.c
@@ -373,7 +373,10 @@ erts_get_default_trace_pattern(int *trace_pattern_is_on,
 	*meta_tracer_pid = erts_default_meta_tracer_pid;
 }
 
-
+int erts_is_default_trace_enabled(void)
+{
+    return erts_default_trace_pattern_is_on;
+}
 
 Uint 
 erts_trace_flag2bit(Eterm flag) 
diff --git a/erts/emulator/beam/global.h b/erts/emulator/beam/global.h
index b98f810455..580336470c 100644
--- a/erts/emulator/beam/global.h
+++ b/erts/emulator/beam/global.h
@@ -1773,6 +1773,7 @@ erts_get_default_trace_pattern(int *trace_pattern_is_on,
 			       Binary **meta_match_spec,
 			       struct trace_pattern_flags *trace_pattern_flags,
 			       Eterm *meta_tracer_pid);
+int erts_is_default_trace_enabled(void);
 void erts_bif_trace_init(void);
 
 /*
diff --git a/erts/emulator/test/call_trace_SUITE.erl b/erts/emulator/test/call_trace_SUITE.erl
index 7030ebed3f..67212cd469 100644
--- a/erts/emulator/test/call_trace_SUITE.erl
+++ b/erts/emulator/test/call_trace_SUITE.erl
@@ -25,6 +25,7 @@
 	 init_per_testcase/2,end_per_testcase/2,
 	 hipe/1,process_specs/1,basic/1,flags/1,errors/1,pam/1,change_pam/1,
 	 return_trace/1,exception_trace/1,on_load/1,deep_exception/1,
+	 upgrade/1,
 	 exception_nocatch/1,bit_syntax/1]).
 
 %% Helper functions.
@@ -46,6 +47,7 @@ suite() -> [{ct_hooks,[ts_install_cth]}].
 all() -> 
     Common = [errors, on_load],
     NotHipe = [process_specs, basic, flags, pam, change_pam,
+	       upgrade,
 	       return_trace, exception_trace, deep_exception,
 	       exception_nocatch, bit_syntax],
     Hipe = [hipe],
@@ -267,6 +269,118 @@ foo() -> foo0.
 foo(X) -> X+1.
 foo(X, Y) -> X+Y.
 
+
+%% Note that the semantics that this test case verifies
+%% are not explicitly specified in the docs (what I could find in R15B).
+%% This test case was written to verify that we do not change
+%% any behaviour with the introduction of "block-free" upgrade in R16.
+%% In short: Do not refer to this test case as an authority of how it must work.
+upgrade(doc) ->
+    "Test tracing on module being upgraded";
+upgrade(Config) when is_list(Config) ->
+    V1 = compile_version(my_upgrade_test, 1, Config),
+    V2 = compile_version(my_upgrade_test, 2, Config),
+    start_tracer(),
+    upgrade_do(V1, V2, false),
+    upgrade_do(V1, V2, true).
+
+upgrade_do(V1, V2, TraceLocalVersion) ->
+    {module,my_upgrade_test} = erlang:load_module(my_upgrade_test, V1),
+
+
+    %% Test that trace is cleared after load_module
+
+    trace_func({my_upgrade_test,'_','_'}, [], [global]),
+    case TraceLocalVersion of
+	true -> trace_func({my_upgrade_test,local_version,0}, [], [local]);
+	_ -> ok
+    end,
+    1 = my_upgrade_test:version(),
+    1 = my_upgrade_test:do_local(),
+    1 = my_upgrade_test:do_real_local(),
+    put('F1_exp', my_upgrade_test:make_fun_exp()),
+    put('F1_loc', my_upgrade_test:make_fun_local()),
+    1 = (get('F1_exp'))(),
+    1 = (get('F1_loc'))(),
+
+    Self = self(),
+    expect({trace,Self,call,{my_upgrade_test,version,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,do_local,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,do_real_local,[]}}),
+    case TraceLocalVersion of
+	true -> expect({trace,Self,call,{my_upgrade_test,local_version,[]}});
+	_ -> ok
+    end,
+    expect({trace,Self,call,{my_upgrade_test,make_fun_exp,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,make_fun_local,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,version,[]}}), % F1_exp
+    case TraceLocalVersion of
+	true -> expect({trace,Self,call,{my_upgrade_test,local_version,[]}}); % F1_loc
+	_ -> ok
+    end,
+
+    {module,my_upgrade_test} = erlang:load_module(my_upgrade_test, V2),
+    2 = my_upgrade_test:version(),
+    put('F2_exp', my_upgrade_test:make_fun_exp()),
+    put('F2_loc', my_upgrade_test:make_fun_local()),
+    2 = (get('F1_exp'))(),
+    1 = (get('F1_loc'))(),
+    2 = (get('F2_exp'))(),
+    2 = (get('F2_loc'))(),
+    expect(),
+
+    put('F1_exp', undefined),
+    put('F1_loc', undefined),
+    erlang:garbage_collect(),
+    erlang:purge_module(my_upgrade_test),
+
+    % Test that trace is cleared after delete_module
+
+    trace_func({my_upgrade_test,'_','_'}, [], [global]),
+    case TraceLocalVersion of
+	true -> trace_func({my_upgrade_test,local_version,0}, [], [local]);
+	_ -> ok
+    end,
+    2 = my_upgrade_test:version(),
+    2 = my_upgrade_test:do_local(),
+    2 = my_upgrade_test:do_real_local(),
+    2 = (get('F2_exp'))(),
+    2 = (get('F2_loc'))(),
+
+    expect({trace,Self,call,{my_upgrade_test,version,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,do_local,[]}}),
+    expect({trace,Self,call,{my_upgrade_test,do_real_local,[]}}),
+    case TraceLocalVersion of
+	true -> expect({trace,Self,call,{my_upgrade_test,local_version,[]}});
+	_ -> ok
+    end,
+    expect({trace,Self,call,{my_upgrade_test,version,[]}}), % F2_exp
+    case TraceLocalVersion of
+	true -> expect({trace,Self,call,{my_upgrade_test,local_version,[]}}); % F2_loc
+	_ -> ok
+    end,
+
+    true = erlang:delete_module(my_upgrade_test),
+    {'EXIT',{undef,_}} = (catch my_upgrade_test:version()),
+    {'EXIT',{undef,_}} = (catch ((get('F2_exp'))())),
+    2 = (get('F2_loc'))(),
+    expect(),
+
+    put('F2_exp', undefined),
+    put('F2_loc', undefined),
+    erlang:garbage_collect(),
+    erlang:purge_module(my_upgrade_test),
+    ok.
+
+compile_version(Module, Version, Config) ->
+    Data = ?config(data_dir, Config),
+    File = filename:join(Data, atom_to_list(Module)),
+    {ok,Module,Bin} = compile:file(File, [{d,'VERSION',Version},
+					    binary,report]),
+    Bin.
+
+
+
 %% Test flags (arity, timestamp) for call_trace/3.
 %% Also, test the '{tracer,Pid}' option.
 flags(_Config) ->
@@ -1151,11 +1265,13 @@ trace_info(What, Key) ->
     Res.
     
 trace_func(MFA, MatchSpec) ->
-    get(tracer) ! {apply,self(),{erlang,trace_pattern,[MFA, MatchSpec]}},
+    trace_func(MFA, MatchSpec, []).
+trace_func(MFA, MatchSpec, Flags) ->
+    get(tracer) ! {apply,self(),{erlang,trace_pattern,[MFA, MatchSpec, Flags]}},
     Res = receive
 	      {apply_result,Result} -> Result
 	  end,
-    ok = io:format("trace_pattern(~p, ~p) -> ~p", [MFA,MatchSpec,Res]),
+    ok = io:format("trace_pattern(~p, ~p, ~p) -> ~p", [MFA,MatchSpec,Flags,Res]),
     Res.
 
 trace_pid(Pid, On, Flags) ->
diff --git a/erts/emulator/test/call_trace_SUITE_data/my_upgrade_test.erl b/erts/emulator/test/call_trace_SUITE_data/my_upgrade_test.erl
new file mode 100644
index 0000000000..11b8a95209
--- /dev/null
+++ b/erts/emulator/test/call_trace_SUITE_data/my_upgrade_test.erl
@@ -0,0 +1,26 @@
+-module(my_upgrade_test).
+
+-export([version/0]).
+-export([do_local/0]).
+-export([do_real_local/0]).
+-export([make_fun_exp/0]).
+-export([make_fun_local/0]).
+
+
+version() ->
+    ?VERSION.
+
+do_local() ->
+    version().
+
+do_real_local() ->
+    local_version().
+
+local_version() ->
+    ?VERSION.
+
+make_fun_exp() ->
+    fun() -> ?MODULE:version() end.
+
+make_fun_local() ->
+    fun() -> local_version() end.