From cf4a20cf0cbc53f03a5b16c7152bbb29907f7108 Mon Sep 17 00:00:00 2001 From: buzbee Date: Wed, 25 May 2011 14:21:14 -0700 Subject: [PATCH] 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 --- vm/Thread.h | 1 - vm/compiler/codegen/CodegenFactory.cpp | 15 +++++++++------ vm/compiler/codegen/arm/CodegenDriver.cpp | 2 +- vm/compiler/codegen/arm/Thumb/Gen.cpp | 6 +++--- vm/interp/Interp.cpp | 2 +- vm/interp/InterpState.h | 1 + vm/interp/Jit.cpp | 4 ++-- vm/mterp/common/asm-constants.h | 16 ++++++++-------- vm/mterp/cstubs/stubdefs.cpp | 2 +- vm/mterp/out/InterpC-allstubs.cpp | 2 +- vm/mterp/out/InterpC-armv5te-vfp.cpp | 2 +- vm/mterp/out/InterpC-armv5te.cpp | 2 +- vm/mterp/out/InterpC-armv7-a-neon.cpp | 2 +- vm/mterp/out/InterpC-armv7-a.cpp | 2 +- vm/mterp/out/InterpC-portable.cpp | 4 ++-- vm/mterp/out/InterpC-x86-atom.cpp | 2 +- vm/mterp/out/InterpC-x86.cpp | 2 +- vm/mterp/portable/enddefs.cpp | 2 +- vm/mterp/portable/entry.cpp | 2 +- 19 files changed, 37 insertions(+), 34 deletions(-) diff --git a/vm/Thread.h b/vm/Thread.h index 57ae24fad..a4c64eca9 100644 --- a/vm/Thread.h +++ b/vm/Thread.h @@ -118,7 +118,6 @@ struct Thread { * 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 61e29d7d1..f42ae746b 100644 --- a/vm/compiler/codegen/CodegenFactory.cpp +++ b/vm/compiler/codegen/CodegenFactory.cpp @@ -57,7 +57,7 @@ static void loadValueDirect(CompilationUnit *cUnit, RegLocation rlSrc, 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 @@ static void loadValueDirectWide(CompilationUnit *cUnit, RegLocation rlSrc, 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 @@ static RegLocation loadValue(CompilationUnit *cUnit, RegLocation rlSrc, 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 @@ static void storeValue(CompilationUnit *cUnit, RegLocation rlDest, 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 @@ static RegLocation loadValueWide(CompilationUnit *cUnit, RegLocation rlSrc, 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 @@ static void storeValueWide(CompilationUnit *cUnit, RegLocation rlDest, 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 1e5f4c7c9..ac5936007 100644 --- a/vm/compiler/codegen/arm/CodegenDriver.cpp +++ b/vm/compiler/codegen/arm/CodegenDriver.cpp @@ -3678,7 +3678,7 @@ static bool handleExecuteInlineC(CompilationUnit *cUnit, MIR *mir) 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<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 b782275d2..999795347 100644 --- a/vm/interp/InterpState.h +++ b/vm/interp/InterpState.h @@ -99,6 +99,7 @@ struct InterpSaveState { 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 bb8d6d7ee..9632e9723 100644 --- a/vm/interp/Jit.cpp +++ b/vm/interp/Jit.cpp @@ -96,7 +96,7 @@ void* dvmSelfVerificationSaveState(const u2* pc, u4* fp, // 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 @@ void* dvmSelfVerificationRestoreState(const u2* pc, u4* fp, 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 f9fb928b2..bee7d11a0 100644 --- a/vm/mterp/common/asm-constants.h +++ b/vm/mterp/common/asm-constants.h @@ -150,15 +150,15 @@ MTERP_OFFSET(offThread_pc, Thread, interpSave.pc, 0) 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 2e7f1b777..0bffd49f1 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 8c2d622da..56c2fb6b0 100644 --- a/vm/mterp/out/InterpC-allstubs.cpp +++ b/vm/mterp/out/InterpC-allstubs.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 246b16ca0..5dedf40fe 100644 --- a/vm/mterp/out/InterpC-armv5te-vfp.cpp +++ b/vm/mterp/out/InterpC-armv5te-vfp.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 38ad1b37c..c2fc146da 100644 --- a/vm/mterp/out/InterpC-armv5te.cpp +++ b/vm/mterp/out/InterpC-armv5te.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 097bcc8fb..993b891d4 100644 --- a/vm/mterp/out/InterpC-armv7-a-neon.cpp +++ b/vm/mterp/out/InterpC-armv7-a-neon.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 dfbc45dab..b2256ab38 100644 --- a/vm/mterp/out/InterpC-armv7-a.cpp +++ b/vm/mterp/out/InterpC-armv7-a.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 6c6da9867..fffae08ad 100644 --- a/vm/mterp/out/InterpC-portable.cpp +++ b/vm/mterp/out/InterpC-portable.cpp @@ -1242,7 +1242,7 @@ void dvmInterpretPortable(Thread* self) 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 @@ GOTO_TARGET_END 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 81e84b91b..210147670 100644 --- a/vm/mterp/out/InterpC-x86-atom.cpp +++ b/vm/mterp/out/InterpC-x86-atom.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 4cf66036c..221000bd8 100644 --- a/vm/mterp/out/InterpC-x86.cpp +++ b/vm/mterp/out/InterpC-x86.cpp @@ -392,7 +392,7 @@ static inline bool checkForNullExportPC(Object* obj, u4* fp, const u2* pc) * 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 5c4af5273..ef28e2f18 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 dddf13688..21b3b7e03 100644 --- a/vm/mterp/portable/entry.cpp +++ b/vm/mterp/portable/entry.cpp @@ -35,7 +35,7 @@ void dvmInterpretPortable(Thread* self) 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; -- 2.11.0