Refine & simplify the interBreak mechanism

Replace dvmUpdateInterpBreak() and friends with more direct
enable/disable subMode calls.  Hide breakFlags manipulation from
higher-level callers and infer what is needed from the active
subMode.

Add documentation to the interpreter control section of
mterp/README.txt

Change-Id: If7ebee5d8e4db8154c4caed72cf89ec088045998

4 files changed, 105 insertions, 109 deletions

diff --git a/vm/interp/Interp.cpp b/vm/interp/Interp.cpp
index 40fbcbb91..c219f363c 100644
--- a/vm/interp/Interp.cpp
+++ b/vm/interp/Interp.cpp
@@ -1495,24 +1495,22 @@ void dvmThrowVerificationError(const Method* method, int kind, int ref)
 }
 
 /*
- * Update interpBreak.  If there is an active break when
- * we're done, set altHandlerTable.  Otherwise, revert to
- * the non-breaking table base.
+ * Update interpBreak for a single thread.
  */
-void dvmUpdateInterpBreak(Thread* thread, int newBreak, int newMode,
-                          bool enable)
+void updateInterpBreak(Thread* thread, ExecutionSubModes subMode, bool enable)
 {
     InterpBreak oldValue, newValue;
-
     do {
         oldValue = newValue = thread->interpBreak;
-        if (enable) {
-            newValue.ctl.breakFlags |= newBreak;
-            newValue.ctl.subMode |= newMode;
-        } else {
-            newValue.ctl.breakFlags &= ~newBreak;
-            newValue.ctl.subMode &= ~newMode;
-        }
+        newValue.ctl.breakFlags = kInterpNoBreak;  // Assume full reset
+        if (enable)
+            newValue.ctl.subMode |= subMode;
+        else
+            newValue.ctl.subMode &= ~subMode;
+        if (newValue.ctl.subMode & SINGLESTEP_BREAK_MASK)
+            newValue.ctl.breakFlags |= kInterpSingleStep;
+        if (newValue.ctl.subMode & SAFEPOINT_BREAK_MASK)
+            newValue.ctl.breakFlags |= kInterpSafePoint;
         newValue.ctl.curHandlerTable = (newValue.ctl.breakFlags) ?
             thread->altHandlerTable : thread->mainHandlerTable;
     } while (dvmQuasiAtomicCas64(oldValue.all, newValue.all,
@@ -1520,6 +1518,21 @@ void dvmUpdateInterpBreak(Thread* thread, int newBreak, int newMode,
 }
 
 /*
+ * Update interpBreak for all threads.
+ */
+void updateAllInterpBreak(ExecutionSubModes subMode, bool enable)
+{
+    Thread* self = dvmThreadSelf();
+    Thread* thread;
+
+    dvmLockThreadList(self);
+    for (thread = gDvm.threadList; thread != NULL; thread = thread->next) {
+        updateInterpBreak(thread, subMode, enable);
+    }
+    dvmUnlockThreadList();
+}
+
+/*
  * Update the normal and debugger suspend counts for a thread.
  * threadSuspendCount must be acquired before calling this to
  * ensure a clean update of suspendCount, dbgSuspendCount and
@@ -1532,26 +1545,31 @@ void dvmAddToSuspendCounts(Thread* thread, int delta, int dbgDelta)
 {
     thread->suspendCount += delta;
     thread->dbgSuspendCount += dbgDelta;
-    dvmUpdateInterpBreak(thread, kInterpSuspendBreak, kSubModeNormal,
-                         (thread->suspendCount != 0) /* enable break? */);
-
+    updateInterpBreak(thread, kSubModeSuspendPending,
+                      (thread->suspendCount != 0));
     // Update the global suspend count total
     gDvm.sumThreadSuspendCount += delta;
 }
 
-/*
- * Update interpBreak for all threads.
- */
-void dvmUpdateAllInterpBreak(int newBreak, int newMode, bool enable)
+
+void dvmDisableSubMode(Thread* thread, ExecutionSubModes subMode)
 {
-    Thread* self = dvmThreadSelf();
-    Thread* thread;
+    updateInterpBreak(thread, subMode, false);
+}
 
-    dvmLockThreadList(self);
-    for (thread = gDvm.threadList; thread != NULL; thread = thread->next) {
-        dvmUpdateInterpBreak(thread, newBreak, newMode, enable);
-    }
-    dvmUnlockThreadList();
+void dvmEnableSubMode(Thread* thread, ExecutionSubModes subMode)
+{
+    updateInterpBreak(thread, subMode, true);
+}
+
+void dvmEnableAllSubMode(ExecutionSubModes subMode)
+{
+    updateAllInterpBreak(subMode, true);
+}
+
+void dvmDisableAllSubMode(ExecutionSubModes subMode)
+{
+    updateAllInterpBreak(subMode, false);
 }
 
 /*
@@ -1616,8 +1634,11 @@ void dvmArmSafePointCallback(Thread* thread, SafePointCallback funct,
     if ((funct == NULL) || (thread->callback == NULL)) {
         thread->callback = funct;
         thread->callbackArg = arg;
-        dvmUpdateInterpBreak(thread, kInterpSafePointCallback,
-                             kSubModeNormal, (funct != NULL));
+        if (funct != NULL) {
+            dvmEnableSubMode(thread, kSubModeCallbackPending);
+        } else {
+            dvmDisableSubMode(thread, kSubModeCallbackPending);
+        }
     } else {
         // Already armed.  Different?
         if ((funct != thread->callback) ||
@@ -1675,21 +1696,18 @@ void dvmInitInterpreterState(Thread* self)
  */
 void dvmInitializeInterpBreak(Thread* thread)
 {
-    u1 flags = 0;
-    u1 subModes = 0;
-
     if (gDvm.instructionCountEnableCount > 0) {
-        flags |= kInterpInstCountBreak;
-        subModes |= kSubModeInstCounting;
+        dvmEnableSubMode(thread, kSubModeInstCounting);
     }
     if (dvmIsMethodTraceActive()) {
-        subModes |= kSubModeMethodTrace;
+        dvmEnableSubMode(thread, kSubModeMethodTrace);
+    }
+    if (gDvm.emulatorTraceEnableCount > 0) {
+        dvmEnableSubMode(thread, kSubModeEmulatorTrace);
     }
     if (gDvm.debuggerActive) {
-        flags |= kInterpDebugBreak;
-        subModes |= kSubModeDebuggerActive;
+        dvmEnableSubMode(thread, kSubModeDebuggerActive);
     }
-    dvmUpdateInterpBreak(thread, flags, subModes, true);
 }
 
 /*
@@ -1733,13 +1751,13 @@ void dvmCheckBefore(const u2 *pc, u4 *fp, Thread* self)
 
     /* Safe point handling */
     if (self->suspendCount ||
-        (self->interpBreak.ctl.breakFlags & kInterpSafePointCallback)) {
+        (self->interpBreak.ctl.subMode & kSubModeCallbackPending)) {
         // Are we are a safe point?
         int flags;
         flags = dexGetFlagsFromOpcode(dexOpcodeFromCodeUnit(*pc));
         if (flags & VERIFY_GC_INST_MASK) {
             // Yes, at a safe point.  Pending callback?
-            if (self->interpBreak.ctl.breakFlags & kInterpSafePointCallback) {
+            if (self->interpBreak.ctl.subMode & kSubModeCallbackPending) {
                 SafePointCallback callback;
                 void* arg;
                 // Get consistent funct/arg pair
@@ -1788,7 +1806,8 @@ void dvmCheckBefore(const u2 *pc, u4 *fp, Thread* self)
 
 #if defined(WITH_JIT)
     // Does the JIT need anything done now?
-    if (self->interpBreak.ctl.breakFlags & kInterpJitBreak) {
+    if (self->interpBreak.ctl.subMode &
+            (kSubModeJitTraceBuild | kSubModeJitSV)) {
         // Are we building a trace?
         if (self->interpBreak.ctl.subMode & kSubModeJitTraceBuild) {
             dvmCheckJit(pc, self);
@@ -1810,8 +1829,7 @@ void dvmCheckBefore(const u2 *pc, u4 *fp, Thread* self)
     if (self->interpBreak.ctl.breakFlags & kInterpSingleStep) {
         if (self->singleStepCount == 0) {
             // We've exhausted our single step count
-            dvmUpdateInterpBreak(self, kInterpSingleStep, kSubModeNormal,
-                                 false /* remove */);
+            dvmDisableSubMode(self, kSubModeCountedStep);
 #if defined(WITH_JIT)
 #if 0
             /*
@@ -1878,8 +1896,7 @@ void dvmCheckBefore(const u2 *pc, u4 *fp, Thread* self)
 void dvmInterpret(Thread* self, const Method* method, JValue* pResult)
 {
     InterpSaveState interpSaveState;
-    int savedBreakFlags;
-    int savedSubModes;
+    ExecutionSubModes savedSubModes;
 
 #if defined(WITH_JIT)
     /* Target-specific save/restore */
@@ -1903,11 +1920,10 @@ void dvmInterpret(Thread* self, const Method* method, JValue* pResult)
      * Strip out and save any flags that should not be inherited by
      * nested interpreter activation.
      */
-    savedBreakFlags = self->interpBreak.ctl.breakFlags & LOCAL_BREAKFLAGS;
-    savedSubModes = self->interpBreak.ctl.subMode & LOCAL_SUBMODE;
-    if (savedBreakFlags | savedSubModes) {
-        dvmUpdateInterpBreak(self, savedBreakFlags, savedSubModes,
-                             false /*disable*/);
+    savedSubModes = (ExecutionSubModes)(
+              self->interpBreak.ctl.subMode & LOCAL_SUBMODE);
+    if (savedSubModes != kSubModeNormal) {
+        dvmDisableSubMode(self, savedSubModes);
     }
 #if defined(WITH_JIT)
     dvmJitCalleeSave(calleeSave);
@@ -1970,8 +1986,7 @@ void dvmInterpret(Thread* self, const Method* method, JValue* pResult)
 #if defined(WITH_JIT)
     dvmJitCalleeRestore(calleeSave);
 #endif
-    if (savedBreakFlags | savedSubModes) {
-        dvmUpdateInterpBreak(self, savedBreakFlags, savedSubModes,
-                             true /*enable*/);
+    if (savedSubModes != kSubModeNormal) {
+        dvmEnableSubMode(self, savedSubModes);
     }
 }
diff --git a/vm/interp/Interp.h b/vm/interp/Interp.h
index 6951c5af4..f0f21986d 100644
--- a/vm/interp/Interp.h
+++ b/vm/interp/Interp.h
@@ -89,20 +89,17 @@ void dvmReportInvoke(Thread* self, const Method* methodToCall);
 void dvmReportReturn(Thread* self);
 
 /*
- * Update interpBreak
+ * InterpBreak & subMode control
  */
-void dvmUpdateInterpBreak(Thread* thread, int newBreak, int newMode,
-                          bool enable);
+void dvmDisableSubMode(Thread* thread, ExecutionSubModes subMode);
+void dvmEnableSubMode(Thread* thread, ExecutionSubModes subMode);
+void dvmDisableAllSubMode(ExecutionSubModes subMode);
+void dvmEnableAllSubMode(ExecutionSubModes subMode);
 void dvmAddToSuspendCounts(Thread* thread, int delta, int dbgDelta);
 void dvmCheckInterpStateConsistency();
 void dvmInitializeInterpBreak(Thread* thread);
 
 /*
- * Update interpBreak for all threads
- */
-void dvmUpdateAllInterpBreak(int newBreak, int newMode, bool enable);
-
-/*
  * Register a callback to occur at the next safe point for a single thread.
  * If funct is NULL, the previous registration is cancelled.
  *
diff --git a/vm/interp/InterpState.h b/vm/interp/InterpState.h
index 144cd2a17..d50967dca 100644
--- a/vm/interp/InterpState.h
+++ b/vm/interp/InterpState.h
@@ -47,15 +47,16 @@ typedef enum ExecutionMode {
  * code generated by the JIT.  Take care when changing.
  */
 typedef enum ExecutionSubModes {
-    kSubModeNormal         = 0x00,
-    kSubModeMethodTrace    = 0x01,
-    kSubModeEmulatorTrace  = 0x02,
-    kSubModeInstCounting   = 0x04,
-    kSubModeDebuggerActive = 0x08,
-#if defined(WITH_JIT)
-    kSubModeJitTraceBuild  = 0x10,
-    kSubModeJitSV          = 0x20,
-#endif
+    kSubModeNormal            = 0x0000,   /* No active subMode */
+    kSubModeMethodTrace       = 0x0001,
+    kSubModeEmulatorTrace     = 0x0002,
+    kSubModeInstCounting      = 0x0004,
+    kSubModeDebuggerActive    = 0x0008,
+    kSubModeSuspendPending    = 0x0010,
+    kSubModeCallbackPending   = 0x0020,
+    kSubModeCountedStep       = 0x0040,
+    kSubModeJitTraceBuild     = 0x4000,
+    kSubModeJitSV             = 0x8000,
     kSubModeDebugProfile   = (kSubModeMethodTrace |
                               kSubModeEmulatorTrace |
                               kSubModeInstCounting |
@@ -69,44 +70,31 @@ typedef enum ExecutionSubModes {
  */
 
 typedef enum InterpBreakFlags {
-    kInterpNoBreak            = 0x00,
-    kInterpSuspendBreak       = 0x01,
-    kInterpInstCountBreak     = 0x02,
-    kInterpDebugBreak         = 0x04,
-    kInterpEmulatorTraceBreak = 0x08,
-    kInterpSingleStep         = 0x10,
-    kInterpSafePointCallback  = 0x20,
-#if defined(WITH_JIT)
-    kInterpJitBreak           = 0x40,
-#endif
+    kInterpNoBreak            = 0x00,    /* Don't check */
+    kInterpSingleStep         = 0x01,    /* Check between each inst */
+    kInterpSafePoint          = 0x02,    /* Check at safe points */
 } InterpBreakFlags;
 
+/*
+ * Mapping between subModes and required check intervals.  Note: in
+ * the future we might want to make this mapping target-dependent.
+ */
+#define SINGLESTEP_BREAK_MASK ( kSubModeInstCounting | \
+                                kSubModeDebuggerActive | \
+                                kSubModeCountedStep | \
+                                kSubModeJitSV | \
+                                kSubModeJitTraceBuild )
+
+#define SAFEPOINT_BREAK_MASK  ( kSubModeSuspendPending | \
+                                kSubModeCallbackPending )
+
 typedef bool (*SafePointCallback)(struct Thread* thread, void* arg);
 
 /*
  * Identify which break and submode flags should be local
  * to an interpreter activation.
  */
-#if defined(WITH_JIT)
 #define LOCAL_SUBMODE (kSubModeJitTraceBuild)
-#define LOCAL_BREAKFLAGS (kInterpJitBreak | kInterpSingleStep)
-#else
-#define LOCAL_SUBMODE (0)
-#define LOCAL_BREAKFLAGS (0)
-#endif
-
-
-/*
- * Specify the starting point when switching between interpreters.
- */
-typedef enum InterpEntry {
-    kInterpEntryInstr = 0,      // continue to next instruction
-    kInterpEntryReturn = 1,     // jump to method return
-    kInterpEntryThrow = 2,      // jump to exception throw
-#if defined(WITH_JIT)
-    kInterpEntryResume = 3,     // Resume after single-step
-#endif
-} InterpEntry;
 
 typedef struct InterpSaveState {
     const u2*       pc;         // Dalvik PC
diff --git a/vm/interp/Jit.cpp b/vm/interp/Jit.cpp
index c048ed617..3434a2c77 100644
--- a/vm/interp/Jit.cpp
+++ b/vm/interp/Jit.cpp
@@ -395,16 +395,15 @@ void dvmCheckSelfVerification(const u2* pc, Thread* self)
             assert(self->jitResumeNPC != NULL);
             assert(self->singleStepCount == 0);
             self->singleStepCount = 1;
-            dvmUpdateInterpBreak(self, kInterpSingleStep, kSubModeNormal,
-                                 true /* enable */);
+            dvmEnableSubMode(self, kSubModeCountedStep);
         }
 
         /*
          * Switch off shadow replay mode.  The next shadowed trace
          * execution will turn it back on.
          */
-        dvmUpdateInterpBreak(self, kInterpJitBreak, kSubModeJitSV,
-                             false /* disable */);
+        dvmDisableSubMode(self, kSubModeJitSV);
+
         self->jitState = kJitDone;
         return;
     }
@@ -976,14 +975,12 @@ void dvmCheckJit(const u2* pc, Thread* self)
      * If we're done with trace selection, switch off the control flags.
      */
      if (allDone) {
-         dvmUpdateInterpBreak(self, kInterpJitBreak,
-                              kSubModeJitTraceBuild, false);
+         dvmDisableSubMode(self, kSubModeJitTraceBuild);
          if (stayOneMoreInst) {
              // Keep going in single-step mode for at least one more inst
              assert(self->jitResumeNPC == NULL);
              self->singleStepCount = MIN(1, self->singleStepCount);
-             dvmUpdateInterpBreak(self, kInterpSingleStep, kSubModeNormal,
-                                  true /* enable */);
+             dvmEnableSubMode(self, kSubModeCountedStep);
          }
      }
      return;
@@ -1276,8 +1273,7 @@ void dvmJitCheckTraceRequest(Thread* self)
                 self->trace[0].isCode = true;
                 self->lastPC = 0;
                 /* Turn on trace selection mode */
-                dvmUpdateInterpBreak(self, kInterpJitBreak,
-                                     kSubModeJitTraceBuild, true);
+                dvmEnableSubMode(self, kSubModeJitTraceBuild);
 #if defined(SHOW_TRACE)
                 LOGD("Starting trace for %s at 0x%x",
                      self->interpSave.method->name, (int)self->interpSave.pc);