erts: Fix undefined behavior in $DISPATCHX()

Some call instructions kept the export entry outside of Arg(0) for
better argument packing, so the $DISPATCHX() macro faked a new
instruction pointer starting at "one behind" the given argument.

Some of these saved the argument on the C stack and passed that
onwards to this macro, which could provoke undefined behavior if
we were to jump out of the block, for example if we needed to
save_calls.

This commit fixes the issue by letting the macro take an argument
directly, and removing the jump on save_calls. I've also taken the
opportunity to move all dispatch-related macros to macros.tab as
it's a pinch cleaner to gather everything there.

6 files changed, 137 insertions, 167 deletions

diff --git a/erts/emulator/beam/beam_emu.c b/erts/emulator/beam/beam_emu.c
index 9f8b56a5d5..27062089c6 100644
--- a/erts/emulator/beam/beam_emu.c
+++ b/erts/emulator/beam/beam_emu.c
@@ -250,72 +250,6 @@ void** beam_ops;
 #define Q(N) (N*sizeof(Eterm *))
 #define l(N) (freg[N].fd)
 
-/*
- * Check that we haven't used the reductions and jump to function pointed to by
- * the I register.  If we are out of reductions, do a context switch.
- */
-
-#define DispatchMacro()				\
-  do {						\
-     BeamInstr dis_next;                        \
-     dis_next = *I;                             \
-     CHECK_ARGS(I);				\
-     if (FCALLS > 0 || FCALLS > neg_o_reds) {	\
-        FCALLS--;				\
-        Goto(dis_next);				\
-     } else {					\
-	goto context_switch;			\
-     }						\
- } while (0)                                    \
-
-#define DispatchMacroFun()			\
-  do {						\
-     BeamInstr dis_next;                        \
-     dis_next = *I;                             \
-     CHECK_ARGS(I);				\
-     if (FCALLS > 0 || FCALLS > neg_o_reds) {	\
-        FCALLS--;				\
-        Goto(dis_next);				\
-     } else {					\
-	goto context_switch_fun;		\
-     }						\
- } while (0)
-
-#define DispatchMacrox()                                                \
-  do {                                                                  \
-     if (FCALLS > 0) {                                                  \
-        BeamInstr dis_next;                                             \
-        SET_I(((Export *) Arg(0))->addressv[erts_active_code_ix()]);    \
-        dis_next = *I;                                                  \
-        FCALLS--;                                                       \
-        CHECK_ARGS(I);                                                  \
-        Goto(dis_next);                                                 \
-     } else if (ERTS_PROC_GET_SAVED_CALLS_BUF(c_p)                      \
-		&& FCALLS > neg_o_reds) {                               \
-        goto save_calls1;                                               \
-     } else {                                                           \
-        SET_I(((Export *) Arg(0))->addressv[erts_active_code_ix()]);    \
-        CHECK_ARGS(I);                                                  \
-	goto context_switch;                                            \
-     }                                                                  \
- } while (0)
-
-#ifdef DEBUG
-/*
- * To simplify breakpoint setting, put the code in one place only and jump to it.
- */
-#  define Dispatch() goto do_dispatch
-#  define Dispatchx() goto do_dispatchx
-#  define Dispatchfun() goto do_dispatchfun
-#else
-/*
- * Inline for speed.
- */
-#  define Dispatch() DispatchMacro()
-#  define Dispatchx() DispatchMacrox()
-#  define Dispatchfun() DispatchMacroFun()
-#endif
-
 #define Arg(N)       I[(N)+1]
 
 #define GetSource(raw, dst)			\
@@ -348,19 +282,6 @@ do {						\
     }						\
 } while(0)
 
-#define DispatchReturn                          \
-do {                                            \
-    if (FCALLS > 0 || FCALLS > neg_o_reds) {	\
-        FCALLS--;				\
-        Goto(*I);                               \
-    }                                           \
-    else {					\
-        c_p->current = NULL;                    \
-        c_p->arity = 1;                         \
-        goto context_switch3;			\
-    }						\
-} while (0)
-
 #ifdef DEBUG
 /* Better static type testing by the C compiler */
 #  define BEAM_IS_TUPLE(Src) is_tuple(Src)
@@ -768,27 +689,9 @@ void process_main(Eterm * x_reg_array, FloatDef* f_reg_array)
 #endif
 
 #include "beam_hot.h"
-
-#ifdef DEBUG
-    /*
-     * Set a breakpoint here to get control just after a call instruction.
-     * I points to the first instruction in the called function.
-     *
-     * In gdb, use 'call dis(I-5, 1)' to show the name of the function.
-     */
- do_dispatch:
-     DispatchMacro();
-
- do_dispatchx:
-     DispatchMacrox();
-
- do_dispatchfun:
-     DispatchMacroFun();
-
-#endif
-
     /*
-     * Jumped to from the Dispatch() macro when the reductions are used up.
+     * The labels are jumped to from the $DISPATCH() macros when the reductions
+     * are used up.
      *
      * Since the I register points just beyond the FuncBegin instruction, we
      * can get the module, function, and arity for the function being
@@ -982,19 +885,6 @@ void process_main(Eterm * x_reg_array, FloatDef* f_reg_array)
   }
 #endif
     return;			/* Never executed */
-
-  save_calls1:
-    {
-	BeamInstr dis_next;
-
-	save_calls(c_p, (Export *) Arg(0));
-
-	SET_I(((Export *) Arg(0))->addressv[erts_active_code_ix()]);
-
-	dis_next = *I;
-	FCALLS--;
-	Goto(dis_next);
-    }
 }
 
 /*
diff --git a/erts/emulator/beam/bif_instrs.tab b/erts/emulator/beam/bif_instrs.tab
index f1877882a1..c02dcd543c 100644
--- a/erts/emulator/beam/bif_instrs.tab
+++ b/erts/emulator/beam/bif_instrs.tab
@@ -283,7 +283,7 @@ call_bif(Exp) {
         $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
         SET_I(c_p->i);
         SWAPIN;
-        Dispatch();
+        $DISPATCH();
     }
 
     /*
@@ -371,7 +371,7 @@ call_bif_only(Exp) {
          */
         SET_I(c_p->i);
         SWAPIN;
-        Dispatch();
+        $DISPATCH();
     }
 
     /*
@@ -416,7 +416,7 @@ send() {
         $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
         SET_I(c_p->i);
         SWAPIN;
-        Dispatch();
+        $DISPATCH();
     } else {
         goto find_func_info;
     }
@@ -557,6 +557,7 @@ nif_bif.apply_bif() {
 }
 
 nif_bif.epilogue() {
+    //| -no_next
     ERTS_REQ_PROC_MAIN_LOCK(c_p);
     ERTS_HOLE_CHECK(c_p);
     if (ERTS_IS_GC_DESIRED(c_p)) {
@@ -578,7 +579,7 @@ nif_bif.epilogue() {
             c_p->flags &= ~F_HIBERNATE_SCHED;
             goto do_schedule;
         }
-        Dispatch();
+        $DISPATCH();
     }
     {
         BeamInstr *cp = cp_val(*E);
diff --git a/erts/emulator/beam/instrs.tab b/erts/emulator/beam/instrs.tab
index 38b1e5909b..9396c09182 100644
--- a/erts/emulator/beam/instrs.tab
+++ b/erts/emulator/beam/instrs.tab
@@ -120,7 +120,7 @@ dealloc_ret.execute() {
     E = ADD_BYTE_OFFSET(E, num_bytes);
     $RETURN();
     CHECK_TERM(x(0));
-    DispatchReturn;
+    $DISPATCH_RETURN();
 }
 
 move_deallocate_return(Src, Deallocate) {
@@ -137,31 +137,19 @@ move_deallocate_return(Src, Deallocate) {
     x(0) = src;
     $RETURN();
     CHECK_TERM(x(0));
-    DispatchReturn;
+    $DISPATCH_RETURN();
 }
 
 // Call instructions
 
-DISPATCH_REL(CallDest) {
-    //| -no_next
-    $SET_I_REL($CallDest);
-    DTRACE_LOCAL_CALL(c_p, erts_code_to_codemfa(I));
-    Dispatch();
-}
-
-DISPATCH_ABS(CallDest) {
-    //| -no_next
-    SET_I((BeamInstr *) $CallDest);
-    DTRACE_LOCAL_CALL(c_p, erts_code_to_codemfa(I));
-    Dispatch();
-}
-
 i_call(CallDest) {
+    //| -no_next
     $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
     $DISPATCH_REL($CallDest);
 }
 
 move_call(Src, CallDest) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
@@ -170,11 +158,13 @@ move_call(Src, CallDest) {
 }
 
 i_call_last(CallDest, Deallocate) {
+    //| -no_next
     $deallocate($Deallocate);
     $DISPATCH_REL($CallDest);
 }
 
 move_call_last(Src, CallDest, Deallocate) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     $deallocate($Deallocate);
@@ -183,59 +173,59 @@ move_call_last(Src, CallDest, Deallocate) {
 }
 
 i_call_only(CallDest) {
+    //| -no_next
     $DISPATCH_REL($CallDest);
 }
 
 move_call_only(Src, CallDest) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     x(0) = src;
     $DISPATCH_REL(call_dest);
 }
 
-DISPATCHX(Dest) {
-    //| -no_next
-    DTRACE_GLOBAL_CALL_FROM_EXPORT(c_p, $Dest);
-    // Dispatchx assumes the Export* is in Arg(0)
-    I = (&$Dest) - 1;
-    Dispatchx();
-}
-
 i_call_ext(Dest) {
+    //| -no_next
     $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
-    $DISPATCHX($Dest);
+    $DISPATCH_EXPORT($Dest);
 }
 
 i_move_call_ext(Src, CallDest) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
     x(0) = src;
-    $DISPATCHX(call_dest);
+    $DISPATCH_EXPORT(call_dest);
 }
 
 i_call_ext_only(Dest) {
-    $DISPATCHX($Dest);
+    //| -no_next
+    $DISPATCH_EXPORT($Dest);
 }
 
 i_move_call_ext_only(CallDest, Src) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     x(0) = src;
-    $DISPATCHX(call_dest);
+    $DISPATCH_EXPORT(call_dest);
 }
 
 i_call_ext_last(Dest, Deallocate) {
+    //| -no_next
     $deallocate($Deallocate);
-    $DISPATCHX($Dest);
+    $DISPATCH_EXPORT($Dest);
 }
 
 i_move_call_ext_last(CallDest, Deallocate, Src) {
+    //| -no_next
     Eterm call_dest = $CallDest;
     Eterm src = $Src;
     $deallocate($Deallocate);
     x(0) = src;
-    $DISPATCHX(call_dest);
+    $DISPATCH_EXPORT(call_dest);
 }
 
 APPLY(I, Deallocate, Next) {
@@ -251,6 +241,7 @@ HANDLE_APPLY_ERROR() {
 }
 
 i_apply() {
+    //| -no_next
     BeamInstr *next;
     $APPLY(NULL, 0, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -261,6 +252,7 @@ i_apply() {
 }
 
 i_apply_last(Deallocate) {
+    //| -no_next
     BeamInstr *next;
     $APPLY(I, $Deallocate, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -271,6 +263,7 @@ i_apply_last(Deallocate) {
 }
 
 i_apply_only() {
+    //| -no_next
     BeamInstr *next;
     $APPLY(I, 0, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -287,6 +280,7 @@ FIXED_APPLY(Arity, I, Deallocate, Next) {
 }
 
 apply(Arity) {
+    //| -no_next
     BeamInstr *next;
     $FIXED_APPLY($Arity, NULL, 0, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -297,6 +291,7 @@ apply(Arity) {
 }
 
 apply_last(Arity, Deallocate) {
+    //| -no_next
     BeamInstr *next;
     $FIXED_APPLY($Arity, I, $Deallocate, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -316,13 +311,8 @@ HANDLE_APPLY_FUN_ERROR() {
      goto find_func_info;
 }
 
-DISPATCH_FUN(I) {
-    //| -no_next
-    SET_I($I);
-    Dispatchfun();
-}
-
 i_apply_fun() {
+    //| -no_next
     BeamInstr *next;
     $APPLY_FUN(next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -333,6 +323,7 @@ i_apply_fun() {
 }
 
 i_apply_fun_last(Deallocate) {
+    //| -no_next
     BeamInstr *next;
     $APPLY_FUN(next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -343,6 +334,7 @@ i_apply_fun_last(Deallocate) {
 }
 
 i_apply_fun_only() {
+    //| -no_next
     BeamInstr *next;
     $APPLY_FUN(next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -359,6 +351,7 @@ CALL_FUN(Fun, Next) {
 }
 
 i_call_fun(Fun) {
+    //| -no_next
     BeamInstr *next;
     $CALL_FUN($Fun, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -369,6 +362,7 @@ i_call_fun(Fun) {
 }
 
 i_call_fun_last(Fun, Deallocate) {
+    //| -no_next
     BeamInstr *next;
     $CALL_FUN($Fun, next);
     if (ERTS_LIKELY(next != NULL)) {
@@ -381,10 +375,12 @@ i_call_fun_last(Fun, Deallocate) {
 return() {
     //| -no_next
     $RETURN();
+
     DTRACE_RETURN_FROM_PC(c_p);
     CHECK_TERM(r(0));
     HEAP_SPACE_VERIFIED(0);
-    DispatchReturn;
+
+    $DISPATCH_RETURN();
 }
 
 get_list(Src, Hd, Tl) {
@@ -677,7 +673,7 @@ move_return(Src) {
     //| -no_next
     x(0) = $Src;
     $RETURN();
-    DispatchReturn;
+    $DISPATCH_RETURN();
 }
 
 move_x1(Src) {
diff --git a/erts/emulator/beam/macros.tab b/erts/emulator/beam/macros.tab
index 9d183e1f41..848e35d45c 100644
--- a/erts/emulator/beam/macros.tab
+++ b/erts/emulator/beam/macros.tab
@@ -136,6 +136,90 @@ AH(NeedStack, NeedHeap, Live) {
     *E = NIL;
 }
 
+
+//
+// Helpers for call instructions
+//
+
+DISPATCH() {
+    BeamInstr dis_next;
+
+    dis_next = *I;
+    CHECK_ARGS(I);
+
+    if (FCALLS > 0 || FCALLS > neg_o_reds) {
+        FCALLS--;
+        Goto(dis_next);
+    } else {
+        goto context_switch;
+    }
+}
+
+DISPATCH_ABS(CallDest) {
+    SET_I((BeamInstr *) $CallDest);
+    DTRACE_LOCAL_CALL(c_p, erts_code_to_codemfa(I));
+
+    $DISPATCH();
+}
+
+DISPATCH_EXPORT(Export) {
+    BeamInstr dis_next;
+    Export *ep;
+
+    ep = (Export*)($Export);
+
+    DTRACE_GLOBAL_CALL_FROM_EXPORT(c_p, ep);
+
+    SET_I(ep->addressv[erts_active_code_ix()]);
+    CHECK_ARGS(I);
+    dis_next = *I;
+
+    if (ERTS_UNLIKELY(FCALLS <= 0)) {
+        if (ERTS_PROC_GET_SAVED_CALLS_BUF(c_p) && FCALLS > neg_o_reds) {
+            save_calls(c_p, ep);
+        } else {
+            goto context_switch;
+        }
+    }
+
+    FCALLS--;
+    Goto(dis_next);
+}
+
+DISPATCH_FUN(I) {
+    BeamInstr dis_next;
+
+    SET_I($I);
+
+    dis_next = *I;
+    CHECK_ARGS(I);
+
+    if (FCALLS > 0 || FCALLS > neg_o_reds) {
+        FCALLS--;
+        Goto(dis_next);
+    } else {
+        goto context_switch_fun;
+    }
+}
+
+DISPATCH_REL(CallDest) {
+    $SET_I_REL($CallDest);
+    DTRACE_LOCAL_CALL(c_p, erts_code_to_codemfa(I));
+
+    $DISPATCH();
+}
+
+DISPATCH_RETURN() {
+    if (FCALLS > 0 || FCALLS > neg_o_reds) {
+        FCALLS--;
+        Goto(*I);
+    } else {
+        c_p->current = NULL;
+        c_p->arity = 1;
+        goto context_switch3;
+    }
+}
+
 // Save the continuation pointer in the reserved slot at the
 // top of the stack as preparation for doing a function call.
 
diff --git a/erts/emulator/hipe/hipe_instrs.tab b/erts/emulator/hipe/hipe_instrs.tab
index 62162fcb9c..8aa8544b2a 100644
--- a/erts/emulator/hipe/hipe_instrs.tab
+++ b/erts/emulator/hipe/hipe_instrs.tab
@@ -93,7 +93,7 @@ hipe_trap.post() {
         /*fall through*/
     case HIPE_MODE_SWITCH_RES_CALL_BEAM:
         SET_I(c_p->i);
-        Dispatch();
+        $DISPATCH();
     case HIPE_MODE_SWITCH_RES_CALL_CLOSURE:
         /* This can be used to call any function value, but currently
            it's only used to call closures referring to unloaded
@@ -104,8 +104,7 @@ hipe_trap.post() {
             next = call_fun(c_p, c_p->arity - 1, reg, THE_NON_VALUE);
             HEAVY_SWAPIN;
             if (next != NULL) {
-                SET_I(next);
-                Dispatchfun();
+                $DISPATCH_FUN(next);
             }
             goto find_func_info;
         }
diff --git a/erts/emulator/internal_doc/beam_makeops.md b/erts/emulator/internal_doc/beam_makeops.md
index 2880099b70..267af78412 100644
--- a/erts/emulator/internal_doc/beam_makeops.md
+++ b/erts/emulator/internal_doc/beam_makeops.md
@@ -1457,26 +1457,26 @@ all instructions.  It expands to the address of the next instruction.
 Here is an example:
 
     i_call(CallDest) {
-        SET_CP(c_p, $NEXT_INSTRUCTION);
+        //| -no_next
+        $SAVE_CONTINUATION_POINTER($NEXT_INSTRUCTION);
         $DISPATCH_REL($CallDest);
     }
 
-When calling a function, the return address is first stored in `c_p->cp`
-(using the `SET_CP()` macro defined in `beam_emu.c`), and then control is
+When calling a function, the return address is first stored in `E[0]`
+(using the `$SAVE_CONTINUATION_POINTER()` macro), and then control is
 transferred to the callee.  Here is the generated code:
 
     OpCase(i_call_f):
     {
-      SET_CP(c_p, I+1);
-      ASSERT(VALID_INSTR(*(I + (fb(BeamExtraData(I[0]))) + 0)));
-      I += fb(BeamExtraData(I[0])) + 0;;
-      DTRACE_LOCAL_CALL(c_p, erts_code_to_codemfa(I));
-      Dispatch();;
+        ASSERT(VALID_INSTR(*(I+2)));
+        *E = (BeamInstr) (I+2);;
+
+        /* ... dispatch code intentionally left out ... */
     }
 
-We can see that that `$NEXT_INSTRUCTION` has been expanded to `I+1`.
+We can see that that `$NEXT_INSTRUCTION` has been expanded to `I+2`.
 That makes sense since the size of the `i_call_f/1` instruction is
-one word.
+two words.
 
 ##### The IP_ADJUSTMENT pre-bound variable #####