Interpreter/Debugger fix #4479968
This one was tricky to track down. The underlying problem arose
with the consolidation of InterpState with Thread. Rather than
having a state structure for each instance of the interpreter, we
moved to a model that had a single thread-local struct shared by all
interpreter instances running on that thread. A portion of interpreter
state can't be shared - and thus was saved and restored on nested
invocations of the interpreter.
The bug here was that the storage for method return values was not
included in the state that needed save/retore. In normal operation,
it doesn't need to be saved - that storage isn't live across an
invoke that could trigger a nested interpreter activation. However,
when debugging, the debugger itself may hijack threads and create
new interpreter instances for its own purposed - and there is a small
window in which live retval can be trashed.
The fix is simply to move retval into the InterpSave struct.
Change-Id: Ib621824b799c5caa16fdfa8f5689a181159059df
diff --git a/vm/Thread.h b/vm/Thread.h
index 57ae24f..a4c64ec 100644
--- a/vm/Thread.h
+++ b/vm/Thread.h
@@ -118,7 +118,6 @@
* be located towards the beginning of the Thread structure for
* efficiency.
*/
- JValue retval;
/*
* interpBreak contains info about the interpreter mode, as well as
diff --git a/vm/compiler/codegen/CodegenFactory.cpp b/vm/compiler/codegen/CodegenFactory.cpp
index 61e29d7..f42ae74 100644
--- a/vm/compiler/codegen/CodegenFactory.cpp
+++ b/vm/compiler/codegen/CodegenFactory.cpp
@@ -57,7 +57,7 @@
if (rlSrc.location == kLocPhysReg) {
genRegCopy(cUnit, reg1, rlSrc.lowReg);
} else if (rlSrc.location == kLocRetval) {
- loadWordDisp(cUnit, rSELF, offsetof(Thread, retval), reg1);
+ loadWordDisp(cUnit, rSELF, offsetof(Thread, interpSave.retval), reg1);
} else {
assert(rlSrc.location == kLocDalvikFrame);
loadWordDisp(cUnit, rFP, dvmCompilerS2VReg(cUnit, rlSrc.sRegLow) << 2,
@@ -90,7 +90,8 @@
if (rlSrc.location == kLocPhysReg) {
genRegCopyWide(cUnit, regLo, regHi, rlSrc.lowReg, rlSrc.highReg);
} else if (rlSrc.location == kLocRetval) {
- loadBaseDispWide(cUnit, NULL, rSELF, offsetof(Thread, retval),
+ loadBaseDispWide(cUnit, NULL, rSELF,
+ offsetof(Thread, interpSave.retval),
regLo, regHi, INVALID_SREG);
} else {
assert(rlSrc.location == kLocDalvikFrame);
@@ -124,7 +125,8 @@
rlSrc.location = kLocPhysReg;
dvmCompilerMarkLive(cUnit, rlSrc.lowReg, rlSrc.sRegLow);
} else if (rlSrc.location == kLocRetval) {
- loadWordDisp(cUnit, rSELF, offsetof(Thread, retval), rlSrc.lowReg);
+ loadWordDisp(cUnit, rSELF, offsetof(Thread, interpSave.retval),
+ rlSrc.lowReg);
rlSrc.location = kLocPhysReg;
dvmCompilerClobber(cUnit, rlSrc.lowReg);
}
@@ -164,7 +166,7 @@
if (rlDest.location == kLocRetval) {
- storeBaseDisp(cUnit, rSELF, offsetof(Thread, retval),
+ storeBaseDisp(cUnit, rSELF, offsetof(Thread, interpSave.retval),
rlDest.lowReg, kWord);
dvmCompilerClobber(cUnit, rlDest.lowReg);
} else {
@@ -192,7 +194,8 @@
dvmCompilerMarkLive(cUnit, rlSrc.highReg,
dvmCompilerSRegHi(rlSrc.sRegLow));
} else if (rlSrc.location == kLocRetval) {
- loadBaseDispWide(cUnit, NULL, rSELF, offsetof(Thread, retval),
+ loadBaseDispWide(cUnit, NULL, rSELF,
+ offsetof(Thread, interpSave.retval),
rlSrc.lowReg, rlSrc.highReg, INVALID_SREG);
rlSrc.location = kLocPhysReg;
dvmCompilerClobber(cUnit, rlSrc.lowReg);
@@ -242,7 +245,7 @@
if (rlDest.location == kLocRetval) {
- storeBaseDispWide(cUnit, rSELF, offsetof(Thread, retval),
+ storeBaseDispWide(cUnit, rSELF, offsetof(Thread, interpSave.retval),
rlDest.lowReg, rlDest.highReg);
dvmCompilerClobber(cUnit, rlDest.lowReg);
dvmCompilerClobber(cUnit, rlDest.highReg);
diff --git a/vm/compiler/codegen/arm/CodegenDriver.cpp b/vm/compiler/codegen/arm/CodegenDriver.cpp
index 1e5f4c7..ac59360 100644
--- a/vm/compiler/codegen/arm/CodegenDriver.cpp
+++ b/vm/compiler/codegen/arm/CodegenDriver.cpp
@@ -3678,7 +3678,7 @@
dvmCompilerClobberCallRegs(cUnit);
dvmCompilerClobber(cUnit, r4PC);
dvmCompilerClobber(cUnit, r7);
- int offset = offsetof(Thread, retval);
+ int offset = offsetof(Thread, interpSave.retval);
opRegRegImm(cUnit, kOpAdd, r4PC, r6SELF, offset);
opImm(cUnit, kOpPush, (1<<r4PC) | (1<<r7));
LOAD_FUNC_ADDR(cUnit, r4PC, fn);
diff --git a/vm/compiler/codegen/arm/Thumb/Gen.cpp b/vm/compiler/codegen/arm/Thumb/Gen.cpp
index 18ef762..abc4420 100644
--- a/vm/compiler/codegen/arm/Thumb/Gen.cpp
+++ b/vm/compiler/codegen/arm/Thumb/Gen.cpp
@@ -214,7 +214,7 @@
static bool genInlinedAbsFloat(CompilationUnit *cUnit, MIR *mir)
{
- int offset = offsetof(Thread, retval);
+ int offset = offsetof(Thread, interpSave.retval);
RegLocation rlSrc = dvmCompilerGetSrc(cUnit, mir, 0);
int reg0 = loadValue(cUnit, rlSrc, kCoreReg).lowReg;
int signMask = dvmCompilerAllocTemp(cUnit);
@@ -229,7 +229,7 @@
static bool genInlinedAbsDouble(CompilationUnit *cUnit, MIR *mir)
{
- int offset = offsetof(Thread, retval);
+ int offset = offsetof(Thread, interpSave.retval);
RegLocation rlSrc = dvmCompilerGetSrcWide(cUnit, mir, 0, 1);
RegLocation regSrc = loadValueWide(cUnit, rlSrc, kCoreReg);
int reglo = regSrc.lowReg;
@@ -248,7 +248,7 @@
/* No select in thumb, so we need to branch. Thumb2 will do better */
static bool genInlinedMinMaxInt(CompilationUnit *cUnit, MIR *mir, bool isMin)
{
- int offset = offsetof(Thread, retval);
+ int offset = offsetof(Thread, interpSave.retval);
RegLocation rlSrc1 = dvmCompilerGetSrc(cUnit, mir, 0);
RegLocation rlSrc2 = dvmCompilerGetSrc(cUnit, mir, 1);
int reg0 = loadValue(cUnit, rlSrc1, kCoreReg).lowReg;
diff --git a/vm/interp/Interp.cpp b/vm/interp/Interp.cpp
index 67566e8..7c68e90 100644
--- a/vm/interp/Interp.cpp
+++ b/vm/interp/Interp.cpp
@@ -1984,7 +1984,7 @@
// Call the interpreter
(*stdInterp)(self);
- *pResult = self->retval;
+ *pResult = self->interpSave.retval;
/* Restore interpreter state from previous activation */
self->interpSave = interpSaveState;
diff --git a/vm/interp/InterpState.h b/vm/interp/InterpState.h
index b782275..9997953 100644
--- a/vm/interp/InterpState.h
+++ b/vm/interp/InterpState.h
@@ -99,6 +99,7 @@
u4* curFrame; // Dalvik frame pointer
const Method *method; // Method being executed
DvmDex* methodClassDex;
+ JValue retval;
void* bailPtr;
#if defined(WITH_TRACKREF_CHECKS)
int debugTrackedRefStart;
diff --git a/vm/interp/Jit.cpp b/vm/interp/Jit.cpp
index bb8d6d7..9632e97 100644
--- a/vm/interp/Jit.cpp
+++ b/vm/interp/Jit.cpp
@@ -96,7 +96,7 @@
// Remember original state
shadowSpace->startPC = pc;
shadowSpace->fp = fp;
- shadowSpace->retval = self->retval;
+ shadowSpace->retval = self->interpSave.retval;
shadowSpace->interpStackEnd = self->interpStackEnd;
/*
@@ -165,7 +165,7 @@
self->interpSave.curFrame = shadowSpace->fp;
self->interpSave.method = shadowSpace->method;
self->interpSave.methodClassDex = shadowSpace->methodClassDex;
- self->retval = shadowSpace->retval;
+ self->interpSave.retval = shadowSpace->retval;
self->interpStackEnd = shadowSpace->interpStackEnd;
return shadowSpace;
diff --git a/vm/mterp/common/asm-constants.h b/vm/mterp/common/asm-constants.h
index f9fb928..bee7d11 100644
--- a/vm/mterp/common/asm-constants.h
+++ b/vm/mterp/common/asm-constants.h
@@ -150,15 +150,15 @@
MTERP_OFFSET(offThread_curFrame, Thread, interpSave.curFrame, 4)
MTERP_OFFSET(offThread_method, Thread, interpSave.method, 8)
MTERP_OFFSET(offThread_methodClassDex, Thread, interpSave.methodClassDex, 12)
-MTERP_OFFSET(offThread_bailPtr, Thread, interpSave.bailPtr, 16)
-MTERP_OFFSET(offThread_threadId, Thread, threadId, 28)
-
/* make sure all JValue union members are stored at the same offset */
-MTERP_OFFSET(offThread_retval, Thread, retval, 32)
-MTERP_OFFSET(offThread_retval_z, Thread, retval.z, 32)
-MTERP_OFFSET(offThread_retval_i, Thread, retval.i, 32)
-MTERP_OFFSET(offThread_retval_j, Thread, retval.j, 32)
-MTERP_OFFSET(offThread_retval_l, Thread, retval.l, 32)
+MTERP_OFFSET(offThread_retval, Thread, interpSave.retval, 16)
+MTERP_OFFSET(offThread_retval_z, Thread, interpSave.retval.z, 16)
+MTERP_OFFSET(offThread_retval_i, Thread, interpSave.retval.i, 16)
+MTERP_OFFSET(offThread_retval_j, Thread, interpSave.retval.j, 16)
+MTERP_OFFSET(offThread_retval_l, Thread, interpSave.retval.l, 16)
+MTERP_OFFSET(offThread_bailPtr, Thread, interpSave.bailPtr, 24)
+MTERP_OFFSET(offThread_threadId, Thread, threadId, 36)
+
//40
MTERP_OFFSET(offThread_subMode, \
Thread, interpBreak.ctl.subMode, 40)
diff --git a/vm/mterp/cstubs/stubdefs.cpp b/vm/mterp/cstubs/stubdefs.cpp
index 2e7f1b7..0bffd49 100644
--- a/vm/mterp/cstubs/stubdefs.cpp
+++ b/vm/mterp/cstubs/stubdefs.cpp
@@ -22,7 +22,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-allstubs.cpp b/vm/mterp/out/InterpC-allstubs.cpp
index 8c2d622..56c2fb6 100644
--- a/vm/mterp/out/InterpC-allstubs.cpp
+++ b/vm/mterp/out/InterpC-allstubs.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-armv5te-vfp.cpp b/vm/mterp/out/InterpC-armv5te-vfp.cpp
index 246b16c..5dedf40 100644
--- a/vm/mterp/out/InterpC-armv5te-vfp.cpp
+++ b/vm/mterp/out/InterpC-armv5te-vfp.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-armv5te.cpp b/vm/mterp/out/InterpC-armv5te.cpp
index 38ad1b3..c2fc146 100644
--- a/vm/mterp/out/InterpC-armv5te.cpp
+++ b/vm/mterp/out/InterpC-armv5te.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-armv7-a-neon.cpp b/vm/mterp/out/InterpC-armv7-a-neon.cpp
index 097bcc8..993b891 100644
--- a/vm/mterp/out/InterpC-armv7-a-neon.cpp
+++ b/vm/mterp/out/InterpC-armv7-a-neon.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-armv7-a.cpp b/vm/mterp/out/InterpC-armv7-a.cpp
index dfbc45d..b2256ab 100644
--- a/vm/mterp/out/InterpC-armv7-a.cpp
+++ b/vm/mterp/out/InterpC-armv7-a.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-portable.cpp b/vm/mterp/out/InterpC-portable.cpp
index 6c6da98..fffae08 100644
--- a/vm/mterp/out/InterpC-portable.cpp
+++ b/vm/mterp/out/InterpC-portable.cpp
@@ -1242,7 +1242,7 @@
curMethod = self->interpSave.method;
pc = self->interpSave.pc;
fp = self->interpSave.curFrame;
- retval = self->retval; /* only need for kInterpEntryReturn? */
+ retval = self->interpSave.retval; /* only need for kInterpEntryReturn? */
methodClassDex = curMethod->clazz->pDvmDex;
@@ -5401,6 +5401,6 @@
bail:
ILOGD("|-- Leaving interpreter loop"); // note "curMethod" may be NULL
- self->retval = retval;
+ self->interpSave.retval = retval;
}
diff --git a/vm/mterp/out/InterpC-x86-atom.cpp b/vm/mterp/out/InterpC-x86-atom.cpp
index 81e84b9..2101476 100644
--- a/vm/mterp/out/InterpC-x86-atom.cpp
+++ b/vm/mterp/out/InterpC-x86-atom.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/out/InterpC-x86.cpp b/vm/mterp/out/InterpC-x86.cpp
index 4cf6603..221000b 100644
--- a/vm/mterp/out/InterpC-x86.cpp
+++ b/vm/mterp/out/InterpC-x86.cpp
@@ -392,7 +392,7 @@
* Redefine what used to be local variable accesses into Thread struct
* references. (These are undefined down in "footer.cpp".)
*/
-#define retval self->retval
+#define retval self->interpSave.retval
#define pc self->interpSave.pc
#define fp self->interpSave.curFrame
#define curMethod self->interpSave.method
diff --git a/vm/mterp/portable/enddefs.cpp b/vm/mterp/portable/enddefs.cpp
index 5c4af52..ef28e2f 100644
--- a/vm/mterp/portable/enddefs.cpp
+++ b/vm/mterp/portable/enddefs.cpp
@@ -3,5 +3,5 @@
bail:
ILOGD("|-- Leaving interpreter loop"); // note "curMethod" may be NULL
- self->retval = retval;
+ self->interpSave.retval = retval;
}
diff --git a/vm/mterp/portable/entry.cpp b/vm/mterp/portable/entry.cpp
index dddf136..21b3b7e 100644
--- a/vm/mterp/portable/entry.cpp
+++ b/vm/mterp/portable/entry.cpp
@@ -35,7 +35,7 @@
curMethod = self->interpSave.method;
pc = self->interpSave.pc;
fp = self->interpSave.curFrame;
- retval = self->retval; /* only need for kInterpEntryReturn? */
+ retval = self->interpSave.retval; /* only need for kInterpEntryReturn? */
methodClassDex = curMethod->clazz->pDvmDex;