Merge branch 'sverker/erts/trace-call-mem-validate-stack/OTP-19761' into maint-28

* sverker/erts/trace-call-mem-validate-stack/OTP-19761:
  erts: Add asserts to verify trace stack frames
  erts: Fix crash in erts_validate_stack()

Author: Erlang/OTP

erts/emulator/beam/beam_bp.c

@@ -1101,6 +1101,27 @@ do_session_breakpoint(Process *c_p, ErtsCodeInfo *info, Eterm *reg,
     return bp_flags;
 }
 
+#ifdef DEBUG
+void assert_return_trace_frame(const Eterm *frame)
+{
+    ASSERT_MFA((ErtsCodeMFA*)cp_val(frame[0]));
+    ASSERT(IS_TRACER_VALID(frame[1]));
+    ASSERT(erts_is_trace_session_weak_id(frame[2]));
+}
+
+void assert_return_to_trace_frame(const Eterm *frame)
+{
+    ASSERT(erts_is_trace_session_weak_id(frame[0]));
+}
+
+void assert_return_call_acc_trace_frame(const Eterm *frame)
+{
+    ASSERT(is_CP(frame[0]) || is_nil(frame[0])); // prev_info
+    ASSERT((unsigned_val(frame[1]) & ~ERTS_BPF_ALL) == 0); // bp_flags
+    ASSERT(erts_is_trace_session_weak_id(frame[2]));
+}
+#endif
+
 static ErtsTracer
 do_call_trace(Process* c_p, ErtsCodeInfo* info, Eterm* reg,
 	      int local, Binary* ms,

erts/emulator/beam/beam_bp.h

@@ -208,6 +208,12 @@ Uint erts_line_breakpoint_hit__cleanup(Eterm *regs, UWord *stk);
 
 const ErtsCodeInfo *erts_find_local_func(const ErtsCodeMFA *mfa);
 
+#ifdef DEBUG
+void assert_return_trace_frame(const Eterm *frame);
+void assert_return_to_trace_frame(const Eterm *frame);
+void assert_return_call_acc_trace_frame(const Eterm *frame);
+#endif
+
 #if ERTS_GLB_INLINE_INCL_FUNC_DEF
 
 ERTS_GLB_INLINE ErtsBpIndex erts_active_bp_ix(void)

erts/emulator/beam/erl_gc.c

@@ -3839,7 +3839,7 @@ erts_max_heap_size(Eterm arg, Uint *max_heap_size, Uint *max_heap_flags)
     return 1;
 }
 
-#ifdef DEBUG
+#if defined(DEBUG) && defined(ERLANG_FRAME_POINTERS)
 void erts_validate_stack(Process *p, Eterm *frame_ptr, Eterm *stack_top) {
     Eterm *stack_bottom = HEAP_END(p);
     Eterm *next_fp = frame_ptr;
@@ -3853,20 +3853,24 @@ void erts_validate_stack(Process *p, Eterm *frame_ptr, Eterm *stack_top) {
     ASSERT((next_fp != NULL) ^ (stack_top == stack_bottom));
 
     /* If the GC happens when we are about to execute a trace we
-       need to skip the trace instructions */
+       need to skip the trace instructions.
+       Note: It's not safe in general to assume p->i is up-to-date in GC.
+       However the return trace intructions do update p->i after returning.
+       */
     if (BeamIsReturnTrace(p->i)) {
-        /* Skip MFA and tracer. */
-        ASSERT_MFA((ErtsCodeMFA*)cp_val(scanner[0]));
-        ASSERT(IS_TRACER_VALID(scanner[1]));
+        assert_return_trace_frame(scanner);
         scanner += BEAM_RETURN_TRACE_FRAME_SZ;
     } else if (BeamIsReturnCallAccTrace(p->i)) {
-        /* Skip prev_info. */
+        assert_return_call_acc_trace_frame(scanner);
         scanner += BEAM_RETURN_CALL_ACC_TRACE_FRAME_SZ;
     } else if (BeamIsReturnToTrace(p->i)) {
+        assert_return_to_trace_frame(scanner);
         scanner += BEAM_RETURN_TO_TRACE_FRAME_SZ;
     }
 
     while (next_fp) {
+        ErtsCodePtr cp;
+
         ASSERT(next_fp >= stack_top && next_fp <= stack_bottom);
 
         /* We may not skip any frames. */
@@ -3876,24 +3880,24 @@ void erts_validate_stack(Process *p, Eterm *frame_ptr, Eterm *stack_top) {
         }
 
         /* {Next frame, Return address} or vice versa */
-        ASSERT(is_CP(scanner[0]) && is_CP(scanner[1]));
         next_fp = (Eterm*)cp_val(scanner[0]);
+        cp = cp_val(scanner[1]);
+
+        scanner += CP_SIZE;
 
         /* Call tracing may store raw pointers on the stack. This is explicitly
          * handled in all routines that deal with the stack. */
-        if (BeamIsReturnTrace((ErtsCodePtr)scanner[1])) {
-            /* Skip MFA and tracer. */
-            ASSERT_MFA((ErtsCodeMFA*)cp_val(scanner[2]));
-            ASSERT(IS_TRACER_VALID(scanner[3]));
+        if (BeamIsReturnTrace(cp)) {
+            assert_return_trace_frame(scanner);
             scanner += BEAM_RETURN_TRACE_FRAME_SZ;
-        } else if (BeamIsReturnCallAccTrace((ErtsCodePtr)scanner[1])) {
-            /* Skip prev_info. */
+        } else if (BeamIsReturnCallAccTrace(cp)) {
+            assert_return_call_acc_trace_frame(scanner);
             scanner += BEAM_RETURN_CALL_ACC_TRACE_FRAME_SZ;
-        } else if (BeamIsReturnToTrace((ErtsCodePtr)scanner[1])) {
+        } else if (BeamIsReturnToTrace(cp)) {
+            assert_return_to_trace_frame(scanner);
             scanner += BEAM_RETURN_TO_TRACE_FRAME_SZ;
         }
 
-        scanner += CP_SIZE;
     }
 }
 #endif

erts/emulator/beam/erl_gc.h

@@ -195,9 +195,13 @@ int erts_dbg_within_proc(Eterm *ptr, Process *p, Eterm* real_htop);
 #endif
 
 #ifdef DEBUG
+
+# ifdef ERLANG_FRAME_POINTERS
 /* Validates the frame chain, ensuring that it always points within the stack
  * and that no frames are skipped. */
 void erts_validate_stack(Process *p, Eterm *frame_ptr, Eterm *stack_top);
+# endif
+
 int
 erts_dbg_check_heap_terms(int (*check_eterm)(Eterm),
                           Process *p,

erts/emulator/beam/erl_trace.c

@@ -3822,6 +3822,11 @@ void erts_change_proc_trace_session_flags(Process* p, ErtsTraceSession* session,
 }
 
 #ifdef DEBUG
+bool erts_is_trace_session_weak_id(Eterm term)
+{
+    return is_small(term) || term == am_default;
+}
+
 void erts_assert_tracer_refs(ErtsPTabElementCommon* t_p)
 {
     ErtsTracerRef *ref, *other;

erts/emulator/beam/erl_trace.h

@@ -152,6 +152,7 @@ Uint32 erts_sum_all_trace_flags(ErtsPTabElementCommon* t_p);
 void erts_change_proc_trace_session_flags(Process*, ErtsTraceSession*,
                                           Uint32 clear_flags, Uint32 set_flags);
 #ifdef DEBUG
+bool erts_is_trace_session_weak_id(Eterm);
 void erts_assert_tracer_refs(ErtsPTabElementCommon* t_p);
 # define ERTS_ASSERT_TRACER_REFS(TP) erts_assert_tracer_refs(TP)
 #else

erts/emulator/beam/jit/arm/beam_asm.hpp

@@ -1374,7 +1374,8 @@ class BeamModuleAssembler : public BeamAssembler,
     void emit_tuple_assertion(const ArgSource &Src, a64::Gp tuple_reg);
 #endif
 
-    void emit_dispatch_return();
+    void emit_return_do(bool set_I);
+    void emit_dispatch_return(bool set_I);
 
 #include "beamasm_protos.h"
 

erts/emulator/beam/jit/arm/instr_call.cpp

@@ -35,13 +35,13 @@ void BeamGlobalAssembler::emit_dispatch_return() {
     a.b(labels[context_switch_simplified]);
 }
 
-void BeamModuleAssembler::emit_dispatch_return() {
+void BeamModuleAssembler::emit_dispatch_return(bool set_I) {
 #ifdef JIT_HARD_DEBUG
     /* Validate return address and {x,0} */
     emit_validate(ArgVal(ArgVal::Type::Word, 1));
 #endif
 
-    if (erts_alcu_enable_code_atags) {
+    if (erts_alcu_enable_code_atags || set_I) {
         /* See emit_i_test_yield. */
         a.str(a64::x30, arm::Mem(c_p, offsetof(Process, i)));
     }
@@ -57,13 +57,17 @@ void BeamModuleAssembler::emit_dispatch_return() {
 }
 
 void BeamModuleAssembler::emit_return() {
+    emit_return_do(false);
+}
+
+void BeamModuleAssembler::emit_return_do(bool set_I) {
     emit_leave_erlang_frame();
-    emit_dispatch_return();
+    emit_dispatch_return(set_I);
 }
 
 void BeamModuleAssembler::emit_move_deallocate_return() {
     a.ldp(XREG0, a64::x30, arm::Mem(E).post(16));
-    emit_dispatch_return();
+    emit_dispatch_return(false);
 }
 
 void BeamModuleAssembler::emit_i_call(const ArgLabel &CallTarget) {

erts/emulator/beam/jit/arm/instr_trace.cpp

@@ -164,7 +164,10 @@ void BeamModuleAssembler::emit_return_trace() {
     emit_leave_runtime<Update::eHeapAlloc>(1);
 
     emit_deallocate(ArgVal(ArgVal::Type::Word, BEAM_RETURN_TRACE_FRAME_SZ));
-    emit_return();
+
+    /* Return and set c_p->i to avoid calls to BeamIsReturnTrace(c_p->i)
+       to assume there is still a trace frame on the stack. */
+    emit_return_do(true);
 }
 
 void BeamModuleAssembler::emit_i_call_trace_return() {
@@ -189,7 +192,10 @@ void BeamModuleAssembler::emit_i_call_trace_return() {
 
     emit_deallocate(
             ArgVal(ArgVal::Type::Word, BEAM_RETURN_CALL_ACC_TRACE_FRAME_SZ));
-    emit_return();
+
+    /* Return and set c_p->i to avoid calls to BeamIsReturnCallAccTrace(c_p->i)
+       to assume there is still a trace frame on the stack. */
+    emit_return_do(true);
 }
 
 void BeamModuleAssembler::emit_i_return_to_trace() {
@@ -207,7 +213,10 @@ void BeamModuleAssembler::emit_i_return_to_trace() {
     emit_leave_runtime<Update::eHeapAlloc>(1);
 
     emit_deallocate(ArgVal(ArgVal::Type::Word, BEAM_RETURN_TO_TRACE_FRAME_SZ));
-    emit_return();
+
+    /* Return and set c_p->i to avoid calls to BeamIsReturnToTrace(c_p->i)
+       to assume there is still a trace frame on the stack. */
+    emit_return_do(true);
 }
 
 void BeamModuleAssembler::emit_i_hibernate() {

erts/emulator/beam/jit/x86/beam_asm.hpp

@@ -1520,6 +1520,8 @@ class BeamModuleAssembler : public BeamAssembler,
     void emit_tuple_assertion(const ArgSource &Src, x86::Gp tuple_reg);
 #endif
 
+    void emit_return_do(bool set_I);
+
 #include "beamasm_protos.h"
 
     const Label &resolve_beam_label(const ArgLabel &Lbl) const {